FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-126033: fix a crash in `xml.etree.ElementTree.Element.remove` when concurrent mutations happen by picnixz · Pull Request #126124 · python/cpython · GitHub

/ cpython Public

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

Merged
picnixz merged 31 commits into
python:mainfrom
picnixz:fix/xml-evil-remove-126033
Mar 31, 2025
ConversationCommits (31)ChecksFiles changed
Merged

gh-126033: fix a crash in xml.etree.ElementTree.Element.remove when concurrent mutations happen#126124
picnixz merged 31 commits into
python:mainfrom
picnixz:fix/xml-evil-remove-126033

Conversation

picnixz commented Oct 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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).

bedevere-app Bot mentioned this pull request Oct 29, 2024
Closed
picnixz requested a review from sobolevn October 29, 2024 12:31
picnixz force-pushed the fix/xml-evil-remove-126033 branch from bb4f6d2 to 59ade8f Compare October 29, 2024 12:33
sobolevn reviewed Oct 29, 2024
View reviewed changes
Comment thread Modules/_elementtree.c Outdated
serhiy-storchaka reviewed Dec 6, 2024
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Misc/NEWS.d/next/Library/2024-10-29-12-59-45.gh-issue-126033.sM3uCn.rst Outdated
Comment thread Lib/test/test_xml_etree.py

picnixz commented Dec 6, 2024

Copy link
Copy Markdown
Member Author

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 commented Dec 6, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 commented Dec 6, 2024

Copy link
Copy Markdown
Member

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 commented Dec 6, 2024

Copy link
Copy Markdown
Member Author

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 commented Dec 6, 2024

Copy link
Copy Markdown
Member

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 commented Dec 6, 2024

Copy link
Copy Markdown
Member Author

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 marked this pull request as draft December 6, 2024 18:03
picnixz marked this pull request as ready for review December 8, 2024 13:08
picnixz requested a review from vstinner February 12, 2025 11:21

picnixz commented Mar 14, 2025

Copy link
Copy Markdown
Member Author

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

vstinner commented Mar 14, 2025

Copy link
Copy Markdown
Member

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

picnixz commented Mar 14, 2025

Copy link
Copy Markdown
Member Author

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 commented Mar 15, 2025

Copy link
Copy Markdown
Member Author

@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 approved these changes Mar 31, 2025
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

picnixz merged commit bab1398 into python:main Mar 31, 2025

miss-islington-app Bot commented Mar 31, 2025

Copy link
Copy Markdown

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

picnixz deleted the fix/xml-evil-remove-126033 branch March 31, 2025 10:31
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 31, 2025
pythongh-126033: fix UAF in xml.etree.ElementTree.Element.remove wh…
1c6193c 
…en concurrent mutations happen (pythonGH-126124)

(cherry picked from commit bab1398)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

bedevere-app Bot commented Mar 31, 2025

Copy link
Copy Markdown

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

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Mar 31, 2025
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 31, 2025
pythongh-126033: fix UAF in xml.etree.ElementTree.Element.remove wh…
e1aa52c 
…en concurrent mutations happen (pythonGH-126124)

(cherry picked from commit bab1398)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

bedevere-app Bot commented Mar 31, 2025

Copy link
Copy Markdown

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

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Mar 31, 2025
picnixz added a commit that referenced this pull request Mar 31, 2025
[3.13] gh-126033: fix UAF in xml.etree.ElementTree.Element.remove w…
b41c8cc 
…hen concurrent mutations happen (GH-126124) (#131929)

gh-126033: fix UAF in `xml.etree.ElementTree.Element.remove` when concurrent mutations happen (GH-126124)
(cherry picked from commit bab1398)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
picnixz added a commit that referenced this pull request Mar 31, 2025
[3.12] gh-126033: fix UAF in xml.etree.ElementTree.Element.remove w…
5d4e891 
…hen concurrent mutations happen (GH-126124) (#131930)

gh-126033: fix UAF in `xml.etree.ElementTree.Element.remove` when concurrent mutations happen (GH-126124)
(cherry picked from commit bab1398)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
picnixz mentioned this pull request Mar 31, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner approved these changes
sobolevn Awaiting requested review from sobolevn
serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees

No one assigned

Labels

topic-XML

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL