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-109118: Fix runtime crash when NameError happens in PEP 695 function#109123

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
JelleZijlstra merged 4 commits intopython:mainfromJelleZijlstra:fixinterpcrash
Sep 9, 2023

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstraJelleZijlstra commentedSep 8, 2023
edited by bedevere-bot
Loading

@JelleZijlstraJelleZijlstra added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelSep 8, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@JelleZijlstra for commit3c448e8 🤖

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelSep 8, 2023
@JelleZijlstraJelleZijlstra added the needs backport to 3.12only security fixes labelSep 8, 2023
@JelleZijlstra
Copy link
MemberAuthor

This is also wrong, trying to figure out the right way to handle the refcounting here now.

@JelleZijlstra
Copy link
MemberAuthor

The problem is that the way we need to manage the refcount on mod_or_class_dict is different for the two instructions that use the_LOAD_FROM_DICT_OR_GLOBALS op: in the LOAD_NAME case, we own the only reference to the dict, so we need to decref it in all cases, including the error conditions. But in the LOAD_FROM_DICT_OR_GLOBALS case, the dict is still on the stack until the end of the instruction, so in the error case, it gets decrefed automatically during stack unwinding. The code that is currently in main is correct for the LOAD_NAME case, but we get a double decref for thegoto error branches. The code in my initial PR is correct for LOAD_FROM_DICT_OR_GLOBALS, but it doesn't DECREF properly in the error case for LOAD_NAME, so we leak references.

I couldn't figure out how to get the refcounting right in both cases with the existing macros, so I got rid of the macros and duplicated the code instead, which made it possible to get the correct refcounting in both conditions.

@JelleZijlstraJelleZijlstra added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelSep 8, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@JelleZijlstra for commit882ada7 🤖

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelSep 8, 2023
@JelleZijlstra
Copy link
MemberAuthor

A few buildbots are now segfaulting ontest_sys_settrace. Doesn't reproduce locally on macOS.

@JelleZijlstra
Copy link
MemberAuthor

The buildbot failures seem unrelated, filed#109143.

Copy link
Member

@carljmcarljm left a comment

Choose a reason for hiding this comment

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

I think the general pattern inbytecodes.c is supposed to be "decref all inputs prior to checking any error cases, and then useERROR_IF pseudo-macro to jump to error" -- andERROR_IF will jump topop_X_error labels to ensure the inputs get popped off the stack before the stack-emptying-with-decref happens. But when I try that approach here (instead of the changes in this PR), it fails; the cases generator generatesgoto pop_1_error in the error cases in bothLOAD_NAME andLOAD_FROM_DICT_OR_GLOBALS, even though in theLOAD_NAME case the "input" to_LOAD_FROM_DICT_OR_GLOBALS is not ever on the stack, sincemacro() just hooks it up directly to the output of_LOAD_LOCAL.

Ithink this is just a bug in the cases generator; it should realize in themacro() case that it needs to account for where the input actually came from in the previous uop when it is deciding how many items need to be popped off the stack at an ERROR_IF. cc@gvanrossum to confirm or contradict my analysis here.

But given that we want to minimize risk at this point, it may be best (at least for 3.12) to go with this code-duplication fix rather than trying to make changes to the cases generator.

@JelleZijlstra
Copy link
MemberAuthor

Thanks for the review! Yes, there's probably a way to get this to work, but especially since the cases generator is quite different on main and on 3.12 by now, it's probably better to accept the code duplication and go with my current solution.

@gvanrossum
Copy link
Member

Ithink this is just a bug in the cases generator; it should realize in themacro() case that it needs to account for where the input actually came from in the previous uop when it is deciding how many items need to be popped off the stack at an ERROR_IF. cc@gvanrossum to confirm or contradict my analysis here.

Oh dear. I'm afraid I've been deep into other things, and it'll take me some time to confirm this.

But given that we want to minimize risk at this point, it may be best (at least for 3.12) to go with this code-duplication fix rather than trying to make changes to the cases generator.

That definitely feels like the safest approach -- a fix to the cases generator likely can't be backported, since the version on main was extensively refactored during the rc phase.

carljm reacted with thumbs up emoji

@JelleZijlstraJelleZijlstraenabled auto-merge (squash)September 9, 2023 01:14
@JelleZijlstraJelleZijlstra merged commit17f9941 intopython:mainSep 9, 2023
@miss-islington
Copy link
Contributor

Thanks@JelleZijlstra for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry,@JelleZijlstra, I could not cleanly backport this to3.12 due to a conflict.
Please backport usingcherry_picker on command line.
cherry_picker 17f994174de9211b2baaff217eeb1033343230fc 3.12

@bedevere-bot
Copy link

GH-109173 is a backport of this pull request to the3.12 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.12only security fixes labelSep 9, 2023
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull requestSep 9, 2023
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull requestSep 9, 2023
…EP 695 function (pythonGH-109123).(cherry picked from commit17f9941)Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Yhg1s pushed a commit that referenced this pull requestSep 12, 2023
… function (GH-109123) (#109173)*gh-109118: Fix runtime crash when NameError happens in PEP 695 function (#109123)(cherry picked from commit17f9941)* [3.12]gh-109118: Fix runtime crash when NameError happens in PEP 695 function (GH-109123).(cherry picked from commit17f9941)Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@carljmcarljmcarljm approved these changes

Assignees

@JelleZijlstraJelleZijlstra

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@JelleZijlstra@bedevere-bot@gvanrossum@miss-islington@carljm

[8]ページ先頭

©2009-2025 Movatter.jp