diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index dbc0ebf994fd1e..a979dba5168187 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -8,6 +8,7 @@ extern "C" { # error "this header requires Py_BUILD_CORE define" #endif +#include "pycore_critical_section.h" // _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED #include "pycore_wasm_trampoline.h" // _PyCFunction_TrampolineCall() #include "pycore_gc.h" // _PyObject_GC_TRACK() #include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_LOAD_PTR_ACQUIRE() @@ -1017,6 +1018,38 @@ static inline Py_ALWAYS_INLINE void _Py_INCREF_MORTAL(PyObject *op) * references. */ PyAPI_FUNC(int) _PyObject_VisitType(PyObject *op, visitproc visit, void *arg); +// Pointer-by-pointer memmove for PyObject** arrays that is safe for shared +// objects in Py_GIL_DISABLED builds. Locking is the caller's responsibility. +static inline void +_PyObject_ptr_wise_atomic_memmove(PyObject *a, PyObject **dest, PyObject **src, + Py_ssize_t n) +{ +#ifndef Py_GIL_DISABLED + (void)a; + memmove(dest, src, n * sizeof(PyObject *)); +#else + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(a); + if (_Py_IsOwnedByCurrentThread(a) && !_PyObject_GC_IS_SHARED(a)) { + // No other threads can read this object's array concurrently + memmove(dest, src, n * sizeof(PyObject *)); + return; + } + if (dest < src) { + for (Py_ssize_t i = 0; i != n; i++) { + _Py_atomic_store_ptr_release(&dest[i], src[i]); + } + } + else { + // copy backwards to avoid overwriting src before it's read + for (Py_ssize_t i = n; i != 0; i--) { + _Py_atomic_store_ptr_release(&dest[i - 1], src[i - 1]); + } + } +#endif +} + +#define _PyObject_ptr_wise_atomic_memmove(a, dest, src, n) _PyObject_ptr_wise_atomic_memmove(_PyObject_CAST(a), dest, src, n) + #ifdef __cplusplus } #endif diff --git a/Lib/test/test_xml_etree_c.py b/Lib/test/test_xml_etree_c.py index deb2bd8d572581..58dd9cab45b1a6 100644 --- a/Lib/test/test_xml_etree_c.py +++ b/Lib/test/test_xml_etree_c.py @@ -2,6 +2,7 @@ import io import struct from test import support +from test.support import threading_helper from test.support.import_helper import import_fresh_module import types import unittest @@ -193,6 +194,48 @@ def test_disallow_instantiation(self): support.check_disallow_instantiation(self, iter_type) +@unittest.skipUnless(cET, 'requires _elementtree') +@threading_helper.requires_working_threading() +class ThreadingTests(unittest.TestCase): + def test_concurrent_slice_deletion(self): + self.check_concurrent_slice_deletion(slice(1, 65)) + + def test_concurrent_extended_slice_deletion(self): + for index in (slice(1, 65, 3), slice(64, 0, -3), slice(1, None, 3)): + with self.subTest(index=index): + self.check_concurrent_slice_deletion(index) + + def check_concurrent_slice_deletion(self, index): + # gh-149965: Read children while slice deletion shifts the child + # pointers. Create the elements here so the writer does not own them. + roots = [] + expected = [] + for _ in range(100): + root = cET.Element('root') + children = [cET.SubElement(root, str(i)) for i in range(100)] + del children[index] + roots.append(root) + expected.append(children) + + def delete(): + for root in roots: + del root[index] + + def read(): + for _ in range(10): + for root in roots: + # Each slice leaves at least two children. Their identity + # can change during deletion, but they must remain valid. + child = root[1] + self.assertIsInstance(child, cET.Element) + self.assertIn(child.tag, tags) + + tags = {str(i) for i in range(100)} + threading_helper.run_concurrently([delete, read, read]) + for root, children in zip(roots, expected): + self.assertEqual(root[:], children) + + @unittest.skipUnless(cET, 'requires _elementtree') class TestAliasWorking(unittest.TestCase): # Test that the cET alias module is alive diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-06-12-19-38-28.gh-issue-149965.w5eiS4.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-06-12-19-38-28.gh-issue-149965.w5eiS4.rst new file mode 100644 index 00000000000000..416c7451b19810 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-06-12-19-38-28.gh-issue-149965.w5eiS4.rst @@ -0,0 +1 @@ +Fix thread-safety issues in ``xml.etree`` diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index cee564c2c1c1f0..0f79adb46472d2 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -18,6 +18,7 @@ #include "Python.h" #include "pycore_ceval.h" // _Py_EnterRecursiveCall() #include "pycore_dict.h" // _PyDict_CopyAsDict() +#include "pycore_object.h" // _PyObject_ptr_wise_atomic_memmove() #include "pycore_pyhash.h" // _Py_HashSecret #include "pycore_tuple.h" // _PyTuple_FromPair #include "pycore_weakref.h" // FT_CLEAR_WEAKREFS() @@ -1947,7 +1948,7 @@ element_subscr(PyObject *op, PyObject *item) } static int -element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) +element_ass_subscr_lock_held(PyObject *op, PyObject *item, PyObject *value) { ElementObject *self = _Element_CAST(op); @@ -2021,19 +2022,21 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) PyList_SET_ITEM(recycle, i, self->extra->children[cur]); - memmove( + _PyObject_ptr_wise_atomic_memmove( + self, self->extra->children + cur - i, self->extra->children + cur + 1, - num_moved * sizeof(PyObject *)); + num_moved); } /* Leftover "tail" after the last removed child */ cur = start + (size_t)slicelen * step; if (cur < (size_t)self->extra->length) { - memmove( + _PyObject_ptr_wise_atomic_memmove( + self, self->extra->children + cur - slicelen, self->extra->children + cur, - (self->extra->length - cur) * sizeof(PyObject *)); + self->extra->length - cur); } self->extra->length -= slicelen; @@ -2135,6 +2138,16 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) } } +static int +element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) +{ + int res = 0; + Py_BEGIN_CRITICAL_SECTION(op); + res = element_ass_subscr_lock_held(op, item, value); + Py_END_CRITICAL_SECTION(); + return res; +} + static PyObject* element_tag_getter(PyObject *op, void *closure) { diff --git a/Objects/listobject.c b/Objects/listobject.c index f9488cc5c384ba..d7f06a76383d7a 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -10,7 +10,7 @@ #include "pycore_list.h" // struct _Py_list_freelist, _PyListIterObject #include "pycore_long.h" // _PyLong_DigitCount #include "pycore_modsupport.h" // _PyArg_NoKwnames() -#include "pycore_object.h" // _PyObject_GC_TRACK(), _PyDebugAllocatorStats() +#include "pycore_object.h" // _PyObject_GC_TRACK(), _PyObject_ptr_wise_atomic_memmove() #include "pycore_pyatomic_ft_wrappers.h" #include "pycore_setobject.h" // _PySet_NextEntry() #include "pycore_stackref.h" // _Py_TryIncrefCompareStackRef() @@ -499,15 +499,12 @@ end:; return ret; } -static void ptr_wise_atomic_memmove(PyListObject *a, PyObject **dest, - PyObject **src, Py_ssize_t n); - static inline void list_shift_items_right_lock_held(PyListObject *self, Py_ssize_t first, Py_ssize_t last) { #ifdef Py_GIL_DISABLED - ptr_wise_atomic_memmove(self, &self->ob_item[first + 1], + _PyObject_ptr_wise_atomic_memmove(self, &self->ob_item[first + 1], &self->ob_item[first], last - first); #else PyObject **items = self->ob_item; @@ -522,7 +519,7 @@ list_shift_items_left_lock_held(PyListObject *self, Py_ssize_t first, Py_ssize_t last) { #ifdef Py_GIL_DISABLED - ptr_wise_atomic_memmove(self, &self->ob_item[first], + _PyObject_ptr_wise_atomic_memmove(self, &self->ob_item[first], &self->ob_item[first + 1], last - first); #else PyObject **items = self->ob_item; @@ -967,33 +964,6 @@ list_clear_slot(PyObject *self) return 0; } -// Pointer-by-pointer memmove for PyObject** arrays that is safe -// for shared lists in Py_GIL_DISABLED builds. -static void -ptr_wise_atomic_memmove(PyListObject *a, PyObject **dest, PyObject **src, Py_ssize_t n) -{ -#ifndef Py_GIL_DISABLED - memmove(dest, src, n * sizeof(PyObject *)); -#else - _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(a); - if (_Py_IsOwnedByCurrentThread((PyObject *)a) && !_PyObject_GC_IS_SHARED(a)) { - // No other threads can read this list concurrently - memmove(dest, src, n * sizeof(PyObject *)); - return; - } - if (dest < src) { - for (Py_ssize_t i = 0; i != n; i++) { - _Py_atomic_store_ptr_release(&dest[i], src[i]); - } - } - else { - // copy backwards to avoid overwriting src before it's read - for (Py_ssize_t i = n; i != 0; i--) { - _Py_atomic_store_ptr_release(&dest[i - 1], src[i - 1]); - } - } -#endif -} /* a[ilow:ihigh] = v if v != NULL. * del a[ilow:ihigh] if v == NULL. @@ -1066,7 +1036,7 @@ list_ass_slice_lock_held(PyListObject *a, Py_ssize_t ilow, Py_ssize_t ihigh, PyO if (d < 0) { /* Delete -d items */ Py_ssize_t tail = Py_SIZE(a) - ihigh; - ptr_wise_atomic_memmove(a, &item[ihigh+d], &item[ihigh], tail); + _PyObject_ptr_wise_atomic_memmove(a, &item[ihigh+d], &item[ihigh], tail); (void)list_resize(a, Py_SIZE(a) + d); // NB: shrinking a list can't fail item = a->ob_item; } @@ -1075,7 +1045,7 @@ list_ass_slice_lock_held(PyListObject *a, Py_ssize_t ilow, Py_ssize_t ihigh, PyO if (list_resize(a, k+d) < 0) goto Error; item = a->ob_item; - ptr_wise_atomic_memmove(a, &item[ihigh+d], &item[ihigh], k - ihigh); + _PyObject_ptr_wise_atomic_memmove(a, &item[ihigh+d], &item[ihigh], k - ihigh); } for (k = 0; k < n; k++, ilow++) { PyObject *w = vitem[k]; @@ -1166,7 +1136,7 @@ list_inplace_repeat_lock_held(PyListObject *self, Py_ssize_t n) Py_ssize_t copied = input_size; while (copied < output_size) { Py_ssize_t items_to_copy = Py_MIN(copied, output_size - copied); - ptr_wise_atomic_memmove(self, items + copied, items, items_to_copy); + _PyObject_ptr_wise_atomic_memmove(self, items + copied, items, items_to_copy); copied += items_to_copy; } #endif @@ -1664,8 +1634,8 @@ list_pop_impl(PyListObject *self, Py_ssize_t index) } Py_ssize_t size_after_pop = Py_SIZE(self) - 1; if (index < size_after_pop) { - ptr_wise_atomic_memmove(self, &items[index], &items[index+1], - size_after_pop - index); + _PyObject_ptr_wise_atomic_memmove(self, &items[index], &items[index+1], + size_after_pop - index); } list_resize(self, size_after_pop); // NB: shrinking a list can't fail return v; @@ -3851,12 +3821,12 @@ list_ass_subscript_lock_held(PyObject *_self, PyObject *item, PyObject *value) lim = Py_SIZE(self) - cur - 1; } - ptr_wise_atomic_memmove(self, self->ob_item + cur - i, + _PyObject_ptr_wise_atomic_memmove(self, self->ob_item + cur - i, self->ob_item + cur + 1, lim); } cur = start + (size_t)slicelength * step; if (cur < (size_t)Py_SIZE(self)) { - ptr_wise_atomic_memmove(self, self->ob_item + cur - slicelength, + _PyObject_ptr_wise_atomic_memmove(self, self->ob_item + cur - slicelength, self->ob_item + cur, Py_SIZE(self) - cur); }