Free-threaded CPython 3.14: concurrent functools.partial.__setstate__ crashes with repr(), calls, or another __setstate__ #157841

Activity

1. aisk

   added

   extension-modulesC modules in the Modules dirC modules in the Modules dir

   type-crashA hard crash of the interpreter, possibly with a core dumpA hard crash of the interpreter, possibly with a core dump

   topic-free-threading

   on Sep 20, 2026
2. Adarsh-Me commented on Sep 21, 2026

   

   Adarsh-Me

   on Sep 21, 2026

   More actions

   I'd like to work on this, subject to one question.

   The analysis looks right: partial_setstate mutates p->args / p->keywords / p->func without holding the per-object mutex that guards the readers in partial_repr and partial_call, so the fix is to take partial->lock in partial_setstate (Modules/_functoolsmodule.c) with the same ordering the readers already use, and to make the swap atomic under free-threading rather than reassigning the fields in place.

   Two things before I write it: (1) the report notes gh-157196 was withdrawn to be resubmitted under the security policy — is this issue intended to be fixed in the open, or does it belong on the security tracker? (2) partial_new may need the same treatment if __setstate__ can race with a fresh construction. Happy to take whichever path you prefer, with a free-threaded regression test and a Misc/NEWS.d entry.
3. espressolee commented on Sep 21, 2026

   

   espressolee

   on Sep 21, 2026

   Author

   More actions

   Thanks for picking this up — and yes, please do.

   (1) Tracker. I filed this publicly on the assessment quoted in the report: triggering it requires already running Python code in the process, which was the assessment given on gh-157196. gh-157099, gh-157124 and gh-157196 are all closed as not planned by their author, and as far as I can see from the public trackers nothing follows them — but I can't see any non-public tracker, so whether this should move to the security process is for the maintainers to say, not me.

   (2) One correction to the premise, and it widens the patch. You wrote that partial_setstate mutates the fields "without holding the per-object mutex that guards the readers in partial_repr and partial_call". In the code I audited at 89c67a9, those readers do not take a critical section on the partial either — so there is no existing reader-side ordering for the writer to join:

   	at 89c67a9
   partial_setstate :801	no critical section; assigns fn, args, kw, phcount, dict at :856-860, then partial_setvectorcall at :861
   partial_repr :705	Py_NewRef(pto->fn/args/kw) at :724-726. The Py_BEGIN_CRITICAL_SECTION at :745 is on kw, entered after that reference is acquired, and protects the dict iteration
   partial_vectorcall :369	reads pto->phcount at :377, then Py_NewRef(pto->fn/args/kw) at :386-388
   partial_call :592	reads pto->phcount at :600 and PyDict_GET_SIZE(pto->kw) at :610 without taking a reference first

   I read your proposal as two parts, and I think the second is the load-bearing one. Making the swap atomic in the writer is necessary, but on its own it does not give a reader a coherent, live state: with no shared protocol, a reader still loads each field independently. So whatever the writer does, partial_repr / partial_vectorcall / partial_call need a matching acquisition path. Worth stating explicitly in the PR how readers get their snapshot.

   Two invariants that I think the fix has to establish, since they are separate:

   • Lifetime. Py_NewRef(pto->args) loads the field and then increments. A concurrent replacement can drop the last reference in between, so the acquisition itself needs to be safe, not just the use afterwards.
   • Coherence. partial_vectorcall samples phcount at :377 and args at :387 and combines them at :419 (tot_nargs = pto_nargs + nargs - pto_phcount). Nothing currently makes those two come from the same state. The nargs < pto_phcount guard at :378 does keep tot_nargs >= pto_nargs, so a stale count does not shrink that total below the sampled tuple — but it does not make the pair consistent either, and protecting object lifetimes alone would not fix it.

   I have not attributed the observed crashes to a particular native access, so please treat the second point as an invariant that needs to hold rather than as my diagnosis of the signal you'll see.

   Two directions I can see, without a view on which suits this object — that's a call for the core devs:

   • take a critical section on self in the writer and in each reader, acquiring the strong references and copying phcount out of one coherent state, then doing the formatting or the call after leaving it; or
   • keep the readers lock-free with a safe acquisition protocol, publishing the whole state as one unit so phcount cannot be sampled apart from args.

   Either way, two things are easy to miss: the displaced values' decrefs can run arbitrary code, so the new state should be fully installed before they run; and the vectorcall function pointer is part of the state too, not just the four fields.

   On partial_new: gh-157099 reached the same crash through it, so it belongs in the audit. My reproducer doesn't exercise that shape, so I'd rather see it covered by the access audit than guessed at.

   What I can offer. Both reader/writer modes — repr + __setstate__, and call + __setstate__ — crashed in all five runs of every free-threaded configuration I tried, while the writer/writer mode was intermittent (one to four crashes in five) and the read-only control never crashed. I haven't tested a writer-only patch, so I can't tell you from measurement what it would leave behind. Point me at a branch and I'll run all four modes, 5 runs × 4 s each, with and without PYTHONMALLOC=debug, on 3.14.6t and on a local --disable-gil build of main, plus the PYTHON_GIL=1 control on the same binary.
4. Adarsh-Me commented on Sep 23, 2026

   

   Adarsh-Me

   on Sep 23, 2026

   More actions

   Opened as #157979.

   @espressolee your correction of my premise was right and it changed the shape of the patch, so thank you for it. I had written "without holding the per-object mutex that guards the readers in partial_repr and partial_call", and as you pointed out there is no reader-side ordering there to join — so a writer-only atomic swap would have been the wrong fix. The PR takes the first of the two directions you sketched (a critical section on self in the writer and in each reader) because it is the one that reuses the protocol the object already has:

   • lifetime — the strong references are acquired inside the section, so nothing can drop the last reference between the load and the incref;
   • coherence — phcount and args are copied out of one state and the formatting or the call happens after leaving the section, so tot_nargs cannot mix two states;
   • the displaced values' decrefs run only after the new state is fully installed, and the vectorcall slot is published as part of the state rather than after it;
   • partial_new()'s flatten path is treated as a reader too, which is the gh-157099 shape you asked to be covered rather than guessed at.

   On measurement: your offer is exactly what this needs, because everything I have is one platform. The build I have is a Windows amd64t Release (MSC v.1929, 3.16.0a0). Against main's _functoolsmodule.c with the new tests present, test_concurrent_setstate_and_reads dies with Fatal Python error: PyMutex_Unlock: unlocking mutex that is not locked; against the PR branch all 7 tests in that file pass. The tests do not only watch for a crash — every state they install carries the same value twice, once positionally and once as a keyword, so a torn read fails an assertion. If you run your four modes against #157979's head, that would tell us whether the reader-side behaviour you measured (both reader/writer modes crashing in 5/5 runs) actually goes away, which is the claim I cannot make from a Windows box.

   On the tracker question, agreed that it is a call for the core devs rather than either of us — raising it explicitly in #157979 so whoever picks it up sees it: this is triggerable from already-running Python code, which is the assessment gh-157196 was closed under, but the earlier closures were by the author and there may be a non-public tracker nobody here can see.