-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-31758: Prevent reference leaks in __setstate__() and __init__() of _elementtree.Element #3956
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
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.
Note that element_resize()
allocates extra nchildren
items. If self->extra->length
!= 0, self->extra->allocated
> nchildren
.
Also note that if children
and attrib
are empty, the element keeps old children and attributes instead of clearing them.
The code at line 941 looks suspicious to me. It looks like a double decref.
|
||
self->handle_start = PyObject_GetAttrString(target, "start"); | ||
Py_XSETREF(self->handle_start, PyObject_GetAttrString(target, "start")); |
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.
Perhaps PyObject_GetAttrString()
, Py_XSETREF()
and ignore_attribute_error()
can be combined in one function.
Modules/_elementtree.c
Outdated
@@ -994,6 +994,10 @@ element_setstate_from_attributes(ElementObject *self, | |||
} | |||
assert(self->extra && self->extra->allocated >= nchildren); | |||
|
|||
/* Decref each of the old children. */ | |||
for (i = 0; i < self->extra->length; i++) { | |||
Py_DECREF(self->extra->children[i]); |
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.
Py_DECREF()
can cause executing arbitrary Python code. This can change the size of the children
list. PyList_GET_ITEM()
in the following loop can crash.
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.
Ah, indeed.
Then ISTM that we should try to write a helper function that would be used both here and by element_ass_subscr()
.
what do you think?
What is the context of these comments? I guess i missed something that should be obvious after reading them..
I agree. ISTM that these
On my pc, this patch seems to work fine. |
Lib/test/test_xml_etree_c.py
Outdated
@@ -117,6 +118,18 @@ def __del__(self): | |||
elem.tail = X() | |||
elem.__setstate__({'tag': 42}) # shouldn't cause an assertion failure | |||
|
|||
def test_refleaks_in_Element___setstate__(self): |
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.
Maybe add @refcount_test
?
I removed the " needs backport to 3.6" label, the 3.6 branch no longer accept bugfixes (only security fixes are accepted): https://devguide.python.org/#status-of-python-branches |
I resolved the conflicts, but Serhiy's comments still apply. |
Sorry, but no chance for that currently.. (though I am sure you could do a better job than me) |
It looks like this issue was resolved with #3997. |
In addition, add a test to
test_xml_etree_c
, to make sure that the refleaks in__setstate__()
are no more.https://bugs.python.org/issue31758