gh-121368: Fix seq lock memory ordering in _PyType_Lookup (#121388) · python/cpython@1d3cf79 · GitHub
Skip to content

Commit 1d3cf79

Browse files
colesburycolesbury
authored
gh-121368: Fix seq lock memory ordering in _PyType_Lookup (#121388)
The `_PySeqLock_EndRead` function needs an acquire fence to ensure that the load of the sequence happens after any loads within the read side critical section. The missing fence can trigger bugs on macOS arm64. Additionally, we need a release fence in `_PySeqLock_LockWrite` to ensure that the sequence update is visible before any modifications to the cache entry.
1 parent 31873be commit 1d3cf79

9 files changed

Lines changed: 50 additions & 16 deletions

File tree

‎Include/cpython/pyatomic.h‎

Lines changed: 3 additions & 0 deletions

‎Include/cpython/pyatomic_gcc.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -542,6 +542,10 @@ static inline void
542542
_Py_atomic_fence_seq_cst(void)
543543
{ __atomic_thread_fence(__ATOMIC_SEQ_CST); }
544544

545+
static inline void
546+
_Py_atomic_fence_acquire(void)
547+
{ __atomic_thread_fence(__ATOMIC_ACQUIRE); }
548+
545549
static inline void
546550
_Py_atomic_fence_release(void)
547551
{ __atomic_thread_fence(__ATOMIC_RELEASE); }

‎Include/cpython/pyatomic_msc.h‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,6 +1066,18 @@ _Py_atomic_fence_seq_cst(void)
10661066
#else
10671067
# error "no implementation of _Py_atomic_fence_seq_cst"
10681068
#endif
1069+
}
1070+
1071+
static inline void
1072+
_Py_atomic_fence_acquire(void)
1073+
{
1074+
#if defined(_M_ARM64)
1075+
__dmb(_ARM64_BARRIER_ISHLD);
1076+
#elif defined(_M_X64) || defined(_M_IX86)
1077+
_ReadBarrier();
1078+
#else
1079+
# error "no implementation of _Py_atomic_fence_acquire"
1080+
#endif
10691081
}
10701082

10711083
static inline void

‎Include/cpython/pyatomic_std.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -961,6 +961,13 @@ _Py_atomic_fence_seq_cst(void)
961961
atomic_thread_fence(memory_order_seq_cst);
962962
}
963963

964+
static inline void
965+
_Py_atomic_fence_acquire(void)
966+
{
967+
_Py_USING_STD;
968+
atomic_thread_fence(memory_order_acquire);
969+
}
970+
964971
static inline void
965972
_Py_atomic_fence_release(void)
966973
{

‎Include/internal/pycore_lock.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -228,12 +228,12 @@ PyAPI_FUNC(void) _PySeqLock_AbandonWrite(_PySeqLock *seqlock);
228228
PyAPI_FUNC(uint32_t) _PySeqLock_BeginRead(_PySeqLock *seqlock);
229229

230230
// End the read operation and confirm that the sequence number has not changed.
231-
// Returns 1 if the read was successful or 0 if the read should be re-tried.
232-
PyAPI_FUNC(uint32_t) _PySeqLock_EndRead(_PySeqLock *seqlock, uint32_t previous);
231+
// Returns 1 if the read was successful or 0 if the read should be retried.
232+
PyAPI_FUNC(int) _PySeqLock_EndRead(_PySeqLock *seqlock, uint32_t previous);
233233

234234
// Check if the lock was held during a fork and clear the lock. Returns 1
235-
// if the lock was held and any associated datat should be cleared.
236-
PyAPI_FUNC(uint32_t) _PySeqLock_AfterFork(_PySeqLock *seqlock);
235+
// if the lock was held and any associated data should be cleared.
236+
PyAPI_FUNC(int) _PySeqLock_AfterFork(_PySeqLock *seqlock);
237237

238238
#ifdef __cplusplus
239239
}

‎Misc/NEWS.d/next/Core and Builtins/2024-07-04-23-38-30.gh-issue-121368.m3EF9E.rst‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fix race condition in ``_PyType_Lookup`` in the free-threaded build due to
2+
a missing memory fence. This could lead to ``_PyType_Lookup`` returning
3+
incorrect results on arm64.

‎Modules/_testcapi/pyatomic.c‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ test_atomic_fences(PyObject *self, PyObject *obj) {
125125
// Just make sure that the fences compile. We are not
126126
// testing any synchronizing ordering.
127127
_Py_atomic_fence_seq_cst();
128+
_Py_atomic_fence_acquire();
128129
_Py_atomic_fence_release();
129130
Py_RETURN_NONE;
130131
}

‎Objects/typeobject.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5387,7 +5387,7 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name)
53875387
#ifdef Py_GIL_DISABLED
53885388
// synchronize-with other writing threads by doing an acquire load on the sequence
53895389
while (1) {
5390-
int sequence = _PySeqLock_BeginRead(&entry->sequence);
5390+
uint32_t sequence = _PySeqLock_BeginRead(&entry->sequence);
53915391
uint32_t entry_version = _Py_atomic_load_uint32_relaxed(&entry->version);
53925392
uint32_t type_version = _Py_atomic_load_uint32_acquire(&type->tp_version_tag);
53935393
if (entry_version == type_version &&

‎Python/lock.c‎

Lines changed: 15 additions & 11 deletions

0 commit comments

Comments
 (0)