gh-126033: fix a crash in xml.etree.ElementTree.Element.remove when concurrent mutations happen

[picnixz]

No need for versioning. Holding a strong reference on the child being compared is enough if we additionally check that the number of children is not changed after the comparison.

The test coverage could be refined but I don't think I need to overcomplicate it even more. I still think we can have a UAF because when one clears the list in __eq__ but I couldn't find a way to trigger it so I just increfed the item before calling __eq__. If anyone thinks it's not needed, please tell me why.

The patch for find* may be more complicated, which is why I splitted the task into three (one for introducing the versioning so that we can work on those failures in parallel).

• Issue: UAF: xml.etree.ElementTree.Element.remove when concurrent mutations happen #126033

[serhiy-storchaka]

Is adding version necessary? Would not keeping a strong reference to a child be enough? Look at the list.remove() code.

[picnixz]

Would not keeping a strong reference to a child be enough? Look at the list.remove() code.

Oh yes. I'll try using this one as a basis.

[picnixz]

Would not keeping a strong reference to a child be enough? Look at the list.remove() code.

Ah I know why I couldn't make it work! it's because the children is not a list itself, but just a huge memory area that was dynamically allocated using PyMem_Malloc. What do you suggest I do?

[serhiy-storchaka]

The list storage is also just a huge memory area that was dynamically allocated using PyMem_Malloc. I do not see difference.

At worst, you can get different exceptions (or no exception) in C and Python implementations, but this is fine.

[picnixz]

The list storage is also just a huge memory area that was dynamically allocated using PyMem_Malloc. I do not see difference.

The difference is that the list object uses Py_SIZE but it's not possible to use this on the current implementation of element trees (or is there?)

[serhiy-storchaka]

Py_SIZE or self->extra->length -- there is not much difference between them. Both just read a word from memory. You may need to check that self->extra was not set to NULL during iteration, this is the only difference.

[picnixz]

Py_SIZE or self->extra->length -- there is not much difference between them

I'm pretty sure I first tried using INCREF/DECREF as usual and encountered an issue with that but I can't remember how. I'll try reproducing this tomorrow.

[picnixz]

@vstinner @serhiy-storchaka friendly ping in case you forgot about this one

[vstinner]

As I wrote, I don't think that 200 lines of tests are needed. I would prefer way less tests.

[picnixz]

I can remove the special test case which makes the test explode as it's only the Python implementation if you want. It would reduce the test by half at least (most of the line length is taken by comments that are needed to explain the discrepency between the Python and the C implementation).

[picnixz]

@vstinner I've removed the pedantic check. However, we still need to have that much of tests because del root[:] and root.clear() behave differently although they should do the same. The reproducer I had did not make crash del root[:], but it made crash root.clear().

[vstinner]

LGTM. The C code change LGTM, I didn't review the tests.

[miss-islington-app]

Thanks @picnixz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

[bedevere-app]

GH-131929 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-131930 is a backport of this pull request to the 3.12 branch.