diff options
| -rw-r--r-- | Lib/test/test_xml_etree_c.py | 32 | ||||
| -rw-r--r-- | Misc/NEWS.d/next/Library/2017-10-08-23-28-30.bpo-31728.XrVMME.rst | 2 | ||||
| -rw-r--r-- | Modules/_elementtree.c | 62 |
3 files changed, 62 insertions, 34 deletions
diff --git a/Lib/test/test_xml_etree_c.py b/Lib/test/test_xml_etree_c.py index b2200d3..d99b4e7 100644 --- a/Lib/test/test_xml_etree_c.py +++ b/Lib/test/test_xml_etree_c.py @@ -84,6 +84,38 @@ class MiscTests(unittest.TestCase): # and so destroy the parser support.gc_collect() + def test_bpo_31728(self): + # A crash or an assertion failure shouldn't happen, in case garbage + # collection triggers a call to clear() or a reading of text or tail, + # while a setter or clear() or __setstate__() is already running. + elem = cET.Element('elem') + class X: + def __del__(self): + elem.text + elem.tail + elem.clear() + + elem.text = X() + elem.clear() # shouldn't crash + + elem.tail = X() + elem.clear() # shouldn't crash + + elem.text = X() + elem.text = X() # shouldn't crash + elem.clear() + + elem.tail = X() + elem.tail = X() # shouldn't crash + elem.clear() + + elem.text = X() + elem.__setstate__({'tag': 42}) # shouldn't cause an assertion failure + elem.clear() + + elem.tail = X() + elem.__setstate__({'tag': 42}) # shouldn't cause an assertion failure + @unittest.skipUnless(cET, 'requires _elementtree') class TestAliasWorking(unittest.TestCase): diff --git a/Misc/NEWS.d/next/Library/2017-10-08-23-28-30.bpo-31728.XrVMME.rst b/Misc/NEWS.d/next/Library/2017-10-08-23-28-30.bpo-31728.XrVMME.rst new file mode 100644 index 0000000..b317d9f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-10-08-23-28-30.bpo-31728.XrVMME.rst @@ -0,0 +1,2 @@ +Prevent crashes in `_elementtree` due to unsafe cleanup of `Element.text` +and `Element.tail`. Patch by Oren Milman. diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index a6d6c61..707ab29 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -61,15 +61,22 @@ do { memory -= size; printf("%8d - %s\n", memory, comment); } while (0) #define JOIN_SET(p, flag) ((void*) ((uintptr_t) (JOIN_OBJ(p)) | (flag))) #define JOIN_OBJ(p) ((PyObject*) ((uintptr_t) (p) & ~(uintptr_t)1)) +/* Py_SETREF for a PyObject* that uses a join flag. */ +Py_LOCAL_INLINE(void) +_set_joined_ptr(PyObject **p, PyObject *new_joined_ptr) +{ + PyObject *tmp = JOIN_OBJ(*p); + *p = new_joined_ptr; + Py_DECREF(tmp); +} + /* Py_CLEAR for a PyObject* that uses a join flag. Pass the pointer by * reference since this function sets it to NULL. */ static void _clear_joined_ptr(PyObject **p) { if (*p) { - PyObject *tmp = JOIN_OBJ(*p); - *p = NULL; - Py_DECREF(tmp); + _set_joined_ptr(p, NULL); } } @@ -356,7 +363,6 @@ static int element_init(PyObject *self, PyObject *args, PyObject *kwds) { PyObject *tag; - PyObject *tmp; PyObject *attrib = NULL; ElementObject *self_elem; @@ -397,15 +403,11 @@ element_init(PyObject *self, PyObject *args, PyObject *kwds) Py_INCREF(tag); Py_XSETREF(self_elem->tag, tag); - tmp = self_elem->text; Py_INCREF(Py_None); - self_elem->text = Py_None; - Py_DECREF(JOIN_OBJ(tmp)); + _set_joined_ptr(&self_elem->text, Py_None); - tmp = self_elem->tail; Py_INCREF(Py_None); - self_elem->tail = Py_None; - Py_DECREF(JOIN_OBJ(tmp)); + _set_joined_ptr(&self_elem->tail, Py_None); return 0; } @@ -675,12 +677,10 @@ _elementtree_Element_clear_impl(ElementObject *self) dealloc_extra(self); Py_INCREF(Py_None); - Py_DECREF(JOIN_OBJ(self->text)); - self->text = Py_None; + _set_joined_ptr(&self->text, Py_None); Py_INCREF(Py_None); - Py_DECREF(JOIN_OBJ(self->tail)); - self->tail = Py_None; + _set_joined_ptr(&self->tail, Py_None); Py_RETURN_NONE; } @@ -702,13 +702,11 @@ _elementtree_Element___copy___impl(ElementObject *self) if (!element) return NULL; - Py_DECREF(JOIN_OBJ(element->text)); - element->text = self->text; - Py_INCREF(JOIN_OBJ(element->text)); + Py_INCREF(JOIN_OBJ(self->text)); + _set_joined_ptr(&element->text, self->text); - Py_DECREF(JOIN_OBJ(element->tail)); - element->tail = self->tail; - Py_INCREF(JOIN_OBJ(element->tail)); + Py_INCREF(JOIN_OBJ(self->tail)); + _set_joined_ptr(&element->tail, self->tail); if (self->extra) { if (element_resize(element, self->extra->length) < 0) { @@ -776,14 +774,12 @@ _elementtree_Element___deepcopy__(ElementObject *self, PyObject *memo) text = deepcopy(JOIN_OBJ(self->text), memo); if (!text) goto error; - Py_DECREF(element->text); - element->text = JOIN_SET(text, JOIN_GET(self->text)); + _set_joined_ptr(&element->text, JOIN_SET(text, JOIN_GET(self->text))); tail = deepcopy(JOIN_OBJ(self->tail), memo); if (!tail) goto error; - Py_DECREF(element->tail); - element->tail = JOIN_SET(tail, JOIN_GET(self->tail)); + _set_joined_ptr(&element->tail, JOIN_SET(tail, JOIN_GET(self->tail))); if (self->extra) { if (element_resize(element, self->extra->length) < 0) @@ -967,13 +963,13 @@ element_setstate_from_attributes(ElementObject *self, Py_INCREF(tag); Py_XSETREF(self->tag, tag); - _clear_joined_ptr(&self->text); - self->text = text ? JOIN_SET(text, PyList_CheckExact(text)) : Py_None; - Py_INCREF(JOIN_OBJ(self->text)); + text = text ? JOIN_SET(text, PyList_CheckExact(text)) : Py_None; + Py_INCREF(JOIN_OBJ(text)); + _set_joined_ptr(&self->text, text); - _clear_joined_ptr(&self->tail); - self->tail = tail ? JOIN_SET(tail, PyList_CheckExact(tail)) : Py_None; - Py_INCREF(JOIN_OBJ(self->tail)); + tail = tail ? JOIN_SET(tail, PyList_CheckExact(tail)) : Py_None; + Py_INCREF(JOIN_OBJ(tail)); + _set_joined_ptr(&self->tail, tail); /* Handle ATTRIB and CHILDREN. */ if (!children && !attrib) @@ -1980,8 +1976,7 @@ element_text_setter(ElementObject *self, PyObject *value, void *closure) { _VALIDATE_ATTR_VALUE(value); Py_INCREF(value); - Py_DECREF(JOIN_OBJ(self->text)); - self->text = value; + _set_joined_ptr(&self->text, value); return 0; } @@ -1990,8 +1985,7 @@ element_tail_setter(ElementObject *self, PyObject *value, void *closure) { _VALIDATE_ATTR_VALUE(value); Py_INCREF(value); - Py_DECREF(JOIN_OBJ(self->tail)); - self->tail = value; + _set_joined_ptr(&self->tail, value); return 0; } |