Fix macOS crash in NetworkChange when the run loop thread already exited - #131867
Open
isaacisland wants to merge 1 commit into
Open
Fix macOS crash in NetworkChange when the run loop thread already exited#131867isaacisland wants to merge 1 commit into
isaacisland wants to merge 1 commit into
Conversation
On macOS StopRunLoop() guards s_runLoop with only a Debug.Assert, which is erased in Release, and then dereferences it via CFRunLoopIsWaiting(s_runLoop). RunLoopThreadStart() sets s_runLoop = IntPtr.Zero and returns whenever CFRunLoopRun() returns early, which happens once the SCDynamicStore run loop source is invalidated (for example a configd restart, or sleep/wake). That path is silent: no exception is raised and subscribers stay registered, so nothing observes that the listener is gone. If the last subscriber is removed after that, StopRunLoop() passes NULL to CFRunLoopIsWaiting. CoreFoundation reads the CFRuntimeBase _cfinfo header at [NULL+8], and the process dies with EXC_BAD_ACCESS / KERN_INVALID_ADDRESS at 0x8. It is a SIGSEGV, so it cannot be caught. Return early when s_runLoop is already zero. The early return still waits on s_runLoopEndedEvent so the stale signal from the exited thread is consumed and a later CreateAndStartRunLoop/StopRunLoop pair starts from a clean state, and the two remaining asserts move below the check because that thread's epilogue has already nulled s_runLoopSource and s_dynamicStoreRef.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
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.
Problem
On macOS,
NetworkChange.StopRunLoop()guardss_runLoopwith only aDebug.Assert— erased in Release — and then dereferences it:RunLoopThreadStart()setss_runLoop = IntPtr.Zeroand returns wheneverCFRunLoopRun()returns early, which happens once theSCDynamicStorerun loop source is invalidated — aconfigdrestart, or sleep/wake on a laptop. That path is silent: no exception is raised, subscribers stay registered, and nothing observes that the listener is gone (address-changed events simply stop firing from then on).If the last subscriber is removed after that,
StopRunLoop()passesNULLtoCFRunLoopIsWaiting. CoreFoundation reads theCFRuntimeBase._cfinfoheader at[NULL+8]and the process dies withEXC_BAD_ACCESS / KERN_INVALID_ADDRESSat0x8. It's a SIGSEGV, so it cannot be caught — the whole process goes down.Evidence
This is not theoretical. We hit it as the single largest crash in a fleet of macOS agents: 13,497 crashes across 6,213 machines over ~5 weeks. Our service is a long-lived daemon that subscribes to
NetworkAddressChangedand unsubscribes during shutdown, so it reaches the 1→0 subscriber transition on every restart.Across 25 crash dumps from distinct machines, the register context is identical:
x0 = 0x0— theCFRunLoopRefargumentx8 = 0x8— the computed fault addresslrinsideCFRunLoopIsWaiting,pcin__CFCheckCFInfoPACSignatureand in all 25, the
.NET Network Address Changethread is absent from the process — i.e. it had already run its epilogue and zeroeds_runLoop. Reproduced on macOS 15.1, 26.2, 26.3, 26.4, 26.5 and 26.6, so it is not tied to an OS version.StopRunLoop()is byte-identical fromrelease/6.0throughrelease/10.0andmain, unchanged since #41768 (Oct 2019).Most applications never hit this because they never unsubscribe — they just exit, and
StopRunLoop()is never called.Fix
Return early when
s_runLoopis already zero.Two details beyond a bare null check:
s_runLoopEndedEvent. That thread has already calledSet()(or is about to), and since it's anAutoResetEvent, leaving the signal unconsumed would make the nextStopRunLoop()return immediately and desynchronize the start/stop state machine. Waiting also correctly handles the window wheres_runLoopis zeroed but the epilogue hasn't finished.s_runLoopSourceands_dynamicStoreRef, so they would fire spuriously in Debug.Notes / possible follow-ups
Happy to fold either of these in, or leave them as separate issues:
CFRunLoopRun()returns early,NetworkAddressChangednever fires again for the lifetime of the process, with no error surfaced. Recovering (re-creating the store and thread on the next subscribe) would be a behavior change, so I've left it out of this fix.SCDynamicStoreCreateRunLoopSource's result is never validity-checked inCreateAndStartRunLoop(), unlike theCFString/pattern creations just above it. A null source there would makeCFRunLoopAddSourcea no-op andCFRunLoopRun()return immediately — reaching the same broken state from the very first subscribe.A deterministic regression test needs the listener thread forced through its epilogue (e.g. calling
CFRunLoopStopons_runLoopdirectly and waiting for it to zero) before removing the last handler. I didn't want to add a test that reaches into private static state without a maintainer's preference on approach — glad to add one in whatever form you'd like.