◐ Shell
reader mode source ↗
Skip to content

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

@colorfulappl 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 bedevere-bot mentioned this pull request Nov 8, 2022
Closed
@bedevere-bot

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
gpshead approved these changes Nov 9, 2022
View reviewed changes

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

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.

@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

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

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

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

colorfulappl commented Nov 24, 2022

Copy link
Copy Markdown
Contributor Author

Added Argument Clinic functional test (#96002).

erlend-aasland
erlend-aasland reviewed Nov 24, 2022
View reviewed changes

@erlend-aasland 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.

Hide comment

LGTM with minor nitpicks :)

@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 colorfulappl force-pushed the fix_ac_str_converter_cleanup branch from 45886f1 to 9d08af5 Compare November 24, 2022 11:30
@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

@erlend-aasland 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.

Hide comment

Thanks!

@erlend-aasland

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

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

miss-islington commented Nov 24, 2022

Copy link
Copy Markdown
Contributor

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

@miss-islington miss-islington merged commit 8dbe08e into python:main Nov 24, 2022
@bedevere-bot

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
@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

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
@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
Closed
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