| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| Fixes race conditions in :mod:`xml.etree.ElementTree` where the ``extra`` | ||
| pointer dereference and ``attrib`` access was unsynchronized. This prevents | ||
| potential crashes (core dumps) in the free-threading build caused by | ||
| concurrent modification of an element's internal structure. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| Expand Up | @@ -276,13 +276,14 @@ create_extra(ElementObject* self, PyObject* attrib) | |
| PyErr_NoMemory(); | ||
| return -1; | ||
| } | ||
|
|
||
| Py_BEGIN_CRITICAL_SECTION(self); | ||
| self->extra->attrib = Py_XNewRef(attrib); | ||
|
|
||
| self->extra->length = 0; | ||
| self->extra->allocated = STATIC_CHILDREN; | ||
| self->extra->children = self->extra->_children; | ||
|
|
||
| Py_END_CRITICAL_SECTION(); | ||
| return 0; | ||
| } | ||
|
|
||
| Expand All | @@ -297,7 +298,7 @@ dealloc_extra(ElementObjectExtra *extra) | |
| Py_XDECREF(extra->attrib); | ||
|
|
||
| for (i = 0; i < extra->length; i++) | ||
| Py_DECREF(extra->children[i]); | ||
| Py_XDECREF(extra->children[i]); | ||
|
|
||
| if (extra->children != extra->_children) { | ||
| PyMem_Free(extra->children); | ||
| Expand All | @@ -311,15 +312,16 @@ clear_extra(ElementObject* self) | |
| { | ||
| ElementObjectExtra *myextra; | ||
|
|
||
| if (!self->extra) | ||
| return; | ||
| Py_BEGIN_CRITICAL_SECTION(self); | ||
|
Comment thread
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityDo not add locking here. The only code path that may call this code concurrently is in element_setstate_from_attributes, and locking should be moved there. It is probably needed anyway by the looks of it.
Sorry, something went wrong.
All reactions
|
||
|
|
||
| /* Avoid DECREFs calling into this code again (cycles, etc.) | ||
| */ | ||
| myextra = self->extra; | ||
| self->extra = NULL; | ||
|
|
||
| dealloc_extra(myextra); | ||
| Py_END_CRITICAL_SECTION(); | ||
| if (myextra) { | ||
| dealloc_extra(myextra); | ||
| } | ||
| } | ||
|
|
||
| /* Convenience internal function to create new Element objects with the given | ||
| Expand Down Expand Up | @@ -544,29 +546,34 @@ element_add_subelement(elementtreestate *st, ElementObject *self, | |
| return -1; | ||
| } | ||
|
|
||
| Py_BEGIN_CRITICAL_SECTION(self); | ||
| if (element_resize(self, 1) < 0) | ||
| return -1; | ||
|
|
||
| self->extra->children[self->extra->length] = Py_NewRef(element); | ||
|
|
||
| self->extra->length++; | ||
|
|
||
| Py_END_CRITICAL_SECTION(); | ||
| return 0; | ||
| } | ||
|
|
||
| LOCAL(PyObject*) | ||
| element_get_attrib(ElementObject* self) | ||
| { | ||
| /* return borrowed reference to attrib dictionary */ | ||
| /* return new reference to attrib dictionary */ | ||
| /* note: this function assumes that the extra section exists */ | ||
|
|
||
| PyObject* res = self->extra->attrib; | ||
| PyObject *res; | ||
| Py_BEGIN_CRITICAL_SECTION(self); | ||
| res = self->extra->attrib; | ||
|
|
||
| if (!res) { | ||
| /* create missing dictionary */ | ||
| res = self->extra->attrib = PyDict_New(); | ||
| } | ||
|
|
||
| Py_XINCREF(res); | ||
| Py_END_CRITICAL_SECTION(); | ||
| return res; | ||
| } | ||
|
|
||
| Expand Down Expand Up | @@ -667,13 +674,15 @@ element_gc_traverse(PyObject *op, visitproc visit, void *arg) | |
| Py_VISIT(JOIN_OBJ(self->text)); | ||
| Py_VISIT(JOIN_OBJ(self->tail)); | ||
|
|
||
| Py_BEGIN_CRITICAL_SECTION(self); | ||
|
Comment thread
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThe GC runs during a stop-the-world pause, so there's no need for locking here.
Sorry, something went wrong.
All reactions
|
||
| if (self->extra) { | ||
| Py_ssize_t i; | ||
| Py_VISIT(self->extra->attrib); | ||
|
|
||
| for (i = 0; i < self->extra->length; ++i) | ||
| Py_VISIT(self->extra->children[i]); | ||
| } | ||
| Py_END_CRITICAL_SECTION(); | ||
| return 0; | ||
| } | ||
|
|
||
| Expand Down Expand Up | @@ -1625,10 +1634,17 @@ static Py_ssize_t | |
| element_length(PyObject *op) | ||
| { | ||
| ElementObject *self = _Element_CAST(op); | ||
| if (!self->extra) | ||
| return 0; | ||
| Py_ssize_t res; | ||
| Py_BEGIN_CRITICAL_SECTION(self); | ||
| if (!self->extra) { | ||
| res = 0; | ||
| } | ||
| else { | ||
| res = self->extra->length; | ||
| } | ||
| Py_END_CRITICAL_SECTION(); | ||
|
|
||
| return self->extra->length; | ||
| return res; | ||
| } | ||
|
|
||
| /*[clinic input] | ||
| Expand Down Expand Up | @@ -1766,9 +1782,12 @@ _elementtree_Element_set_impl(ElementObject *self, PyObject *key, | |
| if (!attrib) | ||
| return NULL; | ||
|
|
||
| if (PyDict_SetItem(attrib, key, value) < 0) | ||
| if (PyDict_SetItem(attrib, key, value) < 0) { | ||
| Py_DECREF(attrib); | ||
| return NULL; | ||
| } | ||
|
|
||
| Py_DECREF(attrib); | ||
| Py_RETURN_NONE; | ||
| } | ||
|
|
||
| Expand Down Expand Up | @@ -2079,12 +2098,15 @@ element_attrib_getter(PyObject *op, void *closure) | |
| { | ||
| PyObject *res; | ||
| ElementObject *self = _Element_CAST(op); | ||
| if (!self->extra) { | ||
| if (create_extra(self, NULL) < 0) | ||
| return NULL; | ||
| Py_BEGIN_CRITICAL_SECTION(self); | ||
| if (!self->extra && create_extra(self, NULL) < 0){ | ||
| res = NULL; | ||
| } | ||
| res = element_get_attrib(self); | ||
| return Py_XNewRef(res); | ||
| else { | ||
| res = element_get_attrib(self); | ||
| } | ||
| Py_END_CRITICAL_SECTION(); | ||
| return res; | ||
| } | ||
|
|
||
| /* macro for setter validation */ | ||
| Expand Down | ||
| Back | FazBrowse Home | New Git URL |
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityDon't do locking here. There are a few callers of this function, and locking should be moved to the callers. For instance, this code in element_resize would not be thread safe with an inner critical section:
Allocations and init functions need not be thread safe.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.