Skip to content

gh-149816: Fix race conditions in Modules/_elementtree.c with free-threaded - #149918

Open
ByteFlowing1337 wants to merge 9 commits into
python:mainfrom
ByteFlowing1337:fix-149816
Open

gh-149816: Fix race conditions in Modules/_elementtree.c with free-threaded#149918
ByteFlowing1337 wants to merge 9 commits into
python:mainfrom
ByteFlowing1337:fix-149816

Conversation

@ByteFlowing1337

@ByteFlowing1337ByteFlowing1337 commented May 16, 2026

Copy link
Copy Markdown
Contributor

This PR fixes (69) Unsynchronized extra pointer dereference in len in Modules/_elementtree.c mentioned in #149816.
It also fixes race condition in getting attrib.

Comment threadLib/test/test_xml_etree_c.py Outdated
Comment threadLib/test/test_xml_etree_c.py Outdated
Comment threadMisc/NEWS.d/next/Library/2026-05-16-19-13-58.gh-issue-149816.Ht-jC5.rst Outdated
Comment threadModules/_elementtree.c Outdated
Comment threadModules/_elementtree.c Outdated
Comment threadModules/_elementtree.c Outdated
@ByteFlowing1337

Copy link
Copy Markdown
ContributorAuthor

@dpdani Thanks for you review!

@devdanzin

Copy link
Copy Markdown
Member

Independent confirmation of item (69) "Unsynchronized extra pointer dereference in len in Modules/_elementtree.c", found by ThreadSanitizer fuzzing (fusil --tsan). Worth noting the race is a bit broader than a read-deref: it's a write/write on the lazily-allocated self->extra, because the if (!self->extra) create_extra(...) guard in the extra accessors isn't atomic. Two threads first-touching a shared Element both take the !self->extra branch and both run create_extra, which does self->extra = PyMem_Malloc(...) (_elementtree.c:274) with no critical section — so one allocation is overwritten (leaked) and readers can observe a torn pointer.

Minimal deterministic reproducer (exit 66 under TSan; create_extra:274 as the write, reached via element_attrib_getter / element_length):

importthreadingimportxml.etree.ElementTreeasETNTHREADS=8barrier=threading.Barrier(NTHREADS)
defworker(elem):
barrier.wait()
for_inrange(4000):
_=elem.attrib# if (!self->extra) create_extra(...) -- unlocked lazy init_=len(elem) # element_length reads self->extrafor_inrange(200):
shared=ET.Element("tag") # extra == NULL until first attrib/child touchts= [threading.Thread(target=worker, args=(shared,)) for_inrange(NTHREADS)]
fortints: t.start()
fortints: t.join()

Confirmed still present on current main (3.16.0a0), on both a debug and a release --with-thread-sanitizer build. PR #149918's approach — taking Py_BEGIN_CRITICAL_SECTION(self) around the if (!self->extra) create_extra(...) check-and-create and the other extra accessors — covers this (both the read-deref and the write/write faces). Faces the fuzzer also hit: create_extra | element_length and clear_extra | create_extra.

(Found by fusil --tsan, a ThreadSanitizer fuzzer; draft and reproducer by Claude Code, minimized and reviewed by hand.)

@dpdanidpdani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this approach is not going to be effective. While it does resolve the sharpest edges, it still leaves some race conditions that need to be resolved.

return -1;
}

Py_BEGIN_CRITICAL_SECTION(self);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't do locking here. There are a few callers of this function, and locking should be moved to the callers. For instance, this code in element_resize would not be thread safe with an inner critical section:

if (!self->extra) {
if (create_extra(self, NULL) <0)
return-1;
}

Allocations and init functions need not be thread safe.


if (!self->extra)
return;
Py_BEGIN_CRITICAL_SECTION(self);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do not add locking here. The only code path that may call this code concurrently is in element_setstate_from_attributes, and locking should be moved there. It is probably needed anyway by the looks of it.

Py_VISIT(JOIN_OBJ(self->text));
Py_VISIT(JOIN_OBJ(self->tail));

Py_BEGIN_CRITICAL_SECTION(self);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The GC runs during a stop-the-world pause, so there's no need for locking here.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ByteFlowing1337@devdanzin@dpdani