Skip to content

Fix the race in the lazy declarations import - #398

Open
jensens wants to merge 1 commit into
masterfrom
fix-load-declarations-race
Open

jensens wants to merge 1 commit into
masterfrom
fix-load-declarations-race

Conversation

@jensens

@jensens jensens commented Nov 5, 2026

Copy link
Copy Markdown
Member

Fixes #391.

_zic_state_load_declarations checked decl_imported and set it after loading, with nothing in between to stop a second thread. Two problems:

  1. Leak: two threads both see the flag unset, both load, and the second overwrites the module state, so the first set of references is lost. This is not specific to free-threading: the import and the attribute lookups can release the GIL, so it also happens on GIL builds.
  2. Ordering (free-threaded only): the flag was a plain store after the field stores and a plain load on the reader side, so on weakly ordered CPUs (arm64) a reader could see the flag set and still read NULL fields.

The fix keeps the change local to that function:

  • Loading still happens without a lock (it runs Python code).
  • Publishing happens in a short Py_BEGIN_CRITICAL_SECTION(module) with no Python calls inside: the first thread stores its references and sets the flag, a later one drops its own.
  • On free-threaded builds the flag is read with acquire and set with release ordering (_Py_atomic_*); elsewhere these stay plain accesses. The fast path once loaded stays lock-free.

Test: a new reproducer in test_coptimizations_declarations_import.py makes two threads meet at a threading.Barrier inside the load (via a module __getattr__ for Implements), so both are guaranteed to load concurrently, then checks that each declarations object gained exactly one reference. It is deterministic and runs on GIL builds too.

Checked locally:

  • The new test fails on master on 3.11, 3.14 and 3.14t (each object +2 instead of +1) and passes with the fix (10/10 runs each).
  • Removing the loser's Py_DECREFs makes it fail again.
  • Full suite with and without the C extension on 3.11, 3.14 and 3.14t; no warnings with -Wall -Wextra; pre-commit passes.
  • The ordering part cannot be tested on x86; it rests on review.

This and #397 both add an entry to CHANGES.rst; whichever is merged second gets a trivial conflict there.

Assisted-by: Claude Opus 5.5

Two threads could both see ``decl_imported`` unset, both load the
declarations, and the second one overwrote the module state, leaking
the first set of references.  This also happens on GIL builds, since
the import and the attribute lookups can release the GIL.  On
free-threaded builds the flag was additionally read and written
without ordering, so a reader could see it set before the fields.

Keep loading without a lock, but publish the result in a critical
section: the first thread stores its references, a later one drops
its own.  Read the flag with acquire and set it with release ordering
on free-threaded builds.

Fixes #391.

Assisted-by: Claude Opus 5.5
raise AttributeError(name)
try:
barrier.wait()
except threading.BrokenBarrierError:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

— Comment created by Claude

If the two threads never meet at the barrier, this except swallows the BrokenBarrierError and the test still passes. That happens when the second thread reaches the barrier more than 5 s late (for example on a loaded CI runner or under coverage), or when a later change serializes the loads. In that case the loads run one after the other, so the test passes even on unfixed code. It no longer catches regressions, and nothing reports that.

Suggestion: count successful barrier.wait() returns and fail the child if fewer than 2 threads got through. If serialized loads should be allowed, print a skip that the parent turns into self.skipTest. Then a run that never raced is reported, not counted as a pass.

_zic_module_state* rec = _zic_state(module);

if (!rec->decl_imported) {
if (!_zic_load_flag(&rec->decl_imported)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

— Comment created by Claude

The new acquire/release ordering assumes that once decl_imported is set, the four fields are valid. _zic_state_clear (the m_clear slot) does not keep that true: it Py_CLEARs builtin_impl_specs, empty, fallback and implements_class but leaves decl_imported at 1.

If anything calls into the module after m_clear, this fast path skips the reload and the callers use NULL fields. For example, a finalizer at interpreter shutdown or GC of a module cycle could call providedBy / implementedBy. Then PyObject_CallFunctionObjArgs(NULL, ...) in implementedByFallback or PyObject_TypeCheck(spec, NULL) crashes. This is not new, but it is the same invariant this PR now depends on. Resetting the flag in _zic_state_clear (rec->decl_imported = 0;) would make the two consistent.

* fields stored before it (issue #391).
*/
#ifdef Py_GIL_DISABLED
#define _zic_load_flag(p) _Py_atomic_load_int_acquire(p)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

— Comment created by Claude

_Py_atomic_load_int_acquire / _Py_atomic_store_int_release are private, underscore-prefixed CPython helpers. They are not covered by the C API stability guarantees and could be renamed or removed in a future release. That would break free-threaded builds only, and CI covers only 3.14t and 3.15t. On 3.13t they were not checked either (the PR tested 3.14t only).

Consider a short comment that states this dependency, or a capability guard with a C11 <stdatomic.h> fallback, so that a header change gives a clear error and not a confusing compile failure on a single build variant.

}
Py_END_CRITICAL_SECTION();

if (published) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

— Comment created by Claude

Optional simplification: the published variable and the second if block can go. Use the idiom already used in _assure_cache_field: inside the critical section, after moving ownership into rec, set the four locals to NULL. After Py_END_CRITICAL_SECTION(), call Py_XDECREF on all four unconditionally. The loser still drops its references outside the lock, and the ownership transfer is visible where it happens.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

copt: free-threading race in _zic_state_load_declarations

2 participants