Skip to content

Commit 085b660

Browse files
authored
fix: data race on last_storage_ptr_ cache in gil_safe_call_once_and_store (#6087)
Under free-threaded CPython (Py_GIL_DISABLED) the GIL provides no mutual exclusion, so the plain pointer last_storage_ptr_ was read and written concurrently without synchronization, a C++ data race. Make it a std::atomic<T *>. Also reorder get_stored() to check is_last_storage_valid() before loading the cached pointer. The writer publishes the pointer before setting the validity flag, so the flag must be observed first for correct acquire/release ordering. Assisted-by: ClaudeCode:claude-fable-5
1 parent 59d7cb2 commit 085b660

1 file changed

Lines changed: 14 additions & 4 deletions

File tree

include/pybind11/gil_safe_call_once.h

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,8 @@ class gil_safe_call_once_and_store {
193193
::new (value->storage) T(fn());
194194
value->finalize = finalize_fn;
195195
value->is_initialized = true;
196+
// Publish the cached pointer before setting the validity flag, so that any reader
197+
// which observes the flag as true is guaranteed to also observe this pointer.
196198
last_storage_ptr_ = reinterpret_cast<T *>(value->storage);
197199
is_initialized_by_at_least_one_interpreter_ = true;
198200
});
@@ -204,11 +206,17 @@ class gil_safe_call_once_and_store {
204206

205207
// This must only be called after `call_once_and_store_result()` was called.
206208
T &get_stored() {
207-
T *result = last_storage_ptr_;
208-
if (!is_last_storage_valid()) {
209+
T *result = nullptr;
210+
// Check the validity flag *before* loading the cached pointer: the writer publishes the
211+
// pointer before setting the flag, so observing the flag as true guarantees the load below
212+
// sees the published pointer.
213+
if (is_last_storage_valid()) {
214+
result = last_storage_ptr_;
215+
} else {
209216
gil_scoped_acquire gil_acq;
210217
auto *value = get_or_create_storage_in_state_dict();
211-
result = last_storage_ptr_ = reinterpret_cast<T *>(value->storage);
218+
result = reinterpret_cast<T *>(value->storage);
219+
last_storage_ptr_ = result;
212220
}
213221
assert(result != nullptr);
214222
return *result;
@@ -260,10 +268,12 @@ class gil_safe_call_once_and_store {
260268

261269
// Fast local cache to avoid repeated lookups when there are no multiple interpreters.
262270
// This is only valid if there is a single interpreter. Otherwise, it is not used.
271+
// It is atomic because under free-threading (`Py_GIL_DISABLED`) the GIL provides no mutual
272+
// exclusion, so this pointer may be read and written concurrently by multiple threads.
263273
// WARNING: We cannot use thread local cache similar to `internals_pp_manager::internals_p_tls`
264274
// because the thread local storage cannot be explicitly invalidated when interpreters
265275
// are destroyed (unlike `internals_pp_manager` which has explicit hooks for that).
266-
T *last_storage_ptr_ = nullptr;
276+
std::atomic<T *> last_storage_ptr_{nullptr};
267277
// This flag is true if the value has been initialized by any interpreter (may not be the
268278
// current one).
269279
detail::atomic_bool is_initialized_by_at_least_one_interpreter_{false};

0 commit comments

Comments
 (0)