[runtime] Fix the alloc/init handle-reuse race in object_map (issue #25861) (#26259)

Scenario:

An object whose native `init` returns a different pointer than `alloc` (e.g. `CKRecordZoneID`) frees its alloc'd address. If another object (e.g. a `__MonoMac_NSAsyncActionDispatcher`) is allocated at that just-freed address on another thread and registered in `object_map`, the first object's `Handle` setter would then unconditionally `UnregisterNSObject` its stale alloc handle and clobber the second object's registration. A later native->managed marshal of that address then fails ("Could not find an existing managed instance", errors 8027/8034/8035).

Fix:

* Defer `object_map` registration until after `init` for user types. User types carry their gchandle in a native ivar (set at alloc time), which is self-cleaning when the address is freed/reused, so it's a safe authoritative fallback lookup during `init`. Only the final (post-`init`) handle is added to `object_map`. Direct bindings have no ivar, so they stay eagerly registered and are protected by the ownership-aware unregister below.
* Use an ownership-aware `UnregisterNSObject (handle, this)` in the `Handle` setter, which only removes the `object_map` entry if it still refers to `this` (mirroring the check `NativeObjectHasDied` already had).
* Add a native->managed ivar fallback to `Runtime.GetNSObject`/`GetNSObject<T>` so a user type whose object_map registration was deferred can still be resolved during `init`.
* Gate both behaviors behind a legacy `AppContext` switch (`ObjCRuntime.Runtime.RegisterObjectsBeforeInit`, default off) to restore the previous behavior if any existing binding relied on it.

Tests:

* AllocInitRaceTest deterministically reproduces the clobber with a tiny custom native allocator (ReuseSlotClassA/ReuseSlotClassB): one class' `init` frees its instance and forces the next allocation to reuse that exact address. ReusedAddressSurvivesAllocInitClobber verifies the reused address still resolves to the correct object; ReusedAddressClobberedWithLegacySwitch documents the pre-fix behavior with the legacy switch on.
* InitCallbackProbeTest exercises surfacing `self` to managed code during `init`.
* A failed-init + GC guard (InitReturnsNilClass) covers the #23679 shape (a native `init` that raises an Objective-C exception, followed by a forced GC).

Fixes #9478.
Fixes #23679.
Fixes #25861.

🤖 Pull request created by Copilot

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Rolf Bjarne Kvinge <rokvin@microsoft.com>

Author: rolfbjarne

src/Foundation/NSObject2.cs

@@ -329,6 +329,11 @@ public NSObject ()
 		{
 			bool alloced = AllocIfNeeded ();
 			InitializeObject (alloced);
+			// This constructor doesn't send 'init', so the handle is final. Complete any
+			// deferred registration for user types (see #25861); no-op if already registered
+			// (e.g. direct bindings, which InitializeObject registers eagerly).
+			if (alloced && !Runtime.RegisterObjectsBeforeInit)
+				Runtime.RegisterNSObject (this, handle, onlyIfNeeded: true);
 		}
 
 		// This is just here as a constructor chain that can will
@@ -509,20 +514,35 @@ private void InitializeObject (bool alloced)
 			// and any subclasses in the platform assembly which is not a direct binding have
 			// to set the correct value in their constructors.
 			IsDirectBinding = (this.GetType ().Assembly == PlatformAssembly);
-			Runtime.RegisterNSObject (this, handle);
 
 			bool native_ref = (flags & Flags.NativeRef) == Flags.NativeRef;
-			CreateManagedRef (!alloced || native_ref);
+
+			if (!Runtime.TryGetIsUserType (handle, out var isUserType, out var error_message))
+				throw new InvalidOperationException ($"Unable to create a managed reference for the pointer {handle} whose managed type is {GetType ().FullName} because it wasn't possible to get the class of the pointer: {error_message}");
+
+			// Issue #25861: when we've just alloc'd a user type, defer adding it to the
+			// object_map until 'init' has completed. A native 'init' may free this handle
+			// and return a different one; we don't want a pointer to freed memory lingering
+			// in the map. User types carry their gchandle in a native ivar (set by
+			// CreateManagedRef below), which serves as a fallback lookup during 'init', so
+			// deferring their object_map registration is safe. The final handle is registered
+			// later (via InitializeHandle for the generated alloc+init constructors, or right
+			// after this call for the parameterless NSObject constructor which doesn't send
+			// 'init'). Direct bindings have no ivar, so they must remain registered throughout
+			// 'init' (e.g. so a native 'init' that surfaces 'self' to managed code resolves to
+			// the wrapper being constructed) and are registered eagerly here.
+			if (!alloced || !isUserType || Runtime.RegisterObjectsBeforeInit)
+				Runtime.RegisterNSObject (this, handle);
+
+			CreateManagedRef (isUserType, !alloced || native_ref);
 		}
 
 		[DllImport ("__Internal")]
 		static extern byte xamarin_set_gchandle_with_flags_safe (IntPtr handle, IntPtr gchandle, XamarinGCHandleFlags gchandle_flags, IntPtr data);
 
