Fix use-after-free race in MSIDFlightManager - #1900
Open
Dharshan BJ (DharshanBJ) wants to merge 7 commits into
Open
Fix use-after-free race in MSIDFlightManager#1900Dharshan BJ (DharshanBJ) wants to merge 7 commits into
Dharshan BJ (DharshanBJ) wants to merge 7 commits into
Conversation
Dharshan BJ (DharshanBJ)
requested review from
Copilot,
Hieu Nguyen (hieunguyenmsft) and
Roman Khrestek (rokhrest)
July 14, 2026 17:58
Comment on lines
22
to
25
| // THE SOFTWARE. | ||
| // TEST | ||
|
|
||
| #import "MSIDBrokerConstants.h" |
| // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| // THE SOFTWARE. | ||
| // TEST |
Serialize all access to _flightProvider on the synchronization queue and hold a strong local reference while messaging the provider, so a concurrent setFlightProvider: cannot deallocate it between the nil-check and the message send (use-after-free -> SIGSEGV). Change the setter to dispatch_barrier_sync so the assignment/release completes before it returns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c3dcf24a-1b1d-4835-b745-826edac60905
Dharshan BJ (DharshanBJ)
changed the title
Dharshan BJ (DharshanBJ)
changed the title
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c3dcf24a-1b1d-4835-b745-826edac60905
| // installed provider) completes before the setter returns. An async barrier let the | ||
| // previous provider be released on the queue after the setter returned, racing with | ||
| // concurrent readers and allowing a stale/nil provider to be observed. | ||
| dispatch_barrier_sync(self.synchronizationQueue, ^{ |
Contributor
There was a problem hiding this comment.
This can deadlock if a provider calls setFlightProvider: from boolForKey: or stringForKey:, since those callbacks run on the same queue and this uses dispatch_barrier_sync. Could we capture the provider while on the queue, then invoke it after leaving the queue?
There was a problem hiding this comment.
updated, boolForKey: / stringForKey: now capture the provider under the queue and invoke it after leaving, so a re-entrant setFlightProvider: no longer deadlocks.
… deadlock Capture a strong reference to the provider on the queue, then invoke boolForKey:/ stringForKey: after leaving the queue. This preserves the use-after-free fix while preventing a deadlock if a provider re-enters setFlightProvider: from its own callback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fe48d6bf-0351-4eb6-b0ed-93de3427ee1c
| }); | ||
|
|
||
| return result; | ||
| return provider ? [provider boolForKey:flightKey] : NO; |
Contributor
There was a problem hiding this comment.
The use-after-free path is already covered by testConcurrentReads_whileProviderSwappedAndCleared_doNotCrash, but the re-entrancy case this change fixes isn't covered anywhere. Can we add a regression test with a provider whose boolForKey: calls setFlightProvider: back on the same manager? It's deterministic now and would have hung before this change.
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.
PR Checklist (must be completed before review)
Proposed changes
Fixes a race condition in
MSIDFlightManagerthat could cause a use-after-free crash (SIGSEGV) when flights are read concurrently with a provider swap.Root cause: A data race / use-after-free on _flightProvider . Readers accessed it through the unsynchronized nonatomic getter while setFlightProvider: used dispatch_barrier_async , releasing the previous provider asynchronously. A concurrent reader could retain a pointer mid-release → objc_retain on freed memory → crash.
The fix ( IdentityCore/src/MSIDFlightManager.m ):
• setFlightProvider: → dispatch_barrier_sync , so the assignment and release of the old provider complete before returning and are exclusive against reads.
• boolForKey: / stringForKey: (and the synchronized public flightProvider getter) read the ivar inside dispatch_sync into a strong local, then invoke the provider outside the block — keeping it alive during use and avoiding a re-entrancy deadlock.
Result: All reads/writes of _flightProvider are serialized on the queue with a strong reference held during use, eliminating the use-after-free.
Scope:
IdentityCore/src/MSIDFlightManager.m(three methods).Type of change
Risk