[issue11105] Compiling evil ast crashes interpreter

[miss-islington]

miss-islington  added the comment:


New changeset 976598d36bd180024c5f0edf1f7ec0f0b436380f by Miss Islington (bot) 
in branch '3.10':
bpo-11105: Do not crash when compiling recursive ASTs (GH-20594)
https://github.com/python/cpython/commit/976598d36bd180024c5f0edf1f7ec0f0b436380f


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Pablo Galindo Salgado]

Pablo Galindo Salgado  added the comment:


New changeset f3491242e41933aa9529add7102edb68b80a25e9 by Batuhan Taskaya in 
branch 'main':
bpo-11105: Do not crash when compiling recursive ASTs (GH-20594)
https://github.com/python/cpython/commit/f3491242e41933aa9529add7102edb68b80a25e9


--
nosy: +pablogsal

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[miss-islington]

Change by miss-islington :


--
nosy: +miss-islington
nosy_count: 9.0 -> 10.0
pull_requests: +25115
pull_request: https://github.com/python/cpython/pull/26521

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Georg Brandl]

Change by Georg Brandl :


--
nosy:  -georg.brandl

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Batuhan Taskaya]

Batuhan Taskaya  added the comment:

> With 3.9 on Windows, using Benjamin's example, I do not get the Windows 
> equivalent of a seg fault.  However, execution stops at compile with no 
> exception, including SystemExit.

I can still reproduce on Linux,

 $ python
Python 3.10.0a0 (heads/bpo-x:f2947e354c, May 21 2020, 18:54:57) 
[GCC 9.2.1 20191008] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import ast
>>> e = ast.UnaryOp(op=ast.Not(), lineno=0, col_offset=0)
>>> e.operand = e
>>> compile(ast.Expression(e), "", "eval")
[1]15320 segmentation fault (core dumped)  python


> These examples amount to limited fuzz testing of compile().  I think it 
> should raise something like "SyntaxError: recursive ast" or even 'bad ast' if 
> malformed non-recursive asts have the same issue.

I dont think it would be easy to locate such errors before they happen, instead 
I propose (actually already proposed in PR 20594) to add recursion guards to 
places where this might happen. This can prevent crashes on both direct and 
indirect cycles

>>> import ast
>>> e = ast.UnaryOp(op=ast.Not(), lineno=0, col_offset=0)
>>> e.operand = e
>>> compile(ast.Expression(e), "", "eval")
Traceback (most recent call last):
  File "", line 1, in 
RecursionError: maximum recursion depth exceeded while traversing 'expr' node
>>> e = ast.UnaryOp(op=ast.Not(), lineno=0, col_offset=0)
>>> f = ast.UnaryOp(op=ast.Not(), lineno=0, col_offset=0)
>>> e.operand = f
>>> f.operand = e
>>> compile(ast.Expression(e), "", "eval")
Traceback (most recent call last):
  File "", line 1, in 
RecursionError: maximum recursion depth exceeded while traversing 'expr' node

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Terry J. Reedy]

Terry J. Reedy  added the comment:

With 3.9 on Windows, using Benjamin's example, I do not get the Windows 
equivalent of a seg fault.  However, execution stops at compile with no 
exception, including SystemExit.

These examples amount to limited fuzz testing of compile().  I think it should 
raise something like "SyntaxError: recursive ast" or even 'bad ast' if 
malformed non-recursive asts have the same issue.

--
nosy: +terry.reedy
versions: +Python 3.10 -Python 2.7, Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Batuhan Taskaya]

Change by Batuhan Taskaya :


--
keywords: +patch
pull_requests: +19824
stage: test needed -> patch review
pull_request: https://github.com/python/cpython/pull/20594

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[ppperry]

ppperry  added the comment:

What about indirect cycles like below:

>>> e = ast.UnaryOp(op=ast.Not(), lineno=0, col_offset=0)
>>> f = ast.UnaryOp(op=ast.Not(), lineno=0, col_offset=0)
>>> e.operand = f
>>> f.operand = e
>>> compile(ast.Expression(e), "", "eval")

(I tested, this also crashes)

--
nosy: +ppperry

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Batuhan]

Batuhan  added the comment:

We can probably implement something like this to prevent this happening
diff --git a/Parser/asdl_c.py b/Parser/asdl_c.py
index daac0966f5..f9da52da7f 100755
--- a/Parser/asdl_c.py
+++ b/Parser/asdl_c.py
@@ -559,6 +559,11 @@ class Obj2ModVisitor(PickleVisitor):
 self.emit("asdl_seq_SET(%s, i, val);" % field.name, depth+2)
 self.emit("}", depth+1)
 else:
+self.emit("if (obj == tmp) {", depth+1)
+self.emit("PyErr_SetString(PyExc_RuntimeError, \"Recursing fields "
+  "are not supported for AST nodes.\");", depth+2, 
reflow=False)
+self.emit("goto failed;", depth+2)
+self.emit("}", depth+1)
 self.emit("res = obj2ast_%s(tmp, &%s, arena);" %
   (field.type, field.name), depth+1)
 self.emit("if (res != 0) goto failed;", depth+1)

