Repository navigation
Conversation
partialobject's fn, args, kw, phcount, dict and vectorcall fields were read without synchronization by partial_repr(), partial_vectorcall(), partial_call(), partial_reduce() and partial_new() while partial_setstate() republished them, so a reader could observe a mixture of two states, or index a tuple that a concurrent __setstate__() had just dropped the last reference to. Every write now happens under the object's critical section, with the displaced values released after it because their finalizers can run arbitrary code. Each reader takes one coherent snapshot of the mutable fields as owned references and works from that. partial_new()'s flatten path takes owned references from the inner partial under that partial's lock, which is how pythongh-157099 reaches the same crash without any shared mutation of the object being built.
Race __setstate__() against repr(), a call through both tp_call and vectorcall, __reduce__() and a nested partial(), and assert that a reader never combines the positional arguments of one state with the keywords of another. Every state the test installs carries the same value twice, once reachable as a positional argument and once as a keyword, so a torn read fails as a mismatch instead of only as a crash. test_concurrent_setstate_writers passes without the fix; it is there to guard the ordering of the displaced-value decrefs, not to reproduce a failure.
|
The following commit authors need to sign the Contributor License Agreement: |
|
You need to sign the CLA before we can review. |
|
The So the racing pair is not any of the accesses I synchronised. The writer is I checked this is not something my change brought in: Two ways out, and I would rather get a steer than guess, because both reach outside
Until one of those lands, this PR fixes the state tearing and the lifetime hazard (the crash the issue reported — Also: the |
|
Thanks for opening the PR and following up. Sorry for the delayed reply. I haven’t run the four-mode validation I offered, so I don’t have independent test results to report. |
|
Additional bounded validation at 51351e7:
This is ordinary regression evidence, not the four-mode concurrent validation I offered; that remains unrun. It does not establish race freedom. Two details from the existing CI logs also seem relevant:
Those CI jobs checked out merge commit Exact ordinary-test selectionThe following methods in
AI assistance: Codex performed the build, selected-test execution and CI-log inspection. No source changes were made, and no concurrency reproducers were run in this follow-up. |
|
Follow-up: the four modes I offered are now executed against PR head Native macOS27.0.1/arm64, CPython3.16.0a0 built with Using the unchanged program from gh-157841, five fresh processes per cell, four seconds per process:
All80processes exited normally, with no signals,30-second timeouts or observed thread exceptions. A wrapper checked actual GIL state before/after and rejected otherwise-unhandled thread exceptions; separate negative fixtures confirmed exception rejection and timeout termination. The existing This is bounded evidence for this PR head. I did not run a patched3.14backport or repeat the old vulnerable-main baseline, and macOS has changed since the original report. It does not establish race freedom or resolve the existing TSan warnings/base attribution. AI assistance: Codex performed the execution, checks and result preparation. No source patch or sanitizer reproducer was run. |
|
@Adarsh-Me — I have completed a local TSan follow-up and put the evaluated implementation and a regression supplement below for review. @rhettinger, once the CLA prerequisite is cleared, could you advise whether this free-threading-only immutable-adapter approach is appropriate here? The audit at head I compared the standalone PR with its actual branching base
The first candidate also passed the three existing The candidate below keeps the partial's vectorcall slot immutable after construction in a free-threaded build. The existing generic vectorcall helper already handles a wrapped callable without vectorcall by falling back to its Local results for this candidate:
Each targeted workload was a single fresh process; this is bounded engineering evidence, not a failure-rate estimate or a proof of race freedom. Performance of always using the adapter for free-threaded partials, current-main integration, Linux CI and other platforms remain unqualified. I checked the current CI routing: Could you review/incorporate the appropriate changes and then run the Linux free-threaded TSan job on the resulting head, including the supplemented module and existing broader test selection? I have not rerun CI on the unchanged revision or treated the local run as Linux qualification. The two patches apply independently to AI assistance: Codex performed the audit, builds, tests, candidate and regression preparation. No source commit or push has been made for this follow-up. Raw logs, program and binary hashes are retained locally. Patch SHA-256 values: implementation Evaluated implementation delta against 51351e7 (one C file, +17/-7)diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c
index 79a0854..c97dc85 100644
--- a/Modules/_functoolsmodule.c
+++ b/Modules/_functoolsmodule.c
@@ -602,6 +602,12 @@ partial_vectorcall(PyObject *self, PyObject *const *args,
static void
partial_setvectorcall(partialobject *pto)
{
+#ifdef Py_GIL_DISABLED
+ /* Dispatch reads this slot before partial_vectorcall acquires our lock.
+ * Keep it immutable after construction; the generic vectorcall helper
+ * also supports wrapped callables without their own vectorcall slot. */
+ pto->vectorcall = partial_vectorcall;
+#else
if (PyVectorcall_Function(pto->fn) == NULL) {
/* Don't use vectorcall if the underlying function doesn't support it */
pto->vectorcall = NULL;
@@ -612,6 +618,7 @@ partial_setvectorcall(partialobject *pto)
else {
pto->vectorcall = partial_vectorcall;
}
+#endif
}
@@ -910,18 +917,22 @@ partial_setstate(PyObject *self, PyObject *state)
else
Py_INCREF(dict);
- PyObject *old_fn, *old_args, *old_kw, *old_dict;
+ PyObject *old_fn, *old_args, *old_kw;
Py_BEGIN_CRITICAL_SECTION(pto);
old_fn = pto->fn;
old_args = pto->args;
old_kw = pto->kw;
- old_dict = pto->dict;
- pto->fn = Py_NewRef(fn);
- pto->args = fnargs;
- pto->kw = kw;
+ /* Public member descriptors use atomic loads before their lock fallback. */
+ FT_ATOMIC_STORE_PTR_RELEASE(pto->fn, Py_NewRef(fn));
+ FT_ATOMIC_STORE_PTR_RELEASE(pto->args, fnargs);
+ FT_ATOMIC_STORE_PTR_RELEASE(pto->kw, kw);
pto->phcount = phcount;
- pto->dict = dict;
+#ifndef Py_GIL_DISABLED
partial_setvectorcall(pto);
+#endif
+ /* Generic attribute lookup reads this pointer without our lock. Match
+ * the generic __dict__ setter's atomic publication and QSBR retirement. */
+ _PyObject_XSetRefDelayed(&pto->dict, dict);
Py_END_CRITICAL_SECTION();
// The old values may have arbitrary finalizers, so they must be dropped
@@ -929,7 +940,6 @@ partial_setstate(PyObject *self, PyObject *state)
Py_XDECREF(old_fn);
Py_XDECREF(old_args);
Py_XDECREF(old_kw);
- Py_XDECREF(old_dict);
Py_RETURN_NONE;
}
Regression supplement: public member publication and dict lookupdiff --git a/Lib/test/test_free_threading/test_functools.py b/Lib/test/test_free_threading/test_functools.py
index 0c7000d..467b0ac 100644
--- a/Lib/test/test_free_threading/test_functools.py
+++ b/Lib/test/test_free_threading/test_functools.py
@@ -73,8 +73,8 @@ def test_reentrant_cache_clear_bounded(self):
class Callable:
- # Instances have no vectorcall slot, so partial() dispatches them through
- # tp_call (partial_call) instead of partial_vectorcall.
+ # Instances have no vectorcall slot; generic dispatch to the wrapped
+ # callable therefore falls back to its tp_call.
def __call__(self, x, y=0, *extra, z=0):
return (x, y, z)
@@ -195,5 +195,55 @@ def thread_func(n):
self.check_reduce_coherent(p)
+@threading_helper.requires_working_threading()
+class TestPartialPointerPublication(unittest.TestCase):
+ def _race_public_read(self, read):
+ args = (0, 0)
+ keywords = {"z": 0}
+ states = ((target, args, keywords, {"tag": 0}),
+ (target, args, keywords, {"tag": 1}))
+ p = partial(target, *args, **keywords)
+ setstate = p.__setstate__
+ setstate(states[0])
+ barrier = Barrier(2)
+ errors = []
+
+ def reader():
+ barrier.wait()
+ try:
+ for _ in range(20000):
+ read(p, args, keywords)
+ except Exception as exc:
+ errors.append(exc)
+
+ def writer():
+ barrier.wait()
+ try:
+ for i in range(20000):
+ setstate(states[i % 2])
+ except Exception as exc:
+ errors.append(exc)
+
+ # Retain both states to isolate pointer-publication races from
+ # destruction of the displaced objects. TSan must observe the public
+ # lookup/descriptor path, rather than an internal locked snapshot.
+ with threading_helper.start_threads([Thread(target=reader),
+ Thread(target=writer)]):
+ pass
+ self.assertEqual(errors, [])
+
+ def test_concurrent_setstate_func_member(self):
+ self._race_public_read(lambda p, args, kw: self.assertIs(p.func, target))
+
+ def test_concurrent_setstate_args_member(self):
+ self._race_public_read(lambda p, args, kw: self.assertIs(p.args, args))
+
+ def test_concurrent_setstate_keywords_member(self):
+ self._race_public_read(lambda p, args, kw: self.assertIs(p.keywords, kw))
+
+ def test_concurrent_setstate_dict_lookup(self):
+ self._race_public_read(lambda p, args, kw: self.assertIn(p.tag, (0, 1)))
+
+
if __name__ == "__main__":
unittest.main() |
|
I'm closing as the CLA has still not been signed. |
Bug report
functools.partialmutates its own state from__setstate__()with nosynchronisation against the readers of that state. Under
--disable-gilthefields (
func,args,keywords,phcount) and the vectorcall slot arepublished as independent stores, so a reader can combine two different states,
and the displaced values' decrefs can run arbitrary code while the object is
half-updated.
Coordinating with @espressolee on #157841, who pointed out that the readers
(
partial_repr,partial_call,partial_vectorcall) take no critical sectioneither at the current head — so making only the writer atomic would leave a
reader sampling each field independently. Two separate invariants had to hold:
Py_NewRef(pto->args)loads then increments, so aconcurrent replacement can drop the last reference in between; the
acquisition itself has to be inside the critical section, not just the use;
partial_vectorcallsamplesphcountandargsseparately and combines them into
tot_nargs, so both have to come fromone state.
How to fix the bug
Every access path takes a critical section on the partial and reads or writes
the whole state from it:
partial_setstate()builds the replacement values first, then installsfunc,args,keywords,phcountand the vectorcall pointer as oneunit, and only drops the displaced references after the new state is fully
published (their
__del__/finalisers can re-enter and read the partial);partial_repr(),partial_call()andpartial_vectorcall()take the samesection, copy out the strong references and
phcount, then do theformatting or the call after leaving it;
partial_new()'s flatten path reaches inside its argument to copyfunc/args/keywords, which is a reader like any other, so it goesthrough the same protocol and owns the references it takes.
PR feature rule
This is a bugfix, not a new feature.
Core Dev guide
Docs & Tools
existing method's behaviour.
Verification
Free-threaded
amd64tRelease build of this branch (MSC v.1929,3.16.0a0), Windows. The two newTestPartialcases were run against twobuilds that differ only in
Modules/_functoolsmodule.c:python -m test test_free_threading.test_functools -vmain's_functoolsmodule.c, new tests presenttest_concurrent_setstate_and_reads→Fatal Python error: PyMutex_Unlock: unlocking mutex that is not lockedRan 7 tests ... OK,Result: SUCCESSThe tests are 10 threads × 1000 operations with
sys.setswitchinterval(1e-6),covering
repr+__reduce__+ call +partial(p, 3)flattening against aconcurrent
__setstate__, and they check a torn read rather than only acrash: every state written carries the same value twice, once positionally and
once as a keyword, so a mixture of two states fails an assertion instead of
happening to survive.
What I did not measure: a writer-only patch (critical section in
partial_setstate()but not in the readers), which is the shape @espressoleerightly says would not give readers a coherent snapshot. Happy to add that data
point if it is useful, and to run anything on a Linux build — the numbers above
are Windows-only.