Skip to content

gh-139748: fix leaks in AC error paths when using FS-based converters #139765

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
Merged
merged 5 commits into from
Oct 8, 2025
Merged

gh-139748: fix leaks in AC error paths when using FS-based converters #139765

merged 5 commits into from
Oct 8, 2025

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Oct 8, 2025 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Oct 8, 2025
Closed
@picnixz picnixz force-pushed the fix/leak/compile-139748 branch 2 times, most recently from 3478a0a to 42bead3 Compare October 8, 2025 12:07
@AA-Turner
Copy link
Member

Why weren't/aren't these refleaks caught by the refleak buildbots?

A

@picnixz picnixz added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Oct 8, 2025
@picnixz
Copy link
Member Author

picnixz commented Oct 8, 2025

Why weren't/aren't these refleaks caught by the refleak buildbots?

  1. Because those leaks only occur on error paths which are usually not tested.
  2. Even if error paths are tested, if the string is actually immortal, it won't be a leak. You won't see the leak if you use compile(b'', '?', 'exec', optimize=99999999999) because ? is immortal.
  3. For successful paths, there is code inside the implementation that does a manual Py_DECREF on something that is meant to be a borrowed reference!

@picnixz picnixz force-pushed the fix/leak/compile-139748 branch 3 times, most recently from a24bf3b to 9f89608 Compare October 8, 2025 13:43
@picnixz picnixz force-pushed the fix/leak/compile-139748 branch from 9f89608 to bf3d10d Compare October 8, 2025 13:56
vstinner
vstinner approved these changes Oct 8, 2025
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@kumaraditya303 kumaraditya303 merged commit b04a57d into python:main Oct 8, 2025
59 checks passed
@miss-islington-app
Copy link

Thanks @picnixz for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington-app
Copy link

Sorry, @picnixz and @kumaraditya303, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker b04a57deef66ce08233be57d1ab5873388df2cea 3.14

@miss-islington-app
Copy link

Sorry, @picnixz and @kumaraditya303, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker b04a57deef66ce08233be57d1ab5873388df2cea 3.13

kumaraditya303 pushed a commit to kumaraditya303/cpython that referenced this pull request Oct 8, 2025
@picnixz @kumaraditya303
[3.14] pythongh-139748: fix leaks in AC error paths when using unicod…
59a091b 
…e FS-based converters (pythonGH-139765)

(cherry picked from commit b04a57d)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@kumaraditya303 kumaraditya303 mentioned this pull request Oct 8, 2025
Merged
@bedevere-app
Copy link

bedevere-app bot commented Oct 8, 2025

GH-139789 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Oct 8, 2025
kumaraditya303 pushed a commit to kumaraditya303/cpython that referenced this pull request Oct 8, 2025
@picnixz @kumaraditya303
[3.14] pythongh-139748: fix leaks in AC error paths when using unicod…
1b08792 
…e FS-based converters (pythonGH-139765)

(cherry picked from commit b04a57d)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
kumaraditya303 pushed a commit to kumaraditya303/cpython that referenced this pull request Oct 8, 2025
@picnixz @kumaraditya303
[3.14] pythongh-139748: fix leaks in AC error paths when using unicod…
50efd4b 
…e FS-based converters (pythonGH-139765)

(cherry picked from commit b04a57d)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@kumaraditya303 kumaraditya303 mentioned this pull request Oct 8, 2025
Merged
@bedevere-app
Copy link

bedevere-app bot commented Oct 8, 2025

GH-139792 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 8, 2025
kumaraditya303 pushed a commit to kumaraditya303/cpython that referenced this pull request Oct 8, 2025
@picnixz @kumaraditya303
[3.13] pythongh-139748: fix leaks in AC error paths when using unicod…
786b335 
…e FS-based converters (pythonGH-139765)

(cherry picked from commit b04a57d)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
kumaraditya303 added a commit that referenced this pull request Oct 8, 2025
@kumaraditya303 @picnixz
[3.14] gh-139748: fix leaks in AC error paths when using unicode FS-b… (
90cd009 
#139789)

* [3.14] gh-139748: fix leaks in AC error paths when using unicode FS-based converters (GH-139765)
(cherry picked from commit b04a57d)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
kumaraditya303 added a commit that referenced this pull request Oct 8, 2025
@kumaraditya303 @picnixz
[3.13] gh-139748: fix leaks in AC error paths when using unicode FS-b… (
dac827c 
#139792)

* [3.13] gh-139748: fix leaks in AC error paths when using unicode FS-based converters (GH-139765)
(cherry picked from commit b04a57d)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@picnixz picnixz deleted the fix/leak/compile-139748 branch October 11, 2025 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner

Assignees

@kumaraditya303 kumaraditya303

Labels

topic-argument-clinic

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@picnixz @AA-Turner @vstinner @kumaraditya303