| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
@dpdani Thanks for you review! |
Sorry, something went wrong.
|
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): import threading
import xml.etree.ElementTree as ET
NTHREADS = 8
barrier = threading.Barrier(NTHREADS)
def worker(elem):
barrier.wait()
for _ in range(4000):
_ = elem.attrib # if (!self->extra) create_extra(...) -- unlocked lazy init
_ = len(elem) # element_length reads self->extra
for _ in range(200):
shared = ET.Element("tag") # extra == NULL until first attrib/child touch
ts = [threading.Thread(target=worker, args=(shared,)) for _ in range(NTHREADS)]
for t in ts: t.start()
for t in ts: 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.) |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| return -1; | ||
| } | ||
|
|
||
| Py_BEGIN_CRITICAL_SECTION(self); |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
|
||
| if (!self->extra) | ||
| return; | ||
| Py_BEGIN_CRITICAL_SECTION(self); |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| Py_VISIT(JOIN_OBJ(self->text)); | ||
| Py_VISIT(JOIN_OBJ(self->tail)); | ||
|
|
||
| Py_BEGIN_CRITICAL_SECTION(self); |
There was a problem hiding this comment.
The GC runs during a stop-the-world pause, so there's no need for locking here.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.