gh-148735: Fix a UAF in Element.findtext()#148738
gh-148735: Fix a UAF in Element.findtext()#148738StanFromIreland wants to merge 2 commits intopython:mainfrom
Element.findtext()#148738Conversation
`Element.findtext()`
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
All the occurrences you change are of the form (get borrowed) followed by (incref) right? if so, it is ok to change it to (get strong ref) but otherwise we may have un-necessary incref/decref. Currently XML is not FT-safe but if we (I) want to make it so, we should avoid incref/decref in hot paths
| @@ -0,0 +1,4 @@ | |||
| :mod:`xml.etree.ElementTree`: Fix a use-after-free in | |||
| :meth:`Element.findtext <xml.etree.ElementTree.Element.findtext>` when the | |||
| tag to find implements an :meth:`~object.__eq__` method that drops every | |||
There was a problem hiding this comment.
Just mention that this happens when there are mutations. Using eq is one possibility (probably the only one really easy to trigger though)
| @@ -0,0 +1,4 @@ | |||
| :mod:`xml.etree.ElementTree`: Fix a use-after-free in | |||
| :meth:`Element.findtext <xml.etree.ElementTree.Element.findtext>` when the | |||
| tag to find implements an :meth:`~object.__eq__` method that drops every | |||
There was a problem hiding this comment.
This describes how to reproduce the issue. But this is not what happens in most user code. Users rarely implement __eq__ which intentionally mutates the element. More realistic scenario -- it is mutated concurrently with running findtext(). This usually happens with threads, but can also happen with garbage collector or indirectly in __eq__. Anyway, the cause is a concurrent mutation.
The number of incerfs/decrefs remains the same. They are not unnecessary. |
|
Yeah I was not sure about this as I am on mobile. Thanks for checking |
3 participants
This was a regression introduced by c57623c.
cpython/Modules/_elementtree.c
Line 575 in ae55e9c
Element.findtext()when tag__eq__mutates parent #148735