gh-130775: Validate negative locations in `ast` #130795

sobolevn · 2025-03-03T17:03:38Z

Issue: Crash in Python/assemble.c:301: write_location_info_entry: Assertion `column >= -1' failed. #130775

picnixz

If we cannot bypass the ast.c layer and directly end up in the assemble.c, then this should be the correct approach (I didn't know where we validate the locations).

picnixz · 2025-03-03T17:06:48Z

Lib/test/test_ast/test_ast.py

+            {'lineno': -2, 'col_offset': 0},
+            {'lineno': 0, 'col_offset': -2},
+            {'lineno': 0, 'col_offset': -2, 'end_col_offset': -2},
+            {'lineno': -2, 'end_lineno': -2, 'col_offset': 0},


Off topic: are we already testing that passing a non-int is fine?

Maybe test with values that are excessively large (either in + or -) just for overflow checks.

I will open a new issue for that, if we don't. Thanks for the idea.

Python/ast.c

Co-authored-by: Victor Stinner <vstinner@python.org>

picnixz · 2025-03-03T18:55:54Z

Misc/NEWS.d/next/Core_and_Builtins/2025-03-03-20-02-45.gh-issue-130775.fEM6T-.rst

+Validate negative :mod:`ast` locations, we only allow ``-1`` as a special
+value.


Since it's a user-facing change, I think we should also have a What's New entry, and a versionchanged in ast.*. Users may be surprised. I also don't know whether you want to backport the change or not.

I don't consider this as a feature, because negative locations do not make sense, they are also documented to be positive numbers in ast.rst

So, I plan to backport :)

yes, I would agree that this is a bug fix for a crash in compile, not a new feature.

It's a new error for ast.parse though.

Hm, looks like ast.parse is not affected:

>>> import ast ... ... tree = ast.Module(body=[ ... ast.Import(names=[ast.alias(name='traceback', lineno=0, col_offset=0)], lineno=0, \ col_offset=-2) ... ], type_ignores=[]) ... >>> ast.parse(tree) Module(body=[Import(names=[alias(name='traceback', asname=None)])], type_ignores=[]) >>> compile(tree, "<string>", "exec") Traceback (most recent call last): File "<python-input-2>", line 1, in <module> compile(tree, "<string>", "exec") ~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^ ValueError: AST node has invalid location (0, 0, -2, -2)

What if you pass optimize=2 to ast.parse?

Yes, this works:

>>> ast.parse(tree, optimize=2) Traceback (most recent call last): File "<python-input-2>", line 1, in <module> ast.parse(tree, optimize=2) ~~~~~~~~~^^^^^^^^^^^^^^^^^^ File "/Users/sobolev/Desktop/cpython/Lib/ast.py", line 53, in parse return compile(source, filename, mode, flags, _feature_version=feature_version, optimize=optimize) ValueError: AST node has invalid location (0, 0, -2, -2)

vstinner

LGTM. I just suggest a shorter test name :-)

vstinner · 2025-03-03T18:56:38Z

Lib/test/test_ast/test_ast.py

@@ -204,6 +204,27 @@ def test_compilation_of_ast_nodes_with_default_end_position_values(self):
        # Check that compilation doesn't crash. Note: this may crash explicitly only on debug mode.
        compile(tree, "<string>", "exec")

+    def test_compilation_of_ast_nodes_with_negative_position_values(self):


Suggested change

def test_compilation_of_ast_nodes_with_negative_position_values(self):

def test_invalid_location(self):

iritkatriel · 2025-03-03T18:41:31Z

Misc/NEWS.d/next/Core_and_Builtins/2025-03-03-20-02-45.gh-issue-130775.fEM6T-.rst

@@ -0,0 +1,2 @@
+Validate negative :mod:`ast` locations, we only allow ``-1`` as a special
+value.


I think this change should appear in what's new in 3.14 as a change to ast.parse() because previously this would not generate an error there.

iritkatriel · 2025-03-03T18:44:52Z

Lib/test/test_ast/test_ast.py

+                    ValueError,
+                    'AST node has invalid location',
+                ):
+                    compile(tree, "<string>", "exec")


I'd test that ast.parse(tree) raises the error as well.

iritkatriel · 2025-03-03T18:55:53Z

Python/assemble.c

@@ -297,6 +297,7 @@ write_location_info_entry(struct assembler* a, location loc, int isize)
    int line_delta = loc.lineno - a->a_lineno;
    int column = loc.col_offset;
    int end_column = loc.end_col_offset;
+    // Values are validated in `VALIDATE_POSITIONS` in `ast.c`:


Suggested change

// Values are validated in `VALIDATE_POSITIONS` in `ast.c`:

iritkatriel · 2025-03-03T18:56:50Z

Python/ast.c

+    if (node->lineno < -1 || node->end_lineno < -1 || \
+        node->col_offset < -1 || node->end_col_offset < -1) { \
+        PyErr_Format(PyExc_ValueError, \
+                     "AST node has invalid location (%d, %d, %d, %d)", \


Suggested change

"AST node has invalid location (%d, %d, %d, %d)", \

"AST node with negative location (%d, %d, %d, %d)", \

JelleZijlstra · 2025-03-03T22:54:21Z

Python/ast.c

@@ -40,6 +40,15 @@ static int validate_typeparam(type_param_ty);
                     node->col_offset, node->end_col_offset, node->lineno, node->end_lineno); \
        return 0; \
    } \
+    if (node->lineno < -1 || node->end_lineno < -1 || \


If we keep allowing -1 I don't see much point in disallowing other values. It's a potential compatibility break for users passing in other invalid values, so we may be better off just making any negative value work.

I remove my LGTM vote, I didn't notice that -1 value is still allowed. I'm not sure about rejecting values <= -2.

pythongh-130775: Validate negative locations in ast

fd105a8

sobolevn requested a review from picnixz March 3, 2025 17:03

sobolevn requested review from Eclips4, isidentical, JelleZijlstra, markshannon and iritkatriel as code owners March 3, 2025 17:03

bedevere-app bot added the awaiting core review label Mar 3, 2025

bedevere-app bot mentioned this pull request Mar 3, 2025

Crash in Python/assemble.c:301: write_location_info_entry: Assertion `column >= -1' failed. #130775

