gh-149816: Fix race conditions in Modules/_elementtree.c with free-threaded - #149918
gh-149816: Fix race conditions in Modules/_elementtree.c with free-threaded#149918ByteFlowing1337 wants to merge 9 commits into
Modules/_elementtree.c with free-threaded#149918Conversation
|
@dpdani Thanks for you review! |
|
Independent confirmation of item (69) "Unsynchronized extra pointer dereference in len in Minimal deterministic reproducer (exit 66 under TSan; 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 (Found by |
| 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.
|
|
||
| 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.
| 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.
This PR fixes
(69) Unsynchronized extra pointer dereference in len in Modules/_elementtree.cmentioned in #149816.It also fixes race condition in getting
attrib.