Skip to content

Commit a18e872

Browse files
greateggsgreggreateggsgreg
committed
Atomic GCHandle ownership and finalizer-thread shutdown guards
Two related sets of races that the GIL hid but free-threaded Python exposes reliably. ClassDerivedObject.tp_dealloc and ManagedType.TryFreeGCHandle both read the GCHandle slot, then mutated it. Under FT, subtype_clear (on the main thread) and the .NET finalizer thread can race for the same slot; a non-atomic read-then-zero lets both threads observe the same handle and double-free it. Both paths now use Interlocked.Exchange to atomically claim ownership of the slot - only the thread that observes a non-zero handle frees it. ClassDerived's strong-to-weak swap follows the same pattern. CreateDerivedType emits IL via Reflection.Emit, whose ModuleBuilder and TypeBuilder operations are documented as not thread-safe. Concurrent dynamic subclass creation under FT corrupts the IL stream and segfaults. Serialise the entire emit-and-bake sequence on the existing _buildersLock (the lock that already guarded the assembly / module builder cache). The .NET finalizer thread can dispatch Py_DecRef calls concurrently with Py_Finalize, and a stale read of ob_ref_local after teardown crashes the process. Three guards: - Finalizer.ThrottledCollect and PyObject's finalizer short-circuit when Runtime._Py_IsFinalizing(); PyObject drops the raw pointer instead of enqueueing a decref so process exit reclaims the memory. - Finalizer.AddFinalizedObject's Refcount > 0 Debug.Assert is kept on GIL builds and skipped on FT; a stale ob_ref_local read from the finalizer thread can crash the process even when the assertion would succeed under the GIL. - Runtime.XDecref's matching Refcount > 0 Debug.Assert gets the same FT-only skip for the same reason.
1 parent 5b76676 commit a18e872

5 files changed

Lines changed: 76 additions & 21 deletions

File tree

‎src/runtime/Finalizer.cs‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,10 @@ internal void ThrottledCollect()
115115

116116
_throttled = unchecked(this._throttled + 1);
117117
if (!started || !Enable || _throttled < Threshold) return;
118+
// Skip the drain while Python is finalizing: the .NET-side queue may
119+
// contain references that became stale during teardown, and calling
120+
// Py_DecRef on torn-down state segfaults under free-threaded Python.
121+
if (Runtime._Py_IsFinalizing() == true) return;
118122
_throttled = 0;
119123
this.Collect();
120124
}
@@ -136,7 +140,13 @@ internal void AddFinalizedObject(ref IntPtr obj, int run
136140
return;
137141
}
138142

139-
Debug.Assert(Runtime.Refcount(new BorrowedReference(obj)) > 0);
143+
// Skip the Refcount sanity check under free-threaded Python: the .NET
144+
// finalizer thread can race with Py_Finalize and a stale read of
145+
// ob_ref_local will crash the process. Keep the check on GIL builds.
146+
if (!Native.ABI.IsFreeThreaded)
147+
{
148+
Debug.Assert(Runtime.Refcount(new BorrowedReference(obj)) > 0);
149+
}
140150

141151
#if FINALIZER_CHECK
142152
lock (_queueLock)

‎src/runtime/PythonTypes/PyObject.cs‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,13 +104,24 @@ internal PyObject(in StolenReference reference)
104104
CheckRun();
105105
#endif
106106

107-
Interlocked.Increment(ref Runtime._collected);
107+
// If Python is finalizing we cannot safely enqueue a decref:
108+
// the .NET finalizer thread runs concurrently with Py_Finalize
109+
// and any later Py_DecRef would touch torn-down state. Drop
110+
// the reference and let process exit reclaim the memory.
111+
if (Runtime._Py_IsFinalizing() == true)
112+
{
113+
rawPtr = IntPtr.Zero;
114+
}
115+
else
116+
{
117+
Interlocked.Increment(ref Runtime._collected);
108118

109-
Finalizer.Instance.AddFinalizedObject(ref rawPtr, run
119+
Finalizer.Instance.AddFinalizedObject(ref rawPtr, run
110120
#if TRACE_ALLOC
111-
, Traceback
121+
, Traceback
112122
#endif
113-
);
123+
);
124+
}
114125
}
115126

116127
Dispose(false);

