Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-125010: Fix use-after-free in AST repr() #125015

Merged
merged 7 commits into from
Oct 6, 2024

Conversation

tomasr8
Copy link
Member

@tomasr8 tomasr8 commented Oct 5, 2024

@Eclips4
Copy link
Member

Eclips4 commented Oct 5, 2024

Could you add a test case for that? I think testing that the initial reproducer doesn't crash would be enough

@tomasr8
Copy link
Member Author

tomasr8 commented Oct 5, 2024

Could you add a test case for that? I think testing that the initial reproducer doesn't crash would be enough

Yep, but maybe I'll try to make the reproducer a bit smaller first, it's quite big 😅 :

>>> s = open("reproducer.txt").read()
>>> len(s)
782813

@Eclips4
Copy link
Member

Eclips4 commented Oct 6, 2024

Could you add a test case for that? I think testing that the initial reproducer doesn't crash would be enough

Yep, but maybe I'll try to make the reproducer a bit smaller first, it's quite big 😅 :

>>> s = open("reproducer.txt").read()
>>> len(s)
782813

Here's how it can be reduced to avoid using the file:

import ast
repro = "{0x0" + "e" * 250_000 + "%" + "e" * 250_000 + "1j}"
print(ast.literal_eval(repro))

@tomasr8
Copy link
Member Author

tomasr8 commented Oct 6, 2024

Test added :)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

The bug doesn't really have anything to do with ast.literal_eval, so I think the test would be clearer if it didn't use ast.literal_eval. I can trigger a crash with a simpler repro that only uses ast.parse (and with a shorter source snippet passed to ast.parse()):

~/dev/cpython (main)⚡ [134] % ./python.exe -c 'import ast; repr(ast.parse("0x0" + "e" * 4_000, mode="eval"))' 
./Include/refcount.h:474: _Py_NegativeRefcount: Assertion failed: object has negative ref count
<object at 0x1398df420 is freed>
Fatal Python error: _PyObject_AssertFailed: _PyObject_AssertFailed
Python runtime state: initialized
ValueError: Exceeds the limit (4300 digits) for integer string conversion; use sys.set_int_max_str_digits() to increase the limit

Current thread 0x00000001f39d4f40 (most recent call first):
  File "<string>", line 1 in <module>
zsh: abort      ./python.exe -c

@AlexWaygood
Copy link
Member

Ah, in fact, here's a repro without even ast.parse():

~/dev/cpython (main)⚡ % ./python.exe -c 'import ast; repr(ast.Constant(value=eval("0x0" + "e" * 4_000)))'
./Include/refcount.h:474: _Py_NegativeRefcount: Assertion failed: object has negative ref count
<object at 0x15a858220 is freed>
Fatal Python error: _PyObject_AssertFailed: _PyObject_AssertFailed
Python runtime state: initialized
ValueError: Exceeds the limit (4300 digits) for integer string conversion; use sys.set_int_max_str_digits() to increase the limit

Current thread 0x00000001f39d4f40 (most recent call first):
  File "<string>", line 1 in <module>
zsh: abort      ./python.exe -c 

@tomasr8
Copy link
Member Author

tomasr8 commented Oct 6, 2024

I simplified the test to use Alex's smaller reproducer and removed the note about sys.set_int_max_str_digits since the source is now a valid syntax and does not raise the other ValueError anymore.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM! The test looks great and I confirmed it segfaults on main. I'll let one of the AST experts subscribed to this PR approve and merge, though.

Copy link
Member

@Eclips4 Eclips4 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you Tomas.

Parser/asdl_c.py Outdated Show resolved Hide resolved
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Eclips4
Copy link
Member

Eclips4 commented Oct 6, 2024

If @JelleZijlstra has no further comments, I'll merge this tomorrow.

@JelleZijlstra JelleZijlstra merged commit a1be83d into python:main Oct 6, 2024
36 checks passed
@tomasr8 tomasr8 deleted the ast-repr-fix branch October 6, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants