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

[rolfbjarne]

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 SIGSEGV when creating invalid AVAssetWriterInput #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

[Copilot]

Pull request overview

This pull request updates the managed runtime’s NSObject registration logic to prevent object_map corruption when a native init returns a different handle than the one produced by alloc, addressing the alloc/init handle-reuse race behind issues #9478 / #23679 / #25861.

Changes:

• Defer object_map registration for user types until after init completes (with an AppContext legacy switch to restore previous behavior).
• Make handle rebind unregistration ownership-aware to avoid removing entries for a different object that reused a freed pointer.
• Add an ivar-based fallback in Runtime.GetNSObject* to resolve user types during init before they’re in object_map, plus new regression/probe tests.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/test-libraries/libtest.m	Adds native helpers to deterministically reproduce pointer reuse and init-time callbacks.
tests/test-libraries/libtest.h	Exposes new native callback APIs and helper class declarations for tests.
tests/monotouch-test/ObjCRuntime/InitCallbackProbeTest.cs	Adds probe tests to ensure init-time callbacks resolve to the correct wrapper instance.
tests/monotouch-test/ObjCRuntime/AllocInitRaceTest.cs	Adds deterministic and stress reproductions for the alloc/init handle-reuse race and a failed-init + GC regression.
tests/bindings-test/ApiDefinition.cs	Binds the new native helper types so tests can instantiate them from managed code.
src/ObjCRuntime/Runtime.cs	Adds the legacy switch, ownership-aware unregister, deferred registration helper, and ivar fallback lookup.
src/Foundation/NSObject2.cs	Implements deferred registration for user types, ownership-aware handle unregistration, and ensures gchandle ivar exists on the final handle when init returns a different pointer.

[rolfbjarne]

/review

[github-actions]

✅ .NET for Apple Platforms PR Reviewer completed successfully!

[github-actions]

Review: ⚠️ Needs Changes

The overall approach is well thought-out and the PR description gives an excellent walk-through of the race. Tests are thorough. Two issues found:

Severity	Location	Summary
❌ error	Runtime.cs RegisterNSObjectIfNeeded	TOCTOU: lock released between ContainsKey and RegisterNSObject, so a concurrent registration can be silently overwritten
⚠️ warning	NSObject2.cs EnsureManagedReference	HasManagedRef = true is set before confirming xamarin_set_gchandle_with_flags_safe succeeds; if it returns 0, HasManagedRef is left stale, which can corrupt reference counting on disposal

Everything else looks correct — the ownership-aware UnregisterNSObject (ptr, this), the deferred-registration logic, the ivar fallback in GetNSObject/GetNSObject<T>, the AppContext legacy switch, and the deterministic test setup are all clean.

Generated by .NET for Apple Platforms PR Reviewer for issue #26259 · 71.1 AIC · ⌖ 5.73 AIC · ⊞ 5.1K
_{Comment /review to run again}

[github-actions]

❌ Failed to apply gist. Check the workflow run for details.

[rolfbjarne]

/review

[github-actions]

✅ .NET for Apple Platforms PR Reviewer completed successfully!

[github-actions]

Code Review — [runtime] Fix the alloc/init handle-reuse race in object_map

⚠️ Needs attention before merging — CI is failing.

The xamarin-macios-sim-pr-tests run has two failing jobs:

• Simulator tests T: monotouch_ios
• M1 - Mac Ventura (13) macOS tests

These need to be investigated and resolved before the PR can land.

---

Overall assessment

The fix is conceptually sound and well-described. The three-part strategy — deferred object_map registration for user types, ownership-aware UnregisterNSObject, and an ivar-fallback in GetNSObject — correctly addresses the race condition and the test suite provides deterministic reproduction of the original bug (and a regression guard for the legacy switch). The code is well-commented.

Issues found

Severity	Location	Summary
⚠️	src/Foundation/NSObject2.cs:574	Retain imbalance in EnsureManagedReference early-exit path — HasManagedRef left false while this still owns a +1 from init
💡	src/ObjCRuntime/Runtime.cs:1241	Speculative GCHandle allocation before lock check when onlyIfNeeded = true

Positive callouts

