gh-146022: Fix free-threading crash in xml.etree.ElementTree's Element - #157000
Closed
NAVEENKUMARKR777 wants to merge 1 commit into
Closed
gh-146022: Fix free-threading crash in xml.etree.ElementTree's Element#157000NAVEENKUMARKR777 wants to merge 1 commit into
NAVEENKUMARKR777 wants to merge 1 commit into
Conversation
…Element _elementtree.c declares Py_MOD_GIL_NOT_USED but has no locking at all, so concurrent readers and writers race on the Element type's internal ElementObjectExtra struct (self->extra). For example, Element.clear() sets self->extra to NULL and frees the old struct while another thread concurrently indexes or measures the length of the same element, causing a use-after-free/NULL-deref segfault. Add the standard Py_BEGIN/END_CRITICAL_SECTION locking to every raw C slot (__getitem__, __setitem__, __len__, __bool__, and the two subscript slots, via the established *_lock_held extraction pattern for functions with multiple return paths) and every tag/text/tail/attrib property accessor, and add @critical_section Argument Clinic annotations to the methods that read or write self->extra (append, clear, extend, insert, remove, set, get, items, keys, find/findtext/ findall, __copy__/__deepcopy__/__sizeof__/__getstate__/__setstate__). Verified against the reported crash reproducer (reliably segfaults without this change, does not with it), a broader multithreaded stress test exercising every touched method concurrently, and the full test_xml_etree/test_xml_etree_c suites on both a regular and a free-threaded (--disable-gil) Windows build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
|
I clearly said on the issue:
If you don't respect this, we will restrict your access to our repositories next time. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes gh-146022.
Modules/_elementtree.cdeclaresPy_MOD_GIL_NOT_USEDbut has no locking at all, so concurrent readers and writers race onElement's internalElementObjectExtrastruct (self->extra). For example,Element.clear()setsself->extratoNULLand frees the old struct while another thread concurrently indexes or measures the length of the same element, causing a use-after-free/NULL-deref segfault — exactly as reported, with a reliable repro script.Changes
Add the standard critical-section locking used throughout the rest of the free-threaded build:
self->extra:element_getitem(__getitem__),element_setitem(__setitem__),element_length(__len__),element_bool(__bool__), and the two subscript slots (element_subscr/element_ass_subscr, via the established*_lock_heldextraction pattern already used inlistobject.cfor functions with multiple return paths).tag/text/tail/attribproperty getters/setters.@critical_sectionArgument Clinic annotations on the methods that read or writeself->extra:append,clear,extend,insert,remove,set,get,items,keys,find/findtext/findall,__copy__/__deepcopy__/__sizeof__/__getstate__/__setstate__.Along the way, this surfaced a real MSVC portability issue: a
goto-to-label pattern right beforePy_END_CRITICAL_SECTION()is invalid pre-C23 when the critical-section macros expand to plain braces (as they do in GIL-enabled builds) —element_setitemwas restructured to avoid it, in line with how the rest of the codebase avoids bare labels immediately preceding the macro's closing brace.This is intentionally scoped to
Element's core state (self->extra,tag,text,tail). The lazy tree-walking iterator (iter/itertext, backingElementIter) andTreeBuilder/XMLParserare a separate, larger subsystem and are out of scope here.Test plan
--disable-gilWindows build.test_xml_etreeandtest_xml_etree_c(507 tests) pass on both a regular and a free-threaded build.🤖 Generated with Claude Code