-		void CreateManagedRef (bool retain)
+		void CreateManagedRef (bool isUserType, bool retain)
 		{
 			HasManagedRef = true;
-			if (!Runtime.TryGetIsUserType (handle, out var isUserType, out var error_message))
-				throw new InvalidOperationException ($"Unable to create a managed reference for the pointer {handle} whose managed type is {GetType ().FullName} because it wasn't possible to get the class of the pointer: {error_message}");
 
 			if (isUserType) {
 				var gchandle_flags = XamarinGCHandleFlags.HasManagedRef | XamarinGCHandleFlags.InitialSet;
@@ -543,6 +563,37 @@ void CreateManagedRef (bool retain)
 				DangerousRetain ();
 		}
 
+		// Issue #25861: if 'init' returned a different handle than 'alloc' for a user type,
+		// the gchandle ivar was set on the (now typically freed) alloc'd handle. Make sure
+		// the final handle also has a gchandle ivar pointing back at this managed object, so
+		// it can be resolved native->managed. Does nothing for direct bindings (no ivar) or
+		// if the ivar is already set.
+		void EnsureManagedReference (NativeHandle newHandle)
+		{
+			if (!Runtime.TryGetIsUserType (newHandle, out var isUserType, out var _) || !isUserType)
+				return;
+			if (Runtime.GetGCHandleForObject (newHandle) != IntPtr.Zero)
+				return;
+			HasManagedRef = true;
+			var gchandle_flags = XamarinGCHandleFlags.HasManagedRef | XamarinGCHandleFlags.InitialSet;
+			var gchandle = GCHandle.Alloc (this, GCHandleType.WeakTrackResurrection);
+			var h = GCHandle.ToIntPtr (gchandle);
+			byte rv;
+			unsafe {
+				rv = xamarin_set_gchandle_with_flags_safe (newHandle, h, gchandle_flags, (IntPtr) GetData ());
+			}
+			if (rv == 0) {
+				// The ivar slot was already claimed (e.g. another managed wrapper won a race
+				// to represent this native object). Free the gchandle we allocated. We keep
+				// HasManagedRef set (it was already set by CreateManagedRef): this object
+				// still owns the +1 that 'init' transferred to 'newHandle', and that +1 must
+				// still be released via ReleaseManagedRef on disposal. This mirrors the same
+				// case in CreateManagedRef.
+				Runtime.NSLog ($"Tried to create a managed reference from an object that already has a managed reference (type: {GetType ()})");
+				gchandle.Free ();
+			}
+		}
+
 		void ReleaseManagedRef ()
 		{
 			var handle = this.Handle; // Get a copy of the handle, because it will be cleared out when calling Runtime.NativeObjectHasDied, and we still need the handle later.
@@ -800,8 +851,15 @@ public NativeHandle Handle {
 				if (handle == value)
 					return;
 
-				if (handle != IntPtr.Zero)
-					Runtime.UnregisterNSObject (handle);
+				if (handle != IntPtr.Zero) {
+					// Issue #25861: use the ownership-aware unregister so we don't remove an
+					// object_map entry that another object created after reusing this (freed)
+					// address. The legacy switch restores the previous unconditional removal.
+					if (Runtime.RegisterObjectsBeforeInit)
+						Runtime.UnregisterNSObject (handle);
+					else
+						Runtime.UnregisterNSObject (handle, this);
+				}
 
 				handle = value;
 
@@ -843,7 +901,24 @@ protected internal void InitializeHandle (NativeHandle handle, string initSelect
 				throw new Exception ($"Could not initialize an instance of the type '{GetType ().FullName}': the native '{initSelector}' method returned nil.\n{Constants.SetThrowOnInitFailureToFalse}.");
 			}
 
+			// Transition to the final (post-'init') handle. The Handle setter (ownership-aware)
+			// unregisters the previous handle if needed and registers the new one.
+			var previousHandle = this.handle;
 			this.Handle = handle;
+
+			// Issue #25861: registration for user types was deferred in InitializeObject.
+			if (!Runtime.RegisterObjectsBeforeInit && handle != NativeHandle.Zero) {
+				if (handle == previousHandle) {
+					// 'init' returned the same handle, so the setter above was a no-op.
+					// Complete the deferred registration now (no-op if already registered).
+					Runtime.RegisterNSObject (this, handle, onlyIfNeeded: true);
+				} else {
+					// 'init' returned a different handle; the gchandle ivar was set on the
+					// previous (now typically freed) handle, so re-establish it on the final
+					// handle for user types.
+					EnsureManagedReference (handle);
+				}
+			}
 		}
 
 		private bool AllocIfNeeded ()

src/ObjCRuntime/Runtime.cs

@@ -290,6 +290,14 @@ public static bool DynamicRegistrationSupported {
 		[BindingImpl (BindingImplOptions.Optimizable)]
 		internal static bool UseCFNetworkHandler => AppContext.TryGetSwitch ("System.Net.Http.NativeHandler.UseCFNetworkHandler", out bool isDefault) ? isDefault : false;
 
+		// Issue #25861: when false (the default), NSObjects created via alloc/init are
+		// added to the object_map only after 'init' has completed. This way a native
+		// 'init' that frees the alloc'd handle (and returns a different one) can't leave
+		// a stale pointer to freed memory in the map, which could later be clobbered when
+		// the memory is reused by another object. Set this AppContext switch to true to
+		// restore the previous behavior (register the object right after 'alloc').
+		internal static bool RegisterObjectsBeforeInit => AppContext.TryGetSwitch ("ObjCRuntime.Runtime.RegisterObjectsBeforeInit", out var value) && value;
+
 		// The linker may turn calls to this property into a constant
 		/// <summary>Determines whether the debug builds will enforce that calls done to AppKit/UIKit APIs are only issued from the UI thread.</summary>
 		/// <remarks>
@@ -1204,6 +1212,21 @@ internal static void UnregisterNSObject (IntPtr ptr)
 			}
 		}
 
+		// Ownership-aware variant of UnregisterNSObject: only removes the entry if it
+		// still refers to `managed` (or is dead). This avoids clobbering an entry that
+		// another object created after reusing a freed native pointer (issue #25861).
+		internal static void UnregisterNSObject (IntPtr ptr, NSObject managed)
+		{
+			lock (lock_obj) {
+				if (object_map.TryGetValue (ptr, out var value)) {
+					if (value.Target is null || object.ReferenceEquals (value.Target, managed)) {
+						object_map.Remove (ptr);
+						value.Free ();
+					}
+				}
+			}
+		}
+
 		internal static void NativeObjectHasDied (IntPtr ptr, NSObject? managed_obj)
 		{
 			lock (lock_obj) {
@@ -1224,7 +1247,12 @@ internal static void NativeObjectHasDied (IntPtr ptr, NSObject? managed_obj)
 			}
 		}
 
-		internal static void RegisterNSObject (NSObject obj, IntPtr ptr)
+		// Completes deferred object_map registration (issue #25861): when 'onlyIfNeeded' is
+		// true, registers the object only if the pointer isn't already present, and leaves
+		// any existing entry untouched. This avoids redundantly re-registering objects that
+		// were registered eagerly (e.g. direct bindings), and avoids clobbering a concurrent
+		// registration (e.g. another object reusing a freed native pointer).
+		internal static void RegisterNSObject (NSObject obj, IntPtr ptr, bool onlyIfNeeded = false)
 		{
 			GCHandle handle;
 			if (Runtime.IsCoreCLR) {
@@ -1234,8 +1262,17 @@ internal static void RegisterNSObject (NSObject obj, IntPtr ptr)
 			}
 
 			lock (lock_obj) {
-				if (object_map.Remove (ptr, out var existing))
-					existing.Free ();
+				if (onlyIfNeeded) {
+					if (object_map.ContainsKey (ptr)) {
+						// Already registered; don't touch the existing entry, just free the
+						// handle we speculatively allocated.
+						handle.Free ();
+						return;
+					}
+				} else {
+					if (object_map.Remove (ptr, out var existing))
+						existing.Free ();
+				}
 				object_map [ptr] = handle;
 #pragma warning disable RBI0014
 				obj.Handle = ptr;
@@ -1865,6 +1902,12 @@ internal static bool RemoveFromObjectMap (NSObject obj)
 
 			var o = TryGetNSObject (ptr, evenInFinalizerQueue);
 
+			// Fallback for issue #25861: a user type still executing its own 'init' isn't in
+			// the object_map yet (deferred until 'init' completes), but carries its gchandle
+			// in a native ivar. Safe here because GetNSObject is only called for Objective-C
+			// objects. See TryGetNSObjectFromIvar for why this can't live in TryGetNSObject.
+			o ??= TryGetNSObjectFromIvar (ptr, evenInFinalizerQueue);
+
 			if (o is not null) {
 				if (owns)
 					o.DangerousRelease ();
@@ -1921,6 +1964,11 @@ internal static bool RemoveFromObjectMap (NSObject obj)
 
 			var obj = TryGetNSObject (ptr, evenInFinalizerQueue: evenInFinalizerQueue);
 
+			// Fallback for issue #25861: resolve a user type still executing its own 'init'
+			// (not yet in the object_map) via its native gchandle ivar. See the non-generic
+			// GetNSObject for why this is safe here.
+			obj ??= TryGetNSObjectFromIvar (ptr, evenInFinalizerQueue);
+
 			// First check if we got an object of the expected type
 			if (obj is T o)
 				return o;
@@ -2794,6 +2842,44 @@ static bool GetIsARM64CallingConvention ()
 			return GCHandle.FromIntPtr (ptr).Target;
 		}
 
+		// Returns the gchandle stored in the native object's gchandle ivar (user types
+		// only; INVALID_GCHANDLE/zero otherwise).
+		[DllImport ("__Internal")]
+		static extern IntPtr xamarin_get_gchandle (IntPtr obj);
+
+		internal static IntPtr GetGCHandleForObject (IntPtr ptr)
+		{
+			return xamarin_get_gchandle (ptr);
+		}
+
+		// Fallback lookup for issue #25861: a user-type object that hasn't been added to
+		// the object_map yet (e.g. it's still executing its own 'init') can still be
+		// resolved via the gchandle stored in its native ivar. Returns null for direct
+		// bindings (which have no ivar) and for objects without a managed reference.
+		internal static NSObject? TryGetNSObjectFromIvar (IntPtr ptr)
+		{
+			var gchandle = xamarin_get_gchandle (ptr);
+			if (gchandle == IntPtr.Zero)
+				return null;
+			return GetGCHandleTarget (gchandle) as NSObject;
+		}
+
+		// Guarded ivar fallback for issue #25861: resolve a user type that's still executing
+		// its own 'init' (and so isn't in the object_map yet) via its native gchandle ivar,
+		// only returning it if it still owns 'ptr' and isn't queued for finalization (unless
+		// asked otherwise). MUST only be called with an Objective-C object pointer: it sends
+		// the xamarinGetGCHandle message, which crashes on non-Objective-C native handles.
+		// That's why this isn't folded into the general TryGetNSObject (which can be called
+		// with arbitrary native handles, e.g. from GetINativeObject); it's only safe from
+		// GetNSObject/GetNSObject<T>, which are only ever called for Objective-C objects.
+		static NSObject? TryGetNSObjectFromIvar (IntPtr ptr, bool evenInFinalizerQueue)
+		{
+			var fromIvar = TryGetNSObjectFromIvar (ptr);
+			if (fromIvar is not null && fromIvar.Handle == ptr && (evenInFinalizerQueue || !fromIvar.InFinalizerQueue))
+				return fromIvar;
+			return null;
+		}
+
 		// Allocate a GCHandle and return the IntPtr to it.
 		internal static IntPtr AllocGCHandle (object? value)
 		{

tests/bindings-test/ApiDefinition.cs

@@ -886,4 +886,38 @@ interface Hitchhiker {
 		[Export ("buildHighway")]
 		void BuildHighway ();
 	}
+
+	// Helper for the issue #25861 design work: its native 'init' calls a method that
+	// can be overridden in managed code, to verify the overridden managed method is
+	// invoked (on the correct instance) while 'init' is still executing.
+	[BaseType (typeof (NSObject))]
+	interface InitCallsVirtualMethod {
+		[Export ("virtualMethodCalledDuringInit:")]
+		void VirtualMethodCalledDuringInit (int value);
+	}
+
+	// Helper for the issue #25861 design work: a directly-bound native class whose
+	// native 'init' synchronously surfaces 'self' to managed code via a C callback.
+	[BaseType (typeof (NSObject))]
+	interface InitSurfacesSelfToManaged {
+	}
+
+	// Helpers for a deterministic reproduction of the alloc/init handle-reuse race in
+	// issue #25861 (and #9478): ReuseSlotClassA's 'init' frees its own instance and forces
+	// the next allocation to reuse that exact address, so a ReuseSlotClassB allocated from
+	// managed code during that 'init' deterministically lands on the address freed by the
+	// ReuseSlotClassA instance.
+	[BaseType (typeof (NSObject))]
+	interface ReuseSlotClassA {
+	}
+
+	[BaseType (typeof (NSObject))]
+	interface ReuseSlotClassB {
+	}
+
+	// Helper for issue #23679: a native class whose 'init' fails (releases self and
+	// returns nil), so constructing the managed wrapper throws.
+	[BaseType (typeof (NSObject))]
+	interface InitReturnsNilClass {
+	}
 }