← 返回首页
gh-99240: Fix double-free bug in Argument Clinic str_converter generated code by colorfulappl · Pull Request #99241 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

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

Merged
miss-islington merged 13 commits into
python:mainfrom
colorfulappl:fix_ac_str_converter_cleanup
Nov 24, 2022
Merged

gh-99240: Fix double-free bug in Argument Clinic str_converter generated code#99241
miss-islington merged 13 commits into
python:mainfrom
colorfulappl:fix_ac_str_converter_cleanup

Conversation

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

Copy link
Copy Markdown
Contributor

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

Automerge-Triggered-By: GH:erlend-aasland

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

bedevere-bot commented Nov 8, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

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

gpshead approved these changes Nov 9, 2022
View reviewed changes

gpshead left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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.

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

gpshead commented Nov 9, 2022

Copy link
Copy Markdown
Member

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 commented Nov 9, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 reviewed Nov 9, 2022
View reviewed changes
Comment thread Tools/clinic/clinic.py Outdated Show resolved Hide resolved

erlend-aasland commented Nov 9, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Nov 24, 2022

Copy link
Copy Markdown
Contributor Author

Added Argument Clinic functional test (#96002).

arhadthedev reviewed Nov 24, 2022
View reviewed changes
Comment thread Modules/clinic/_testclinic.c.h Outdated Show resolved Hide resolved
erlend-aasland reviewed Nov 24, 2022
View reviewed changes

erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

LGTM with minor nitpicks :)

Comment thread Misc/NEWS.d/next/Library/2022-11-08-15-54-43.gh-issue-99240.MhYwcz.rst Outdated Show resolved Hide resolved
Comment thread Modules/_testclinic.c Outdated Show resolved Hide resolved
Comment thread Tools/clinic/clinic.py Outdated Show resolved Hide resolved
colorfulappl and others added 3 commits November 24, 2022 19:21
Simplify news …
946ad3f
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Delete unnecessary function doc …
7767139
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Simplify function doc …
9d08af5
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
colorfulappl force-pushed the fix_ac_str_converter_cleanup branch from 45886f1 to 9d08af5 Compare November 24, 2022 11:30
erlend-aasland reviewed Nov 24, 2022
View reviewed changes
Comment thread Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Polish comment
485c7aa
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 approved these changes Nov 24, 2022
View reviewed changes

erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Thanks!

erlend-aasland commented Nov 24, 2022

Copy link
Copy Markdown
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 commented Nov 24, 2022

Copy link
Copy Markdown
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 commented Nov 24, 2022

Copy link
Copy Markdown
Contributor

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

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
[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 commented Dec 20, 2022

Copy link
Copy Markdown

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
[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 commented Dec 20, 2022

Copy link
Copy Markdown

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
[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
[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 mentioned this pull request Jan 2, 2024
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

arhadthedev arhadthedev left review comments
gpshead gpshead approved these changes
erlend-aasland erlend-aasland approved these changes

Assignees

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

Footer

© 2026 GitHub, Inc.