--
nosy: +BTaskaya

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Batuhan]

Change by Batuhan :


--
versions: +Python 3.9 -Python 3.2, Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Eric Snow]

Changes by Eric Snow :


--
nosy: +eric.snow

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Benjamin Peterson]

Benjamin Peterson  added the comment:

Have him try on 3.3. This should be less of an issue there where there is an 
AST validator. It doesn't fix this bug, but it does fix most accidental AST 
construction bugs.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Gregory P. Smith]

Gregory P. Smith  added the comment:

i haven't confirmed if it is this exact bug but I believe a coworker just ran 
into something similar.  he wrote code to use the ast to remove docstrings from 
code before passing it to compile() (as that saves a noticable amount of 
memory).  in the case the ast for code like:

def foo():
  """this is a docstring."""

Removing the docstring and passing such a thing to compile triggers a problem.  
A workaround was to add a pass in such cases:

if (node.body and isinstance(node.body[0], ast.Expr) and
isinstance(node.body[0].value, ast.Str)):
  docstring = node.body.pop(0)
  if len(node.body) == 0:
# An empty body will sometimes provoke a segfault when you call
# compile on the code object.
node.body.append(ast.Pass(lineno=docstring.lineno,
  col_offset=docstring.col_offset))

regardless, it'd be better if compile() *never* crashed on strange input.

--
nosy: +gregory.p.smith

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Ezio Melotti]

Changes by Ezio Melotti :


--
components: +None
stage:  -> test needed
versions:  -Python 3.1

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Meador Inge]

Changes by Meador Inge :


--
nosy: +meador.inge

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Georg Brandl]

Georg Brandl  added the comment:

Alex: If the node attributes were not mutable, it would be extremely awkward, 
not to say inefficient, to mutate an already existing AST as returned by 
ast.parse().

The AST objects in the _ast module aren't what Python works with internally, 
anyway. When calling ast.parse(), the AST is converted to Python objects (these 
are defined in Python-ast.c), and compile()ing such an object converts them 
back to the internal tree representation.  This conversion is where recursions 
would need to be handled.

--
nosy: +georg.brandl

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Benjamin Peterson]

Benjamin Peterson  added the comment:

2011/2/3 Alexander Belopolsky :
>
> Alexander Belopolsky  added the comment:
>
> On Thu, Feb 3, 2011 at 12:08 PM, Benjamin Peterson
>  wrote:
> ..
>>> I wonder: Why ast nodes need to be mutable?
>>
>> So people can change them.
>
> Well, they are hashable, so this needs to be done carefully.  Is this
> necessary for AST-based optimizations?  Does Python actually change
> AST after it has been created?  Note that for some optimizations it
> may be more appropriate to build a new tree rather than mutate the old
> one.  Depending on the algorithm, you may or may not need to change
> the nodes after they have been created in the process.

Other people are, though. The hash is by identity anyway.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Alexander Belopolsky]

Alexander Belopolsky  added the comment:

On Thu, Feb 3, 2011 at 12:08 PM, Benjamin Peterson
 wrote:
..
>> I wonder: Why ast nodes need to be mutable?
>
> So people can change them.

Well, they are hashable, so this needs to be done carefully.  Is this
necessary for AST-based optimizations?  Does Python actually change
AST after it has been created?  Note that for some optimizations it
may be more appropriate to build a new tree rather than mutate the old
one.  Depending on the algorithm, you may or may not need to change
the nodes after they have been created in the process.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Benjamin Peterson]

Benjamin Peterson  added the comment:

2011/2/3 Alexander Belopolsky :
>
> Alexander Belopolsky  added the comment:
>
> Looks like a stack overflow caused by an infinite recursion.  I am not sure 
> if it is possible to add cycle detection code without sacrificing performance 
> or setting some arbitrary limits.

Yes, it's definitely low priority. It's probably easier to crash the
interpreter by producing differently malformed ast anyway.

>
> I wonder: Why ast nodes need to be mutable?

So people can change them.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Alexander Belopolsky]

Alexander Belopolsky  added the comment:

Looks like a stack overflow caused by an infinite recursion.  I am not sure if 
it is possible to add cycle detection code without sacrificing performance or 
setting some arbitrary limits.

I wonder: Why ast nodes need to be mutable?

--
nosy: +belopolsky

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11105] Compiling evil ast crashes interpreter

[Benjamin Peterson]

New submission from Benjamin Peterson :

You don't want to know why I was thinking about this...

$ ./python 
Python 3.2rc2+ (py3k:88302, Feb  1 2011, 19:02:10) 
[GCC 4.4.4] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import ast
>>> e = ast.UnaryOp(op=ast.Not(), lineno=0, col_offset=0)
>>> e.operand = e
>>> compile(ast.Expression(e), "", "eval")
Segmentation fault

--
messages: 127783
nosy: benjamin.peterson
priority: low
severity: normal
status: open
title: Compiling evil ast crashes interpreter
type: crash
versions: Python 2.7, Python 3.1, Python 3.2, Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com