gh-131998: Fix `NULL` dereference when using an unbound method descriptor in a specialized code path #132000

ZeroIntensity · 2025-04-02T02:25:39Z

Issue: The interpreter crashes when specializing bound method calls on unbound objects #131998

sobolevn

(not a full review)

sobolevn · 2025-04-02T07:25:18Z

Lib/test/test_types.py

+        # GH-131998: The specialized instruction would get tricked into dereferencing
+        # a bound "self" that didn't exist if subsequently called unbound.
+        code = """if True:
+        import glob


Do we need this import?

Yeah, the optimizer is finicky and I can't get it to reliably reproduce without the import. I'll add a comment for clarity.

I reproduce the crash in a reliable way without the import glob.

Does it reliably reproduce in the test case? I think it only happens if you've already got some compiled bytecode to trigger the optimizer, which isn't the case in CI.

You can replace import glob with just this code copied from Lib/types.py (a dependency of glob):

# CellType comes from types.py def _cell_factory(): a = 1 def f(): nonlocal a return f.__closure__[0] CellType = type(_cell_factory())

I'm ok-ish with keeping import glob, but add a comment explaining the purpose of this unused import.

Does it reliably reproduce in the test case? I think it only happens if you've already got some compiled bytecode to trigger the optimizer, which isn't the case in CI.

Sorry, I mean that I can reproduce the crash without import glob when I run a script: ./python reproducer.py. I confirm that for this test case, import glob (or the code example that I proposed) is needed to trigger the bug.

I added a comment.

This import shouldn't be needed with the changes below.

Lib/test/test_types.py

Co-authored-by: sobolevn <[email protected]>

Lib/test/test_types.py

Co-authored-by: Victor Stinner <[email protected]>

markshannon

We need to guard total_args before reading self.

This applies to CALL_METHOD_DESCRIPTOR_FAST as well.

Python/bytecodes.c

bedevere-app · 2025-04-02T14:16:57Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ZeroIntensity · 2025-04-02T21:00:07Z

I have made the requested changes; please review again

bedevere-app · 2025-04-02T21:00:12Z

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

python-cla-bot · 2025-04-06T13:56:04Z

All commit authors signed the Contributor License Agreement.

hugovk · 2025-04-07T06:12:30Z

This PR had a merge conflict.

Next 3.13/3.14 releases are tomorrow.

ZeroIntensity · 2025-04-07T13:17:51Z

Fixed the conflicts.

Friendly ping @markshannon -- this is a blocker for 3.13.3.

markshannon

I think we can remove the import glob by filling the stack with lists before the call.

Lib/test/test_types.py

markshannon · 2025-04-08T09:19:11Z

Lib/test/test_types.py

+        # GH-131998: The specialized instruction would get tricked into dereferencing
+        # a bound "self" that didn't exist if subsequently called unbound.
+        code = """if True:
+        import glob


This import shouldn't be needed with the changes below.

Lib/test/test_types.py

bedevere-app · 2025-04-08T09:38:26Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

Co-authored-by: Mark Shannon <[email protected]>

markshannon · 2025-04-08T09:42:14Z

FTR, here's the reproducer on main without importing glob.

>>> def call(part):
...     [] + ([] + [])
...     part.pop()
...
>>> for _ in range(3):
...     call(['a'])
...
>>> call(list)
Segmentation fault (core dumped)

ZeroIntensity · 2025-04-08T09:46:27Z

I'm not going to trigger the noisy bot again, but this should be good to go.

miss-islington-app · 2025-04-08T10:31:47Z

Thanks @ZeroIntensity for the PR, and @Yhg1s for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington-app · 2025-04-08T10:31:50Z

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

cherry_picker ac3c439cdfee8452f2bcceacd67a1f4e423ac3cf 3.13

ZeroIntensity · 2025-04-08T10:32:35Z

I'll deal with the backport.

…method descriptor in a specialized code path (pythonGH-132000) (cherry picked from commit ac3c439) Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: sobolevn <[email protected]> Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Mark Shannon <[email protected]>

bedevere-app · 2025-04-08T10:39:08Z

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

