Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improve thread-safety of ElementObjects in ``element_ass_subscr``
39 changes: 35 additions & 4 deletions Modules/_elementtree.c
Original file line number Diff line number Diff line change
Expand Up @@ -1863,6 +1863,35 @@ element_subscr(PyObject *op, PyObject *item)
}
}

// Pointer-by-pointer memmove for PyObject** arrays that is safe
// for shared ElementObjects in Py_GIL_DISABLED builds.
static void
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.

I'm not sure we should duplicate ptr_wise_atomic_memmove here.

Could you please link to the related C-API Working group issue in this PR, and cross-reference it in the original issue?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since the function is not exposed as part of the public API, I don't think linking a C-API Working Group issue is needed here.

Anyway, it's inspired by #149936

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.

I'll add a link to capi-workgroup/decisions#106 for traceability.

ptr_wise_atomic_memmove(ElementObject *a, PyObject **dest, PyObject **src, Py_ssize_t n)
{
#ifndef Py_GIL_DISABLED
memmove(dest, src, n * sizeof(PyObject *));
#else
// XXX: maybe a critical section isn't needed for ElementObject?
// _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
}

static int
element_ass_subscr(PyObject *op, PyObject *item, PyObject *value)
{
Expand Down Expand Up @@ -1938,19 +1967,21 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value)

PyList_SET_ITEM(recycle, i, self->extra->children[cur]);

memmove(
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(
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;
Expand Down
Loading