From: Oren Milman Date: Tue, 10 Oct 2017 20:26:24 +0000 (+0300) Subject: bpo-31728: Prevent crashes in _elementtree due to unsafe cleanup of Element.text... X-Git-Tag: v3.7.0a2~30 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=39ecb9c71b6e55d8a61a710d0144231bd88f9ada;p=python bpo-31728: Prevent crashes in _elementtree due to unsafe cleanup of Element.text and Element.tail (#3924) --- diff --git a/Lib/test/test_xml_etree_c.py b/Lib/test/test_xml_etree_c.py index 25517a7269..765652ff75 100644 --- a/Lib/test/test_xml_etree_c.py +++ b/Lib/test/test_xml_etree_c.py @@ -85,6 +85,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 0000000000..b317d9f210 --- /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 bddac851d9..b4c0f4c87a 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___impl(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) @@ -968,13 +964,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) @@ -2009,8 +2005,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; } @@ -2019,8 +2014,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; }