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

Segmentation fault after deleting __fields__ attribute from ast.AST #126105

Closed
federicovalenso opened this issue Oct 29, 2024 · 5 comments
Closed
Assignees
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@federicovalenso
Copy link
Contributor

federicovalenso commented Oct 29, 2024

Crash report

What happened?

>>> import ast
>>> dir(ast.AST)
['__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__le__', '__lt__', '__match_args__', '__module__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '_attributes', '_fields']
>>> del ast.AST._fields
>>> dir(ast.AST)
['__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__le__', '__lt__', '__match_args__', '__module__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '_attributes']
>>> t = ast.AST(arg1=123)
Segmentation fault

This happens because fields variable is null-pointer here, moreover it was previously checked for null here.

CPython versions tested on:

3.11

Operating systems tested on:

Linux

Output from running 'python -VV' on the command line:

Python 3.11.9 (main, Oct 28 2024, 23:26:29) [GCC 13.2.0]

Linked PRs

@federicovalenso federicovalenso added the type-crash A hard crash of the interpreter, possibly with a core dump label Oct 29, 2024
@picnixz picnixz added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes labels Oct 29, 2024
@picnixz
Copy link
Contributor

picnixz commented Oct 29, 2024

cc @Eclips4 @JelleZijlstra (Would adding Py_TPFLAGS_IMMUTABLETYPE work or is it too restrictive?)

@sobolevn
Copy link
Member

I can confirm, that this still happens on main:

Python 3.14.0a1+ (heads/main:19e93e2e269, Oct 28 2024, 11:42:01) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import ast
>>> del ast.AST._fields
>>> t = ast.AST(arg1=123)
[1]    85885 segmentation fault  ./python.exe
                          

Py_TPFLAGS_IMMUTABLETYPE

I think that this is an overkill and it change the semantics here. We can just modify the code and check for NULL explicitly.

See

ast_type_init(PyObject *self, PyObject *args, PyObject *kw)

@sobolevn sobolevn self-assigned this Oct 29, 2024
@picnixz
Copy link
Contributor

picnixz commented Oct 29, 2024

Oh yes, we only need to modify:

    if (PyObject_GetOptionalAttr((PyObject*)Py_TYPE(self), state->_fields, &fields) < 0) {
        goto cleanup;
    }

with a NULL check beforehand I think. What do you think?

@Eclips4
Copy link
Member

Eclips4 commented Oct 29, 2024

cc @Eclips4 @JelleZijlstra (Would adding Py_TPFLAGS_IMMUTABLETYPE work or is it too restrictive?)

If you are talking about making nodes immutable, we can't do that because a lot of code relies on the fact that nodes are mutable. So it would be a huge breaking change.

Eclips4 pushed a commit that referenced this issue Oct 29, 2024
…6115)

Previously, if the `ast.AST._fields` attribute was deleted, attempts to create a new `as`t node would crash due to the assumption that `_fields` always had a non-NULL value. Now it has been fixed by adding an extra check to ensure that `_fields` does not have a NULL value (this can happen when you manually remove `_fields` attribute).
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 29, 2024
pythonGH-126115)

Previously, if the `ast.AST._fields` attribute was deleted, attempts to create a new `as`t node would crash due to the assumption that `_fields` always had a non-NULL value. Now it has been fixed by adding an extra check to ensure that `_fields` does not have a NULL value (this can happen when you manually remove `_fields` attribute).
(cherry picked from commit b2eaa75)

Co-authored-by: sobolevn <mail@sobolevn.me>
Eclips4 pushed a commit to Eclips4/cpython that referenced this issue Oct 29, 2024
… deleted (pythonGH-126115)

Previously, if the `ast.AST._fields` attribute was deleted, attempts to create a new `as`t node would crash due to the assumption that `_fields` always had a non-NULL value. Now it has been fixed by adding an extra check to ensure that `_fields` does not have a NULL value (this can happen when you manually remove `_fields` attribute).
(cherry picked from commit b2eaa75)

Co-authored-by: sobolevn <mail@sobolevn.me>
Eclips4 pushed a commit that referenced this issue Oct 29, 2024
…ed (GH-126115) (#126130)

gh-126105: Fix crash in `ast` module, when `._fields` is deleted (GH-126115)

Previously, if the `ast.AST._fields` attribute was deleted, attempts to create a new `as`t node would crash due to the assumption that `_fields` always had a non-NULL value. Now it has been fixed by adding an extra check to ensure that `_fields` does not have a NULL value (this can happen when you manually remove `_fields` attribute).
(cherry picked from commit b2eaa75)

Co-authored-by: sobolevn <mail@sobolevn.me>
Eclips4 added a commit that referenced this issue Oct 29, 2024
#126132)

[3.12] gh-126105: Fix crash in `ast` module, when `._fields` is deleted (GH-126115)

Previously, if the `ast.AST._fields` attribute was deleted, attempts to create a new `as`t node would crash due to the assumption that `_fields` always had a non-NULL value. Now it has been fixed by adding an extra check to ensure that `_fields` does not have a NULL value (this can happen when you manually remove `_fields` attribute).
(cherry picked from commit b2eaa75)

Co-authored-by: sobolevn <mail@sobolevn.me>
@Eclips4
Copy link
Member

Eclips4 commented Oct 29, 2024

Thank you Valery for the report and Nikita for the fix.

@Eclips4 Eclips4 closed this as completed Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Development

No branches or pull requests

4 participants