Repository navigation
Crash in Python/assemble.c:301: write_location_info_entry: Assertion `column >= -1' failed. #130775
Description
Activity
I'm tempted to raise when constructing artificial AST nodes, but I'm not sure I'm not sure whether it could break code. Maybe people are using fake AST nodes with bad location values just for their own needs, like for a sentinel value. OTOH, checking the consistency of all AST nodes in
compilemay seem an overkill. I don't have enough insight on that part of the codebase to know how to avoid that assertion.@JelleZijlstra any recommendation here?
Reacted by Peter BiermaIs this causing a problem in practice, or just when fuzzing?
Is this causing a problem in practice, or just when fuzzing?
In practice, it's causing an assertion failure and a crash. So yes, it could be a problem in practice for static analysis tools that may perhaps incorrectly determine some values.
In addition, it may cause issues since without the assertion, the location data would be incorrect and we could end up with incorrect bytecode.
Or maybe convert
assertto a regular exception?if (column < -1 || end_column < -1) { const char *msg = column < -1 ? "column" : "end_column"; int value = column < -1 ? column : end_column; PyErr_Format(PyExc_ValueError, "invalid %s value: %d", msg, value); return ERROR; }
Will produce:
» ./python.exe Python 3.14.0a5+ (heads/main-dirty:8f11af45de6, Mar 3 2025, 14:19:56) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> 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=[]) ... ... compile(tree, "<string>", "exec") ... Traceback (most recent call last): File "<python-input-0>", line 7, in <module> compile(tree, "<string>", "exec") ~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^ ValueError: invalid column value: -2
Yes, but my question was: should we do it when we construct the AST nodes or should we do it in assemble.c?
In my opinion, it is better to do in
assemble.c, but I am not a big expert in this module.I can think of some advantages of doing in either module:
- in AST: traceback may be nicer, and users usually craft AST nodes rather than bytecode. Probably where errors would arise the most (like, wrongly crafting an AST node with a negative column value is something I can totally imagine)
- in assemble: we only need one check at that place. It could prevent footguns from other places I'm not aware of and that can hit that line (I don't remember whether it's possible to craft a wrong bytecode explicitly and assemble it without passing by a pure python module or if requires the C API for that)
I think it's better to do it in the assembler. There are other things you can do with ASTs than compiling, and for those it may make sense to use invalid column values. For example, if you're constructing an AST and then stringifying it with something like
ast.unparse(for example, for a lint autofix), you may want to use -1 as a column value because you don't have a real one.Reacted by Bénédikt Trancolumn = -1seems valid according to both the assertion and the way we internally represent such locations, but I think it would be good to explicitly rejectcolumn = -2. I don't know if I have time for working on this one so anyone else can pick it up before I assign the issue to myself (for future contributors, just drop a comment say that you're working on it)I now fully sure that adding this to
assemble.cis better, because we don't validate any other values inast, for example, importing an empty module name:>>> import ast ... ... tree = ast.Module(body=[ ... ast.Import(names=[ast.alias(name='', lineno=0, col_offset=0)], lineno=0, col_offset=-1) ... ], type_ignores=[]) ... >>> eval(compile(tree, "<string>", "exec")) Traceback (most recent call last): File "<python-input-10>", line 1, in <module> eval(compile(tree, "<string>", "exec")) ~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "<string>", line 0, in <module> ValueError: Empty module name
I have a PR with the change above.
Reacted by Bénédikt TranSorry, I was not really correct with my phrase:
because we don't validate any other values in
astWe do validate some values, for example, locations:
Lines 29 to 48 in b3c18bf
So, it might be a good idea to fix this macro. It does not make any sense to use
-2as the value for any locations.- added a commit that references this issue
on Mar 3, 2025
Crash report
What happened?
Bug Description
This is a bug that only affects DEBUG builds.
The reproducer is as follow:
This code fails with:
backtrace
CPython versions tested on:
CPython main branch
Operating systems tested on:
Linux
Output from running 'python -VV' on the command line:
Python 3.14.0a4+ (heads/main-dirty:75f59bb6293, Jan 23 2025, 22:33:08) [GCC 13.3.0]
Linked PRs
ast#130795ast(GH-130795) #132243ast(GH-130795) #132260