address review

Author: ByteFlowing1337

Lib/test/test_xml_etree_c.py

@@ -1,12 +1,12 @@
 # xml.etree test for cElementTree
 import io
 import struct
+import time
 from test import support
 from test.support.import_helper import import_fresh_module
 from test.support import threading_helper
 import types
 import unittest
-import threading
 
 cET = import_fresh_module('xml.etree.ElementTree',
                           fresh=['_elementtree'])
@@ -258,18 +258,19 @@ def test_element_with_children(self):
         self.check_sizeof(e, self.elementsize + self.extra +
                              struct.calcsize('8P'))
 
+
 @unittest.skipUnless(cET, 'requires _elementtree')
 @threading_helper.requires_working_threading()
 class TestElementTreeFreeThreading(unittest.TestCase):
     def test_element_concurrent_clear_and_access(self):
-        #Race len(), .attrib, and .clear() to verify fix for gh-149861.
+        # Race len(), .attrib, and .clear() to verify fix for gh-149861.
         root = cET.Element('root')
         children = [cET.Element(f'child-{i}') for i in range(5)]
 
-        stop_event = threading.Event()
+        end_time = time.monotonic() + 1.0
 
         def reader_task():
-            while not stop_event.is_set():
+            while time.monotonic() < end_time:
                 len(root)
                 try:
                     _ = root.attrib
@@ -279,23 +280,19 @@ def reader_task():
                     pass
 
         def writer_task():
-            while not stop_event.is_set():
+            while time.monotonic() < end_time:
                 # Test element_add_subelement / extend
                 root.extend(children)
                 # Test clear_extra
                 root.clear()
 
         threads = []
         for _ in range(4):
-            threads.append(threading.Thread(target=reader_task))
+            threads.append(reader_task)
         for _ in range(2):
-            threads.append(threading.Thread(target=writer_task))
-
-        with threading_helper.start_threads(threads):
-            import time
-            time.sleep(1.0)
-            stop_event.set()
+            threads.append(writer_task)
 
+        threading_helper.run_concurrently(worker_func=threads, nthreads=6)
 
 
 def install_tests():

Misc/NEWS.d/next/Library/2026-05-16-19-13-58.gh-issue-149816.Ht-jC5.rst

@@ -1,4 +1,4 @@
 Fixes race conditions in :mod:`xml.etree.ElementTree` where the ``extra``
-pointer dereference and ``attrib`` access was unsynchronized . This prevents
+pointer dereference and ``attrib`` access was unsynchronized. This prevents
 potential crashes (core dumps) in the free-threading build caused by
 concurrent modification of an element's internal structure.

Modules/_elementtree.c

@@ -271,31 +271,20 @@ typedef struct {
 LOCAL(int)
 create_extra(ElementObject* self, PyObject* attrib)
 {
-    int res;
-    Py_BEGIN_CRITICAL_SECTION(self);
-
-    if (self->extra != NULL) {
-        res = 0;
-        goto end;
-    }
-    ElementObjectExtra* extra = PyMem_Malloc(sizeof(ElementObjectExtra));
-    if (!extra) {
+    self->extra = PyMem_Malloc(sizeof(ElementObjectExtra));
+    if (!self->extra) {
         PyErr_NoMemory();
-        res = -1;
-        goto end;
+        return -1;
     }
+    Py_BEGIN_CRITICAL_SECTION(self);
+    self->extra->attrib = Py_XNewRef(attrib);
 
-    extra->attrib = Py_XNewRef(attrib);
-
-    extra->length = 0;
-    extra->allocated = STATIC_CHILDREN;
-    extra->children = extra->_children;
-    self->extra = extra;
-    res = 0;
+    self->extra->length = 0;
+    self->extra->allocated = STATIC_CHILDREN;
+    self->extra->children = self->extra->_children;
 
-  end: ;
     Py_END_CRITICAL_SECTION();
-    return res;
+    return 0;
 }
 
 LOCAL(void)
@@ -575,23 +564,15 @@ element_get_attrib(ElementObject* self)
     /* return new reference to attrib dictionary */
     /* note: this function assumes that the extra section exists */
 
-    PyObject *res = NULL;
+    PyObject *res;
     Py_BEGIN_CRITICAL_SECTION(self);
-    if (self->extra == NULL) {
-        PyErr_SetString(PyExc_AttributeError, "extra section does not exist");
-        goto end;
-    }
     res = self->extra->attrib;
 
     if (!res) {
         /* create missing dictionary */
-        res = PyDict_New();
-        if (res) {
-            self->extra->attrib = res;
-        }
+        res = self->extra->attrib = PyDict_New();
     }
     Py_XINCREF(res);
-  end: ;
     Py_END_CRITICAL_SECTION();
     return res;
 }
@@ -709,7 +690,6 @@ static int
 element_gc_clear(PyObject *op)
 {
     ElementObject *self = _Element_CAST(op);
-    Py_BEGIN_CRITICAL_SECTION(self);
     Py_CLEAR(self->tag);
     _clear_joined_ptr(&self->text);
     _clear_joined_ptr(&self->tail);
@@ -718,7 +698,6 @@ element_gc_clear(PyObject *op)
      * so fully deallocate it.
     */
     clear_extra(self);
-    Py_END_CRITICAL_SECTION();
     return 0;
 }
 
@@ -1655,9 +1634,12 @@ static Py_ssize_t
 element_length(PyObject *op)
 {
     ElementObject *self = _Element_CAST(op);
-    Py_ssize_t res = 0;
+    Py_ssize_t res;
     Py_BEGIN_CRITICAL_SECTION(self);
-    if (self->extra) {
+    if (!self->extra) {
+        res = 0;
+    }
+    else {
         res = self->extra->length;
     }
     Py_END_CRITICAL_SECTION();
@@ -2114,16 +2096,15 @@ element_tail_getter(PyObject *op, void *closure)
 static PyObject*
 element_attrib_getter(PyObject *op, void *closure)
 {
-    PyObject *res = NULL;
+    PyObject *res;
     ElementObject *self = _Element_CAST(op);
     Py_BEGIN_CRITICAL_SECTION(self);
-    if (!self->extra) {
-        if (create_extra(self, NULL) < 0)
-            goto end;
+    if (!self->extra && create_extra(self, NULL) < 0){
+            res = NULL;
+    }
+    else {
+        res = element_get_attrib(self);
     }
-    res = element_get_attrib(self);
-
-  end: ;
     Py_END_CRITICAL_SECTION();
     return res;
 }