Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

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

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged

Conversation

colorfulappl
Copy link
Contributor

@colorfulapplcolorfulappl commentedNov 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
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

Copy link
Member

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

erlend-aasland reacted with thumbs up emoji
@gpsheadgpshead self-assigned thisNov 9, 2022
@gpsheadgpshead added topic-argument-clinic type-bugAn unexpected behavior, bug, or error labelsNov 9, 2022
@gpshead
Copy link
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
Copy link
ContributorAuthor

colorfulappl commentedNov 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.

gpshead reacted with thumbs up emoji

@erlend-aasland
Copy link
Contributor

erlend-aasland commentedNov 9, 2022
edited
Loading

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.

@colorfulappl
Copy link
ContributorAuthor

Added Argument Clinic functional test (#96002).

erlend-aasland reacted with rocket emoji

Copy link
Contributor

@erlend-aaslanderlend-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 :)

colorfulappland others added3 commitsNovember 24, 2022 19:21
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>
@colorfulapplcolorfulapplforce-pushed thefix_ac_str_converter_cleanup branch from45886f1 to9d08af5CompareNovember 24, 2022 11:30
# Conflicts:#Lib/test/test_clinic.py#Modules/_testclinic.c#Modules/clinic/_testclinic.c.h
Copy link
Contributor

@erlend-aaslanderlend-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!

colorfulappl reacted with hooray emoji
@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
ContributorAuthor

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-islingtonmiss-islington merged commit8dbe08e intopython:mainNov 24, 2022
colorfulappl added a commit to colorfulappl/cpython that referenced this pull requestDec 20, 2022
…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
Copy link

GH-100352 is a backport of this pull request to the3.11 branch.

colorfulappl added a commit to colorfulappl/cpython that referenced this pull requestDec 20, 2022
…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
Copy link

GH-100353 is a backport of this pull request to the3.10 branch.

kumaraditya303 pushed a commit that referenced this pull requestDec 20, 2022
… generated code (GH-99241) (#100352)(cherry picked from commit8dbe08e)Fix double-free bug mentioned atGH-99240, by moving memory clean up out of "exit" label.
kumaraditya303 pushed a commit that referenced this pull requestDec 20, 2022
… generated code (GH-99241) (#100353)(cherry picked from commit8dbe08e)Fix double-free bug mentioned atGH-99240, by moving memory clean up out of "exit" label.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@arhadthedevarhadthedevarhadthedev left review comments

@gpsheadgpsheadgpshead approved these changes

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

Assignees

@gpsheadgpshead

Labels
topic-argument-clinictype-bugAn 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

[8]ページ先頭

©2009-2025 Movatter.jp