FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-125010: Fix `use-after-free` in AST `repr()` by tomasr8 · Pull Request #125015 · python/cpython · GitHub

Repository navigation

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

Merged
JelleZijlstra merged 7 commits into
python:mainfrom
tomasr8:ast-repr-fix
Oct 6, 2024
Merged

JelleZijlstra merged 7 commits into
python:mainfrom
tomasr8:ast-repr-fix

Conversation

tomasr8 commented Oct 5, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

Eclips4 commented Oct 5, 2024

Copy link
Copy Markdown
Member

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

tomasr8 commented Oct 5, 2024

Copy link
Copy Markdown
Member Author

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 commented Oct 6, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Oct 6, 2024

Copy link
Copy Markdown
Member Author

Test added :)

Comment thread Lib/test/test_ast/test_ast.py Outdated
Comment thread Lib/test/test_ast/test_ast.py Outdated
Comment thread Lib/test/test_ast/test_ast.py Outdated

AlexWaygood left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
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 commented Oct 6, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

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.

AlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Eclips4 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. Thank you Tomas.

Comment thread Parser/asdl_c.py Outdated

ZeroIntensity left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, thanks!

Eclips4 commented Oct 6, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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

JelleZijlstra merged commit a1be83d into python:main Oct 6, 2024
tomasr8 deleted the ast-repr-fix branch October 6, 2024 20:11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL