GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-99240: Fix double-free bug in Argument Clinic str_converter generated code by colorfulappl · Pull Request #99241 · python/cpython

Conversation

@colorfulappl

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

  • Issue: Double-free in Argument Clinic str_converter generated code #99240

Automerge-Triggered-By: GH:erlend-aasland

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@colorfulappl
Fix double-free bug in Argument Clinic str_converter generated code
06b9d4a
@bedevere-bot bedevere-bot mentioned this pull request Nov 8, 2022
Double-free in Argument Clinic str_converter generated code #99240
Closed
@bedevere-bot bedevere-bot added the awaiting review label Nov 8, 2022
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colorfulappl
Add News
67c3078
gpshead
gpshead approved these changes Nov 9, 2022
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Nov 9, 2022
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colorfulappl
Use walrus operator
cf16400
@colorfulappl

colorfulappl commented Nov 9, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland reviewed Nov 9, 2022
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Nov 9, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colorfulappl added 3 commits November 24, 2022 15:14
@colorfulappl
Merge branch 'main' into fix_ac_str_converter_cleanup
adce0ab
@colorfulappl
Rename "post_operations" to "post_parsing"
3ae2aea
@colorfulappl
Add Argument Clinic functional test cases
c3595e1
@colorfulappl

colorfulappl commented Nov 24, 2022

Copy link
Copy Markdown
Contributor Author

Added Argument Clinic functional test (#96002).
erlend-aasland reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
arhadthedev
arhadthedev reviewed Nov 24, 2022
View reviewed changes
Comment thread Modules/clinic/_testclinic.c.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland reviewed Nov 24, 2022
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2022-11-08-15-54-43.gh-issue-99240.MhYwcz.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testclinic.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
colorfulappl and others added 3 commits November 24, 2022 19:21
@colorfulappl @erlend-aasland
Simplify news
946ad3f 
Co-authored-by: Erlend E. Aasland <[email protected]>
@colorfulappl @erlend-aasland
Delete unnecessary function doc
7767139 
Co-authored-by: Erlend E. Aasland <[email protected]>
@colorfulappl @erlend-aasland
Simplify function doc
9d08af5 
Co-authored-by: Erlend E. Aasland <[email protected]>
@colorfulappl
colorfulappl force-pushed the fix_ac_str_converter_cleanup branch from 45886f1 to 9d08af5 Compare November 24, 2022 11:30
@colorfulappl
Do not generate redundant NULL check
aa218c4
erlend-aasland
erlend-aasland reviewed Nov 24, 2022
View reviewed changes
Comment thread Tools/clinic/clinic.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
colorfulappl added 2 commits November 24, 2022 21:32
@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
@colorfulappl
Rerun make clinic
aea779f
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

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colorfulappl reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@erlend-aasland erlend-aasland added the 🤖 automerge label Nov 24, 2022
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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. :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented Nov 24, 2022

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington
miss-islington merged commit 8dbe08e into python:main Nov 24, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label 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

bedevere-bot commented Dec 20, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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
Possible heap-use-after-free in ctypes in Python 3.12 #113576
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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@colorfulappl @bedevere-bot @gpshead @erlend-aasland @miss-islington @arhadthedev
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.