[2.7] bpo-31728: Prevent crashes in _elementtree due to unsafe cleanup of Element.text and Element.tail (GH-3924)

Lib/test/test_xml_etree_c.py

@@ -53,6 +53,31 @@ def test_del_attribute(self):
             del element.attrib
         self.assertEqual(element.attrib, {'A': 'B', 'C': 'D'})
 
+    def test_bpo_31728(self):
+        # A crash shouldn't happen in case garbage collection triggers a call
+        # to clear() or a reading of text or tail, while a setter or clear()
+        # 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()
+
 
 def test_main():
     from test import test_xml_etree, test_xml_etree_c

Misc/NEWS.d/next/Library/2017-10-11-13-05-19.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.

Modules/_elementtree.c

@@ -131,6 +131,15 @@ static PyObject* elementpath_obj;
 
 /* helpers */
 
+/* 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);
+}
+
 LOCAL(PyObject*)
 deepcopy(PyObject* object, PyObject* memo)
 {
@@ -585,12 +594,10 @@ element_clear(ElementObject* self, PyObject* args)
     }
 
     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;
 }
@@ -610,13 +617,11 @@ element_copy(ElementObject* self, PyObject* args)
     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) {
 
@@ -678,14 +683,12 @@ element_deepcopy(ElementObject* self, PyObject* args)
     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) {
 
@@ -1624,13 +1627,11 @@ element_setattr(ElementObject* self, const char* name, PyObject* value)
         Py_INCREF(value);
         Py_SETREF(self->tag, value);
     } else if (strcmp(name, "text") == 0) {
-        Py_DECREF(JOIN_OBJ(self->text));
-        self->text = value;
-        Py_INCREF(self->text);
+        Py_INCREF(value);
+        _set_joined_ptr(&self->text, value);
     } else if (strcmp(name, "tail") == 0) {
-        Py_DECREF(JOIN_OBJ(self->tail));
-        self->tail = value;
-        Py_INCREF(self->tail);
+        Py_INCREF(value);
+        _set_joined_ptr(&self->tail, value);
     } else if (strcmp(name, "attrib") == 0) {
         if (!self->extra)
             element_new_extra(self, NULL);