[runtime] Fix a race condition when switching between weak and strong GCHandles. Fixes #24702.#24841
Draft
rolfbjarne wants to merge 4 commits intomainfrom
Draft
[runtime] Fix a race condition when switching between weak and strong GCHandles. Fixes #24702.#24841rolfbjarne wants to merge 4 commits intomainfrom
rolfbjarne wants to merge 4 commits intomainfrom
Conversation
Add a test that reproduces the race condition described in #24702: one thread switches an object's gchandle between weak and strong (via retain/release), while another thread fetches the gchandle (by calling an exported ObjC method). This can cause a crash when the fetching thread reads a gchandle that has been freed by the switching thread. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Instead of replacing a single gchandle between weak and strong (which races with concurrent readers), use a dual-gchandle approach: - A permanent weak gchandle is used for all lookups. This handle is created once and never modified, eliminating the race with readers. - A separate strong gchandle is created/freed as needed to keep the managed object alive. This handle is stored in a global hash table and is never accessed by the reader path. The one-time conversion from the initial strong handle to the weak lookup handle happens during object initialization (from RegisterToggleReferenceCoreCLR), before concurrent access is possible. Fixes #24702 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace get/set_strong_gchandle with: - clear_strong_gchandle: atomically looks up, removes, and frees the strong gchandle within a single lock acquisition. - set_strong_gchandle: creates the gchandle before the lock, then atomically checks if one already exists. If so, frees the newly created one; otherwise stores it. This eliminates TOCTOU races where two threads could both see no strong gchandle and both try to set one, or where a gchandle could be freed between lookup and removal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The initial GCHandle stored on native objects is always a weak handle (GCHandleType.WeakTrackResurrection), created in CreateManagedRef in NSObject2.cs. Since it's always weak, the one-time strong-to-weak conversion branch in xamarin_switch_gchandle is dead code. This simplifies xamarin_switch_gchandle to just manage the strong gchandle in the hash table without any flag checking, and removes the XamarinGCHandleFlags_WeakGCHandle flag from both the native enum and the managed enum since it is no longer used. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes the runtime’s GCHandle switching mechanism to avoid a race when transitioning between weak/strong handles for user types, and adds a regression test to exercise the concurrency scenario reported in #24702.
Changes:
- Reworks
xamarin_switch_gchandleto keep the per-objectgc_handlestable (weak) and manage strong handles separately in a global map. - Removes the
WeakGCHandlebit from the managed/nativeXamarinGCHandleFlagsenums. - Adds
GCHandleSwitchRaceTestto reproduce the retain/release vs exported-method lookup race.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/monotouch-test/ObjCRuntime/GCHandleSwitchRaceTest.cs | New test that races retain/release against an exported ObjC method call to catch the GCHandle switching race. |
| src/Foundation/NSObject2.cs | Removes WeakGCHandle flag usage when creating managed refs for user types. |
| runtime/xamarin/trampolines.h | Keeps native XamarinGCHandleFlags enum in sync with managed changes (removes WeakGCHandle). |
| runtime/runtime.m | Implements the dual-GCHandle scheme using a global strong-handle dictionary and updates xamarin_switch_gchandle/cleanup logic. |
Comments suppressed due to low confidence (3)
tests/monotouch-test/ObjCRuntime/GCHandleSwitchRaceTest.cs:26
- The PR description still contains TODO items (“Write better description/Thorough review/Validate fix”). Given this is a runtime lifetime/GCHandle change, please replace the TODOs with a concrete description of the root cause, why the dual-GCHandle scheme is safe, and what test configurations (platform/registrar/linker) were used to validate the fix.
// This test verifies a race condition in xamarin_switch_gchandle:
// Thread A switches between weak/strong gchandles (via retain/release),
// while Thread B fetches the gchandle (by calling an exported ObjC method).
// The race is that Thread B can read the old gchandle after Thread A has
// freed it but before Thread A has set the new one.
// Ref: https://github.com/dotnet/macios/issues/24702
var done = new ManualResetEvent (false);
runtime/runtime.m:1603
set_strong_gchandlealways allocates a new strong GCHandle before checking if one already exists. Sincexamarin_switch_gchandleis called on every retain, this can turn into an attach/lookup + allocate/free on every retain even when the object is already in strong mode. Consider checking for an existing strong handle first (and early-return inxamarin_switch_gchandlewhen no transition is needed), only creating a new GCHandle when the dictionary doesn't already contain one.
GCHandle new_gchandle = xamarin_gchandle_new (managed_object, FALSE);
GCHandle existing = INVALID_GCHANDLE;
pthread_mutex_lock (&strong_gchandle_hash_lock);
if (strong_gchandle_hash == NULL)
strong_gchandle_hash = CFDictionaryCreateMutable (kCFAllocatorDefault, 0, NULL, NULL);
const void *value;
if (CFDictionaryGetValueIfPresent (strong_gchandle_hash, self, &value))
existing = (GCHandle) value;
if (existing == INVALID_GCHANDLE)
CFDictionarySetValue (strong_gchandle_hash, self, (const void *) new_gchandle);
pthread_mutex_unlock (&strong_gchandle_hash_lock);
runtime/runtime.m:1602
GCHandleis being stored in the CFDictionary by casting it directly to/fromconst void*((const void *) new_gchandle/(GCHandle) value). IfGCHandleis an integer type, this is implementation-defined and may also trigger compiler warnings depending on settings. Consider casting viauintptr_t(both when storing and retrieving) or store a small heap-allocated struct with proper CFDictionary value callbacks (similar togchandle_hashin trampolines.m).
const void *value;
if (CFDictionaryGetValueIfPresent (strong_gchandle_hash, self, &value))
existing = (GCHandle) value;
if (existing == INVALID_GCHANDLE)
CFDictionarySetValue (strong_gchandle_hash, self, (const void *) new_gchandle);
You can also share your feedback on Copilot code review. Take the survey.
Collaborator
✅ [CI Build #6fd97a9] Build passed (Build packages) ✅Pipeline on Agent |
Collaborator
✅ [PR Build #6fd97a9] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
Collaborator
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
Collaborator
🔥 [CI Build #6fd97a9] Build failed (Build macOS tests) 🔥Build failed for the job 'Build macOS tests' (with job status 'Failed') Pipeline on Agent |
Collaborator
🔥 [CI Build #6fd97a9] Test results 🔥Test results❌ Tests failed on VSTS: test results 5 tests crashed, 31 tests failed, 100 tests passed. Failures❌ dotnettests tests (iOS) [attempt 2]1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (MacCatalyst) [attempt 2]1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ dotnettests tests (macOS) [attempt 2]1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (iOS) [attempt 2]1 tests failed, 10 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (MacCatalyst) [attempt 2]15 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (macOS) [attempt 2]12 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ Tests on macOS Monterey (12) tests🔥 Failed catastrophically on VSTS: test results - mac_monterey (no summary found). Html Report (VSDrops) Download ❌ Tests on macOS Ventura (13) tests🔥 Failed catastrophically on VSTS: test results - mac_ventura (no summary found). Html Report (VSDrops) Download ❌ Tests on macOS Sonoma (14) tests🔥 Failed catastrophically on VSTS: test results - mac_sonoma (no summary found). Html Report (VSDrops) Download ❌ Tests on macOS Sequoia (15) tests🔥 Failed catastrophically on VSTS: test results - mac_sequoia (no summary found). Html Report (VSDrops) Download ❌ Tests on macOS Tahoe (26) tests🔥 Failed catastrophically on VSTS: test results - mac_tahoe (no summary found). Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS testsPipeline on Agent |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TODO:
Fixes #24702.