Open

picnixz reviewed Mar 3, 2025

View reviewed changes

vstinner reviewed Mar 3, 2025

View reviewed changes

Python/ast.c Outdated Show resolved Hide resolved

Update Python/ast.c

9d2037e

Co-authored-by: Victor Stinner <vstinner@python.org>

picnixz reviewed Mar 3, 2025

View reviewed changes

vstinner previously approved these changes Mar 3, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Mar 3, 2025

iritkatriel reviewed Mar 3, 2025

View reviewed changes

JelleZijlstra reviewed Mar 3, 2025

View reviewed changes

bedevere-app bot added awaiting review and removed awaiting merge labels Mar 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-130775: Validate negative locations in `ast` #130795

gh-130775: Validate negative locations in `ast` #130795

sobolevn commented Mar 3, 2025 •

edited by bedevere-app bot

Loading

picnixz left a comment

picnixz Mar 3, 2025

sobolevn Mar 3, 2025

picnixz Mar 3, 2025

sobolevn Mar 3, 2025

cfbolz Mar 4, 2025

iritkatriel Mar 4, 2025

sobolevn Mar 4, 2025 •

edited

Loading

iritkatriel Mar 4, 2025

sobolevn Mar 4, 2025

vstinner left a comment

vstinner Mar 3, 2025

iritkatriel Mar 3, 2025

iritkatriel Mar 3, 2025

iritkatriel Mar 3, 2025

iritkatriel Mar 3, 2025

JelleZijlstra Mar 3, 2025

		Validate negative :mod:`ast` locations, we only allow ``-1`` as a special
		value.

	def test_compilation_of_ast_nodes_with_negative_position_values(self):
	def test_invalid_location(self):

		@@ -0,0 +1,2 @@
		Validate negative :mod:`ast` locations, we only allow ``-1`` as a special
		value.

	"AST node has invalid location (%d, %d, %d, %d)", \
	"AST node with negative location (%d, %d, %d, %d)", \

gh-130775: Validate negative locations in ast #130795

Are you sure you want to change the base?

gh-130775: Validate negative locations in ast #130795

Conversation

sobolevn commented Mar 3, 2025 • edited by bedevere-app bot Loading

picnixz 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

sobolevn Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner 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

gh-130775: Validate negative locations in `ast` #130795

gh-130775: Validate negative locations in `ast` #130795

sobolevn commented Mar 3, 2025 •

edited by bedevere-app bot

Loading

sobolevn Mar 4, 2025 •

edited

Loading