• The ownership-aware UnregisterNSObject (IntPtr, NSObject) overload mirrors the check that NativeObjectHasDied already had — a clean, consistent pattern.
• The onlyIfNeeded default argument on RegisterNSObject keeps all existing call sites unchanged.
• The ReusedAddressClobberedWithLegacySwitch test is particularly valuable: it documents the old (buggy) behavior and proves the test scenario genuinely exercises the changed code path.
• The TryGetNSObjectFromIvar safety comment (explaining why it must not be folded into TryGetNSObject) is clear and important for future maintainers.

Generated by .NET for Apple Platforms PR Reviewer for issue #26259 · 102 AIC · ⌖ 5.78 AIC · ⊞ 5.1K
_{Comment /review to run again}

Comments that could not be inline-anchored

src/Foundation/NSObject2.cs:574

🤖 ⚠️ Memory management — When GetGCHandleForObject (newHandle) != IntPtr.Zero this method returns early without setting HasManagedRef = true. But this still holds the +1 retain on newHandle that init transferred to it. ReleaseManagedRef checks HasManagedRef to decide whether to release; with it left false (it was cleared when the alloc'd address was dealloced) that retain leaks permanently.

The rv == 0 path lower down explicitly keeps HasManagedRef = true for exactly t…

src/ObjCRuntime/Runtime.cs:1241

🤖 💡 Performance — With onlyIfNeeded = true, a GCHandle is allocated (or a tracking handle is created for CoreCLR) before the lock check. If the key is already present this allocation is immediately freed. A non-locking pre-check with object_map.ContainsKey (ptr) (accepted as a best-effort hint, with the authoritative check remaining inside the lock) would avoid the allocation in the common case where the object is already registered.

_Rule: Performance — avoid unnecessary alloc…

[rolfbjarne]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).

[vs-mobiletools-engineering-service2]

✅ API diff for current PR / commit

NET (empty diffs)

• iOS: vsdrops gist ( No breaking changes )
• tvOS: vsdrops gist ( No breaking changes )
• MacCatalyst: vsdrops gist ( No breaking changes )
• macOS: vsdrops gist ( No breaking changes )

✅ API diff vs stable

NET (empty diffs)

• iOS: vsdrops gist ( No breaking changes )
• tvOS: vsdrops gist ( No breaking changes )
• MacCatalyst: vsdrops gist ( No breaking changes )
• macOS: vsdrops gist ( No breaking changes )

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: 58d0fc31e8c4868b962302b21709831b87161dbd [PR build]

[vs-mobiletools-engineering-service2]

🚀 [CI Build #58d0fc3] Test results 🚀

Test results

✅ All tests passed on VSTS: test results.

🎉 All 204 tests passed 🎉

Tests counts

✅ assembly-processing: All 1 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (iOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (MacCatalyst): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (macOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (Multiple platforms): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (tvOS): All 1 tests passed. Html Report (VSDrops) Download
✅ framework: All 2 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 4 tests passed. Html Report (VSDrops) Download
✅ generator: All 5 tests passed. Html Report (VSDrops) Download
✅ interdependent-binding-projects: All 4 tests passed. Html Report (VSDrops) Download
✅ introspection: All 4 tests passed. Html Report (VSDrops) Download
✅ linker (iOS): All 15 tests passed. Html Report (VSDrops) Download
✅ linker (MacCatalyst): All 15 tests passed. Html Report (VSDrops) Download
✅ linker (macOS): All 21 tests passed. Html Report (VSDrops) Download
✅ linker (tvOS): All 15 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 19 tests passed. Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 18 tests passed. Html Report (VSDrops) Download
✅ monotouch (macOS): All 20 tests passed. Html Report (VSDrops) Download
✅ monotouch (tvOS): All 19 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ sharpie: All 1 tests passed. Html Report (VSDrops) Download
✅ windows: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 4 tests passed. Html Report (VSDrops) Download
✅ xtro: All 1 tests passed. Html Report (VSDrops) Download

macOS tests

✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download
✅ Tests on macOS Ventura (13): All 5 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ Tests on macOS Sonoma (14): All 5 tests passed. Html Report (VSDrops) Download
✅ Tests on macOS Sequoia (15): All 5 tests passed. Html Report (VSDrops) Download
✅ Tests on macOS Tahoe (26): All 5 tests passed. Html Report (VSDrops) Download

Linux Build Verification

✅ Linux build succeeded

Pipeline on Agent
Hash: 58d0fc31e8c4868b962302b21709831b87161dbd [PR build]