… descriptor in a specialized code path (GH-132000) (#132262) (cherry picked from commit ac3c439) Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: sobolevn <[email protected]> Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Mark Shannon <[email protected]>

…descriptor in a specialized code path (python#132000) Co-authored-by: sobolevn <[email protected]> Co-authored-by: Victor Stinner <[email protected]> Co-authored-by: Mark Shannon <[email protected]>

ZeroIntensity added 4 commits April 1, 2025 22:13

Fix crash when the stackref is NULL.

345ad57

Add a test.

1914aca

Add a test case.

5101034

Add blurb

Verified

This commit was signed with the committer’s verified signature.

tmshort Todd Short

GPG key ID: 5EE35E207F7BED27

Verified
Learn about vigilant mode

Loading
Loading status checks…

2233afe

bedevere-app bot mentioned this pull request Apr 2, 2025

The interpreter crashes when specializing bound method calls on unbound objects #131998

Closed

sobolevn reviewed Apr 2, 2025

View reviewed changes

vstinner added the needs backport to 3.13 label Apr 2, 2025

ZeroIntensity and others added 3 commits April 2, 2025 05:50

Update Lib/test/test_types.py

Verified

This commit was signed with the committer’s verified signature.

tmshort Todd Short

GPG key ID: 5EE35E207F7BED27

Verified
Learn about vigilant mode

Loading
Loading status checks…

538c83b

Co-authored-by: sobolevn <[email protected]>

Try removing the 'import glob' now.

04b61df

Add back the 'import glob', it doesn't reproduce.

Loading
Loading status checks…

cd12c28

vstinner reviewed Apr 2, 2025

View reviewed changes

Lib/test/test_types.py Outdated Show resolved Hide resolved

Update Lib/test/test_types.py

Loading
Loading status checks…

f73dd59

Co-authored-by: Victor Stinner <[email protected]>

ZeroIntensity marked this pull request as ready for review April 2, 2025 10:12

ZeroIntensity requested a review from markshannon as a code owner April 2, 2025 10:12

bedevere-app bot added the awaiting review label Apr 2, 2025

markshannon requested changes Apr 2, 2025

View reviewed changes

Python/bytecodes.c Outdated Show resolved Hide resolved

bedevere-app bot removed the awaiting review label Apr 2, 2025

bedevere-app bot added the awaiting changes label Apr 2, 2025

bedevere-app bot mentioned this pull request Apr 2, 2025

gh-132011: Fix crash on invalid CALL_LIST_APPEND deoptimization #132018

Merged

Check for 'total_args' instead of 'self'

Loading
Loading status checks…

a2a07ea

bedevere-app bot added awaiting change review and removed awaiting changes labels Apr 2, 2025

bedevere-app bot requested a review from markshannon April 2, 2025 21:00

ZeroIntensity added 2 commits April 7, 2025 06:52

Merge branch 'main' into pythongh-131998-method-descriptor-crash

Loading
Loading status checks…

aefcf8a

Fix the damn import.

Loading
Loading status checks…

0eb6933

markshannon requested changes Apr 8, 2025

View reviewed changes

bedevere-app bot removed the awaiting change review label Apr 8, 2025

bedevere-app bot added the awaiting changes label Apr 8, 2025

ZeroIntensity and others added 3 commits April 8, 2025 05:40

Apply suggestions from code review

Loading
Loading status checks…

d1d5c0a

Co-authored-by: Mark Shannon <[email protected]>

Remove 'import glob'

Loading
Loading status checks…

0261714

Fix stray newline change.

Loading
Loading status checks…

99037ae

Remove comment.

Loading
Loading status checks…

d87de41

Yhg1s added awaiting core review and removed awaiting changes labels Apr 8, 2025

Merge branch 'main' into pythongh-131998-method-descriptor-crash

Loading
Loading status checks…

363acec

Yhg1s enabled auto-merge (squash) April 8, 2025 10:08

Yhg1s merged commit ac3c439 into python:main Apr 8, 2025
64 checks passed

bedevere-app bot removed the awaiting core review label Apr 8, 2025

miss-islington-app bot assigned Yhg1s Apr 8, 2025

bedevere-app bot removed the needs backport to 3.13 label Apr 8, 2025

ZeroIntensity deleted the gh-131998-method-descriptor-crash branch April 8, 2025 10:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-131998: Fix `NULL` dereference when using an unbound method descriptor in a specialized code path #132000

gh-131998: Fix `NULL` dereference when using an unbound method descriptor in a specialized code path #132000

ZeroIntensity commented Apr 2, 2025 •

edited by bedevere-app bot

Loading

sobolevn left a comment

sobolevn Apr 2, 2025

ZeroIntensity Apr 2, 2025

vstinner Apr 2, 2025

ZeroIntensity Apr 2, 2025

vstinner Apr 2, 2025

vstinner Apr 2, 2025

vstinner Apr 2, 2025

ZeroIntensity Apr 2, 2025

markshannon Apr 8, 2025

markshannon left a comment

bedevere-app bot commented Apr 2, 2025

ZeroIntensity commented Apr 2, 2025

bedevere-app bot commented Apr 2, 2025

python-cla-bot bot commented Apr 6, 2025

hugovk commented Apr 7, 2025

ZeroIntensity commented Apr 7, 2025

markshannon left a comment

markshannon Apr 8, 2025

bedevere-app bot commented Apr 8, 2025

markshannon commented Apr 8, 2025

ZeroIntensity commented Apr 8, 2025

miss-islington-app bot commented Apr 8, 2025

miss-islington-app bot commented Apr 8, 2025

ZeroIntensity commented Apr 8, 2025

bedevere-app bot commented Apr 8, 2025

gh-131998: Fix NULL dereference when using an unbound method descriptor in a specialized code path #132000

gh-131998: Fix NULL dereference when using an unbound method descriptor in a specialized code path #132000

Conversation

ZeroIntensity commented Apr 2, 2025 • edited by bedevere-app bot Loading

sobolevn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon left a comment

Choose a reason for hiding this comment

bedevere-app bot commented Apr 2, 2025

ZeroIntensity commented Apr 2, 2025

bedevere-app bot commented Apr 2, 2025

python-cla-bot bot commented Apr 6, 2025

hugovk commented Apr 7, 2025

ZeroIntensity commented Apr 7, 2025

markshannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-app bot commented Apr 8, 2025

markshannon commented Apr 8, 2025

ZeroIntensity commented Apr 8, 2025

miss-islington-app bot commented Apr 8, 2025

miss-islington-app bot commented Apr 8, 2025

ZeroIntensity commented Apr 8, 2025

bedevere-app bot commented Apr 8, 2025

gh-131998: Fix `NULL` dereference when using an unbound method descriptor in a specialized code path #132000

gh-131998: Fix `NULL` dereference when using an unbound method descriptor in a specialized code path #132000

ZeroIntensity commented Apr 2, 2025 •

edited by bedevere-app bot

Loading