‎src/runtime/Runtime.cs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -607,7 +607,10 @@ internal static unsafe void XIncref(BorrowedReference op)
607607
internal static unsafe void XDecref(StolenReference op)
608608
{
609609
#if DEBUG
610-
Debug.Assert(op == null || Refcount(new BorrowedReference(op.Pointer)) > 0);
610+
// The Refcount > 0 check is racy under free-threaded Python: the .NET
611+
// finalizer thread can dispatch decrefs concurrently with Py_Finalize,
612+
// and a stale read of ob_ref_local can crash the process. Skip on FT.
613+
Debug.Assert(op == null || Native.ABI.IsFreeThreaded || Refcount(new BorrowedReference(op.Pointer)) > 0);
611614
Debug.Assert(_isInitialized || Py_IsInitialized() != 0 || _Py_IsFinalizing() != false);
612615
#endif
613616
if (op == null) return;

‎src/runtime/Types/ClassDerived.cs‎

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ protected override void SetTypeNewSlot(BorrowedReference pyType, SlotsHolder slo
7373
// Python derived types rely on base tp_new and overridden __init__
7474
}
7575

76-
public new static void tp_dealloc(NewReference ob)
76+
public new static unsafe void tp_dealloc(NewReference ob)
7777
{
7878
var self = (CLRObject?)GetManagedObject(ob.Borrow());
7979

@@ -83,14 +83,26 @@ protected override void SetTypeNewSlot(BorrowedReference pyType, SlotsHolder slo
8383
// self may be null after Shutdown begun
8484
if (self is not null)
8585
{
86-
// The python should now have a ref count of 0, but we don't actually want to
87-
// deallocate the object until the C# object that references it is destroyed.
88-
// So we don't call PyObject_GC_Del here and instead we set the python
89-
// reference to a weak reference so that the C# object can be collected.
90-
GCHandle oldHandle = GetGCHandle(ob.Borrow());
91-
GCHandle gc = GCHandle.Alloc(self, GCHandleType.Weak);
92-
SetGCHandle(ob.Borrow(), gc);
93-
oldHandle.Free();
86+
// Replace the strong handle with a weak one so the C# wrapper can be
87+
// collected, but the Python object survives until it does.
88+
//
89+
// Under free-threaded Python a concurrent tp_dealloc/tp_clear path
90+
// could race with us; do the swap atomically and only free the old
91+
// handle if we were the thread that observed it.
92+
GCHandle weak = GCHandle.Alloc(self, GCHandleType.Weak);
93+
BorrowedReference borrow = ob.Borrow();
94+
int offset = Util.ReadInt32(Runtime.PyObject_TYPE(borrow), Offsets.tp_clr_inst_offset);
95+
IntPtr* slot = (IntPtr*)(borrow.DangerousGetAddress() + offset);
96+
IntPtr oldRaw = System.Threading.Interlocked.Exchange(ref *slot, (IntPtr)weak);
97+
if (oldRaw != IntPtr.Zero)
98+
{
99+
((GCHandle)oldRaw).Free();
100+
}
101+
else
102+
{
103+
// Lost the race; another thread already cleared the slot.
104+
weak.Free();
105+
}
94106
}
95107
}
96108

@@ -165,6 +177,23 @@ internal static Type CreateDerivedType(string name,
165177
assemblyName = "Python.Runtime.Dynamic";
166178
}
167179

180+
// Reflection.Emit's ModuleBuilder/TypeBuilder operations are not
181+
// thread-safe; concurrent DefineType calls on the same module
182+
// corrupt the IL stream and segfault under free-threaded Python.
183+
// Serialise the entire emit-and-bake sequence on _buildersLock.
184+
lock (_buildersLock)
185+
{
186+
return CreateDerivedTypeImpl(name, baseType, typeInterfaces, py_dict, assemblyName, moduleName);
187+
}
188+
}
189+
190+
private static Type CreateDerivedTypeImpl(string name,
191+
Type baseType,
192+
IList<Type> typeInterfaces,
193+
BorrowedReference py_dict,
194+
string assemblyName,
195+
string moduleName)
196+
{
168197
ModuleBuilder moduleBuilder = GetModuleBuilder(assemblyName, moduleName);
169198

170199
Type baseClass = baseType;

‎src/runtime/Types/ManagedType.cs‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ internal static void SetGCHandle(BorrowedReference reflectedClrObject, GCHandle
230230
internal static bool TryFreeGCHandle(BorrowedReference reflectedClrObject)
231231
=> TryFreeGCHandle(reflectedClrObject, Runtime.PyObject_TYPE(reflectedClrObject));
232232

233-
internal static bool TryFreeGCHandle(BorrowedReference reflectedClrObject, BorrowedReference type)
233+
internal static unsafe bool TryFreeGCHandle(BorrowedReference reflectedClrObject, BorrowedReference type)
234234
{
235235
Debug.Assert(type != null);
236236
Debug.Assert(reflectedClrObject != null);
@@ -240,13 +240,15 @@ internal static bool TryFreeGCHandle(BorrowedReference reflectedClrObject, Borro
240240
int offset = Util.ReadInt32(type, Offsets.tp_clr_inst_offset);
241241
Debug.Assert(offset > 0);
242242

243-
IntPtr raw = Util.ReadIntPtr(reflectedClrObject, offset);
243+
// Atomic claim of the GCHandle: under free-threaded Python tp_clear
244+
// and tp_dealloc can race against each other (e.g. main thread vs
245+
// .NET finalizer thread). A non-atomic read-then-zero would let
246+
// both threads see the same handle and double-free it.
247+
IntPtr* slot = (IntPtr*)(reflectedClrObject.DangerousGetAddress() + offset);
248+
IntPtr raw = System.Threading.Interlocked.Exchange(ref *slot, IntPtr.Zero);
244249
if (raw == IntPtr.Zero) return false;
245250

246-
var handle = (GCHandle)raw;
247-
handle.Free();
248-
249-
Util.WriteIntPtr(reflectedClrObject, offset, IntPtr.Zero);
251+
((GCHandle)raw).Free();
250252
return true;
251253
}
252254

0 commit comments

Comments
 (0)