Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-126105: Fix crash inast module, when._fields is deleted#126115
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm wondering why we have:
if (PyObject_GetOptionalAttr((PyObject*)Py_TYPE(self),state->_fields,&fields)<0) {
This could be because some AST nodes do not have a_fields attribute but then, why don't we simply change it to an eagerPyObject_GetAttr?
Misc/NEWS.d/next/Library/2024-10-29-11-45-44.gh-issue-126105.cOL-R6.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Docs give me more confidence:https://docs.python.org/3/library/ast.html#ast.AST._fields
|
I don't think that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. Thanks!
Uh oh!
There was an error while loading.Please reload this page.
| Py_ssize_t i, numfields = 0; | ||
| int res = -1; | ||
| PyObject *key, *value, *fields, *attributes = NULL, *remaining_fields = NULL; | ||
| if (PyObject_GetOptionalAttr((PyObject*)Py_TYPE(self), state->_fields, &fields) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A simpler fix would have been this, right?
if (PyObject_GetOptionalAttr((PyObject*)Py_TYPE(self), state->_fields, &fields) <= 0) {However, your fix is better.
b2eaa75 intopython:mainUh oh!
There was an error while loading.Please reload this page.
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 commitb2eaa75)Co-authored-by: sobolevn <mail@sobolevn.me>
Sorry,@sobolevn and@Eclips4, I could not cleanly backport this to |
GH-126130 is a backport of this pull request to the3.13 branch. |
Eclips4 commentedOct 29, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'll take care of backport to 3.12. |
… 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 commitb2eaa75)Co-authored-by: sobolevn <mail@sobolevn.me>
GH-126132 is a backport of this pull request to the3.12 branch. |
…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 commitb2eaa75)Co-authored-by: sobolevn <mail@sobolevn.me>
@Eclips4 thank you for your help! |
#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 commitb2eaa75)Co-authored-by: sobolevn <mail@sobolevn.me>
python#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).
python#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).
Uh oh!
There was an error while loading.Please reload this page.
This PR changes the logic a bit, but it looks like a correct thing to me.
Previously we were handling
NULLoffieldscase forremaining_fieldscpython/Python/Python-ast.c
Lines 5089 to 5098 in9b14083
But, we didn't really handle the
fieldsitself.Further calls assume that
fieldscannot beNULL.The other way of fixing this is to add a default for
fieldslike:In this case
ast.AST(arg=1)with no_fieldswill produce:It does not seem correct to me, because
_fieldsis a part of our API. SinceASTobjects are mostly internal, there are no real cases of missing_fields.