Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
gh-99240: Fix double-free bug in Argument Clinic str_converter generated code#99241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
gh-99240: Fix double-free bug in Argument Clinic str_converter generated code#99241
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bedevere-bot commentedNov 8, 2022
Most changes to Pythonrequire a NEWS entry. Please add it using theblurb_it web app or theblurb command-line tool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
overall this looks good, only a minor Python code modernization suggestion.
I do think your second bullet from the bug "If function _PyArg_ParseStack parses failed, assign all the parsed arguments to "NULL" after they are freed, this should be done in _PyArg_ParseStack." is also worth doing to make bugs like this less likely to occur. That can be done in its own PR.
Uh oh!
There was an error while loading.Please reload this page.
Of particular note, since the generated files check in CI passed, does that mean we don't have any actual standard library argument clinic uses of this str conversion via codec functionality yet? |
colorfulappl commentedNov 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes, I have searched this in CPython source code and didn't find any usage of argument clinic str conversion with "encoding" parameter setting. And test for this functionality is added in#96178, which has not been merged yet. |
Uh oh!
There was an error while loading.Please reload this page.
erlend-aasland commentedNov 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Also, my preference, as stated in#96178 (comment), would be tofirst merge that PR (with aminimal test suite), and then for each bugfix PR (such as this one) add both the bugfix and the complementing test. So, I'm suggesting putting this on hold until#96178 is merged. |
Added Argument Clinic functional test (#96002). |
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM with minor nitpicks :)
Misc/NEWS.d/next/Library/2022-11-08-15-54-43.gh-issue-99240.MhYwcz.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
45886f1
to9d08af5
CompareUh oh!
There was an error while loading.Please reload this page.
# Conflicts:#Lib/test/test_clinic.py#Modules/_testclinic.c#Modules/clinic/_testclinic.c.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks!
@colorfulappl, are you up for fixing |
I have not started yet, but I am willing to have a try. :) |
Status check is done, and it's a success ✅. |
…verter generated code (pythonGH-99241)(cherry picked from commit8dbe08e)Fix double-free bug mentioned atpythonGH-99240, by moving memory clean up out of "exit" label.
bedevere-bot commentedDec 20, 2022
GH-100352 is a backport of this pull request to the3.11 branch. |
…verter generated code (pythonGH-99241)(cherry picked from commit8dbe08e)Fix double-free bug mentioned atpythonGH-99240, by moving memory clean up out of "exit" label.
bedevere-bot commentedDec 20, 2022
GH-100353 is a backport of this pull request to the3.10 branch. |
Uh oh!
There was an error while loading.Please reload this page.
Fix double-free bug mentioned at#99240,
by moving memory clean up out of "exit" label.
str_converter
generated code #99240Automerge-Triggered-By: GH:erlend-aasland