Skip to content

gh-133009: fix UAF in xml.etree.ElementTree.Element.__deepcopy__ #133010

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

Merged
merged 8 commits into from
May 10, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions Lib/test/test_xml_etree.py
Original file line number Diff line number Diff line change
Expand Up @@ -2960,6 +2960,24 @@ def element_factory(x, y):
del b
gc_collect()

def test_deepcopy(self):
# Prevent crashes when __deepcopy__() mutates the element being copied.
# See https://github.com/python/cpython/issues/133009.
class X(ET.Element):
def __deepcopy__(self, memo):
root.clear()
return self

root = ET.Element('a')
evil = X('x')
root.extend([evil, ET.Element('y')])
if is_python_implementation():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also make the C implementation raising RuntimeError. It is fine either way.

Copy link
Member Author

@picnixz picnixz May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll postpone this for a future PR as I want to backport this one to 3.13 and 3.14 first.

self.assertRaises(RuntimeError, copy.deepcopy, root)
else:
c = copy.deepcopy(root)
# In the C implementation, we can still copy the evil element.
self.assertListEqual(list(c), [evil])


class MutationDeleteElementPath(str):
def __new__(cls, elem, *args):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
:mod:`xml.etree.ElementTree`: Fix a crash in :meth:`Element.__deepcopy__
<object.__deepcopy__>` when the element is concurrently mutated.
Patch by Bénédikt Tran.
29 changes: 22 additions & 7 deletions Modules/_elementtree.c
Original file line number Diff line number Diff line change
Expand Up @@ -805,6 +805,7 @@ _elementtree_Element___deepcopy___impl(ElementObject *self, PyObject *memo)
{
Py_ssize_t i;
ElementObject* element;
PyObject* tmp;
PyObject* tag;
PyObject* attrib;
PyObject* text;
Expand All @@ -813,12 +814,16 @@ _elementtree_Element___deepcopy___impl(ElementObject *self, PyObject *memo)

PyTypeObject *tp = Py_TYPE(self);
elementtreestate *st = get_elementtree_state_by_type(tp);
tag = deepcopy(st, self->tag, memo);
tmp = Py_NewRef(self->tag);
tag = deepcopy(st, tmp, memo);
Py_DECREF(tmp);
if (!tag)
return NULL;

if (self->extra && self->extra->attrib) {
tmp = Py_NewRef(self->extra->attrib);
attrib = deepcopy(st, self->extra->attrib, memo);
Py_DECREF(tmp);
if (!attrib) {
Py_DECREF(tag);
return NULL;
Expand All @@ -835,12 +840,16 @@ _elementtree_Element___deepcopy___impl(ElementObject *self, PyObject *memo)
if (!element)
return NULL;

text = deepcopy(st, JOIN_OBJ(self->text), memo);
tmp = Py_NewRef(JOIN_OBJ(self->text));
text = deepcopy(st, tmp, memo);
Py_DECREF(tmp);
if (!text)
goto error;
_set_joined_ptr(&element->text, JOIN_SET(text, JOIN_GET(self->text)));

tail = deepcopy(st, JOIN_OBJ(self->tail), memo);
tmp = Py_NewRef(JOIN_OBJ(self->tail));
tail = deepcopy(st, tmp, memo);
Py_DECREF(tmp);
if (!tail)
goto error;
_set_joined_ptr(&element->tail, JOIN_SET(tail, JOIN_GET(self->tail)));
Expand All @@ -850,9 +859,10 @@ _elementtree_Element___deepcopy___impl(ElementObject *self, PyObject *memo)
if (element_resize(element, self->extra->length) < 0)
goto error;

// TODO(picnixz): check for an evil child's __deepcopy__ on 'self'
for (i = 0; i < self->extra->length; i++) {
PyObject* child = deepcopy(st, self->extra->children[i], memo);
for (i = 0; self->extra && i < self->extra->length; i++) {
tmp = Py_NewRef(self->extra->children[i]);
PyObject* child = deepcopy(st, tmp, memo);
Py_DECREF(tmp);
if (!child || !Element_Check(st, child)) {
if (child) {
raise_type_error(child);
Expand All @@ -865,7 +875,12 @@ _elementtree_Element___deepcopy___impl(ElementObject *self, PyObject *memo)
}

assert(!element->extra->length);
element->extra->length = self->extra->length;
/*
* The original 'self->extra' may be gone at this point if deepcopy()
* had side-effects. However, 'i' is the number of copied items that
* we were able to successfully copy.
*/
element->extra->length = i;
}

/* add object to memo dictionary (so deepcopy won't visit it again) */
Expand Down
Loading