Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-99240: Fix double-free bug in Argument Clinic str_converter generated code #99241

Merged
merged 13 commits into from
Nov 24, 2022
Merged

gh-99240: Fix double-free bug in Argument Clinic str_converter generated code #99241

merged 13 commits into from
Nov 24, 2022

Conversation

colorfulappl
Copy link
Contributor

@colorfulappl colorfulappl commented Nov 8, 2022 •
edited by miss-islington
Loading

Fix double-free bug mentioned at #99240,
by moving memory clean up out of "exit" label.

Automerge-Triggered-By: GH:erlend-aasland

@bedevere-bot bedevere-bot mentioned this pull request Nov 8, 2022
Closed
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

gpshead
gpshead approved these changes Nov 9, 2022
View reviewed changes
Copy link
Member

@gpshead gpshead left a 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.

Tools/clinic/clinic.py Outdated Show resolved Hide resolved
@gpshead gpshead self-assigned this Nov 9, 2022
@gpshead gpshead added topic-argument-clinic type-bug An unexpected behavior, bug, or error labels Nov 9, 2022
@gpshead
Copy link
Member

gpshead commented Nov 9, 2022

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
Copy link
Contributor Author

colorfulappl commented Nov 9, 2022 •
edited
Loading

does that mean we don't have any actual standard library argument clinic uses of this str conversion via codec functionality yet?

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.

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Nov 9, 2022 •
edited
Loading

Also, my preference, as stated in #96178 (comment), would be to first merge that PR (with a minimal 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.

@colorfulappl
Copy link
Contributor Author

Added Argument Clinic functional test (#96002).

erlend-aasland
erlend-aasland reviewed Nov 24, 2022
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a 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 Outdated Show resolved Hide resolved
Modules/_testclinic.c Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
colorfulappl and others added 3 commits November 24, 2022 19:21
@colorfulappl @erlend-aasland
Simplify news
946ad3f 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@colorfulappl @erlend-aasland
Delete unnecessary function doc
7767139 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@colorfulappl @erlend-aasland
Simplify function doc
9d08af5 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@colorfulappl
Polish comment
485c7aa
@colorfulappl
Merge branch 'main' into fix_ac_str_converter_cleanup
18ae693 
# Conflicts:
#	Lib/test/test_clinic.py
#	Modules/_testclinic.c
#	Modules/clinic/_testclinic.c.h
erlend-aasland
erlend-aasland approved these changes Nov 24, 2022
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@erlend-aasland
Copy link
Contributor

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.

@colorfulappl, are you up for fixing _PyArg_ParseStack while you're at it?

@colorfulappl
Copy link
Contributor Author

are you up for fixing _PyArg_ParseStack while you're at it?

I have not started yet, but I am willing to have a try. :)

@miss-islington
Copy link
Contributor

Status check is done, and it's a success ✅.

@miss-islington miss-islington merged commit 8dbe08e into python:main Nov 24, 2022
colorfulappl added a commit to colorfulappl/cpython that referenced this pull request Dec 20, 2022
@colorfulappl
[3.11] pythongh-99240: Fix double-free bug in Argument Clinic str_con…
32f7fad 
…verter generated code (pythonGH-99241)

(cherry picked from commit 8dbe08e)

Fix double-free bug mentioned at pythonGH-99240, by moving memory clean up out of "exit" label.
@bedevere-bot
Copy link

GH-100352 is a backport of this pull request to the 3.11 branch.

colorfulappl added a commit to colorfulappl/cpython that referenced this pull request Dec 20, 2022
@colorfulappl
[3.10] pythongh-99240: Fix double-free bug in Argument Clinic str_con…
0c00e33 
…verter generated code (pythonGH-99241)

(cherry picked from commit 8dbe08e)

Fix double-free bug mentioned at pythonGH-99240, by moving memory clean up out of "exit" label.
@bedevere-bot
Copy link

GH-100353 is a backport of this pull request to the 3.10 branch.

kumaraditya303 pushed a commit that referenced this pull request Dec 20, 2022
@colorfulappl
[3.11] gh-99240: Fix double-free bug in Argument Clinic str_converter…
ba8e30c 
… generated code (GH-99241) (#100352)

(cherry picked from commit 8dbe08e)

Fix double-free bug mentioned at GH-99240, by moving memory clean up out of "exit" label.
kumaraditya303 pushed a commit that referenced this pull request Dec 20, 2022
@colorfulappl
[3.10] gh-99240: Fix double-free bug in Argument Clinic str_converter…
53063b7 
… generated code (GH-99241) (#100353)

(cherry picked from commit 8dbe08e)

Fix double-free bug mentioned at GH-99240, by moving memory clean up out of "exit" label.
@jiridanek jiridanek mentioned this pull request Jan 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arhadthedev arhadthedev arhadthedev left review comments

@gpshead gpshead gpshead approved these changes

@erlend-aasland erlend-aasland erlend-aasland approved these changes

Assignees

@gpshead gpshead

Labels
topic-argument-clinic type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@colorfulappl @bedevere-bot @gpshead @erlend-aasland @miss-islington @arhadthedev