Fix data race on `GIDGoogleUser` tokens by w-goog · Pull Request #640 · google/GoogleSignIn-iOS

w-goog · 2026-09-22T21:35:21Z

Fixes a data race on GIDGoogleUser tokens. This addresses a number of potential issues. (Yes, I used Markdown to write this, an LLM didn't write it :P)

What changed

access, refresh, and ID tokens (plus granted scopes and configuration) are now stored on the same object.
updates to that object by GSI are protected by a recursive lock.
a use-case specific lock was added for the token snapshot and profile changes.
KVO on the same thread still works (see the removed @synchronized(self))
Behaviour change: configuration is now computed once when the user is created (it was previously computed on first access). This will differ from GTMAppAuth's prior behavior on automated refreshes. Tbh, that was probably a bug which was caching nil values.
there's a new CI job w/Thread Sanitizer

What could have gone wrong
Previously, accessing these properties on different threads could have resulted in a crash. AppAuth & GSI mostly deliver everything on the main queue, but it seems likely that user.accessToken.tokenString would be read by networking code on the background queue. A refresh on the main queue at the same time could result in EXC_BAD_ACCESS . When you multiply that (small) likelihood by the number of times this code runs...it's probably happening.

Additionally:

updateTokensWithAuthState was updating each token individually, and the synchronized self only applied in updateWithTokenResponse; calls to the token update from elsewhere had no lock.
A client call of synchronized(googleUser) would share the library's lock, potentially leading to deadlocks.
Readers could receive a mixed set of tokens, since they were each updated separately.
Overlapping token updates could apply out of order.

What might still go wrong

authState can still be read directly, by GIDSignIn and others via fetcherAuthorizer. There's a TODO on the property, but the fix would be a public API change.
performActionWithFreshTokens in AppAuth still reads OIDAuthState without a lock if GTMAppAuth allows that off of the main thread. This is existing behaviour, and would require a fix in AppAuth.

… lock

… concurrency

…vate locks

…ve lock

…lock

…ocks

… an update

…omments

… snapshot

…'s lock

…ng a user

mdmathias

There are a lot of commits here, including a merge from main and two separate "Changelog: serialize…" commits. The changelog commits' wording ("serialize under the object lock") no longer matches the final design. Please squash, or otherwise clean up to make sure things are easy to follow and match.

mdmathias · 2026-09-29T22:07:50Z

+
+  // Guards `_tokens` and `_profile`. It is only ever held for a single read or write of those
+  // ivars, never while calling out to other code.
+  os_unfair_lock _tokenLock;


The os_unfair_lock use looks correct, but I don't think we need it (since it seems like it rebuilds what atomic properties do under the hood for free). Every critical section is a single pointer read or write, which is exactly what a synthesized atomic property provides. The runtime does the same retain-and-return under its own lock, so a concurrent setter still can't free the snapshot. Could we:

Drop the custom tokens accessors and let @property(atomic, strong) GIDGoogleUserTokens *tokens synthesize?

Move profile into GIDGoogleUserTokens, or a private atomic backing property, so -profile doesn't need a lock either?

That removes _tokenLock, the lock-ordering rule, and os_unfair_lock_assert_not_owner, which crashes in release builds if violated. It also makes profile and the tokens change together: right now updateWithTokenResponse:… updates them in separate critical sections, so readers can see a new profile with old tokens even though they share a lock. It also fixes the old snapshot being released while the lock is held (L188). I don't think speed is a concern either way at this call frequency.

mdmathias · 2026-09-29T22:20:17Z


+@interface GIDGoogleUser ()
+
+- (void)getAccessToken:(GIDToken *_Nullable *_Nullable)accessToken


The method is declared in a GIDGoogleUser (Internal) category in the .m, then redeclared here in the test. Could it move to GIDGoogleUser_Private.h? Alternatively, expose the GIDGoogleUserTokens snapshot internally and have callers read one object instead of three out-params. That's simpler, and scopes and configuration come along for free.

mdmathias · 2026-09-29T22:24:02Z

+
+// The token properties are derived from `tokens`, so KVO observers of each one are notified
+// whenever `tokens` is replaced.
+ (NSSet<NSString *> *)keyPathsForValuesAffectingAccessToken {


Every token property now depends on tokens. So when only the refresh token or the granted scopes change, observers of accessToken (and idToken) are still notified. Before, KVO fired only for the property that actually changed. This is visible to apps (for example, anyone who re-authorizes whenever accessToken fires). Could we call it out in the CHANGELOG? Or override automaticallyNotifiesObserversForKey: and notify each key only when it actually changes?

mdmathias · 2026-09-29T22:27:02Z

  NSMutableDictionary *additionalParameters = [@{} mutableCopy];
+  // Read the auth state under `_authStateLock` so building the request cannot interleave with
+  // -updateWithTokenResponse:authorizationResponse:profileData:.
+  [self lockAuthState];


The lock also covers +[GIDEMMSupport updatedEMMParametersWithParameters:] and +[GIDSignInPreferences loggingParameters]. Consider reading only the authState fields under the lock and building additionalParameters after unlocking.

mdmathias · 2026-09-29T22:33:27Z

+                 originalAuthorizationResponse:authorizationResponse
                                      callback:^(OIDTokenResponse *_Nullable tokenResponse,
                                                 NSError *_Nullable error) {
+    // Update the auth state under `_authStateLock` so this refresh cannot interleave with


The description says this fixes "Overlapping token updates could apply out of order", but the lock only makes each update atomic; it doesn't order them. The refresh request is built under the lock, but the lock is released while the request is in flight. If addScopes calls updateWithTokenResponse:authorizationResponse:… in the meantime, this callback then applies a response based on the pre-addScopes grant on top of it. As far as I can tell, AppAuth doesn't check which authorization a token response belongs to. The result is that grantedScopes and accessToken can revert. This is existing behavior, not a regression. Could we either drop the response when self.authState.lastAuthorizationResponse != authorizationResponse (the value you already capture at L307), or remove that line from the description?

mdmathias · 2026-09-29T22:54:42Z

-@property(nonatomic, readwrite, nullable) GIDToken *idToken;
-
 /// A representation of the state of the OAuth session for this instance.
+// TODO: Reads through this property bypass the lock GIDGoogleUser takes around its own auth


Thanks for the TODO. Since GIDSignIn and fetcherAuthorizer read authState without the lock, locking encodeWithCoder: and emmSupport protects against GSI's own updates but not AppAuth/GTMAppAuth refreshes that go through fetcherAuthorizer. Could we file an issue for that and link it from the TODO?

Also, it would be nice to connect TODOs with an issue on the repo for tracking purposes.

mdmathias · 2026-09-29T22:57:14Z

+      // The consistency guarantee comes from the snapshot accessor's single lock acquisition.
+      // Reading the three properties individually would NOT be atomic even with the per-accessor
+      // locking, by design.
+      if ([accessToken.tokenString isEqualToString:accessTokenA]) {


If accessToken is neither A nor B, the read checks nothing. Consider asserting that idToken matches the initial token in that case too, so a mixed initial/A snapshot is also caught.

mdmathias · 2026-09-29T22:59:35Z

+  // The consistency assertion above is what gives it meaning without TSan.
+}
+
+- (void)testRefreshTokensIfNeeded_readsConsistentSnapshot {


This looks similar to testRefreshTokensIfNeededWithCompletion_refresh_givenAccessTokenExpired. Suggest deleting it, or renaming it and asserting something new.

mdmathias · 2026-09-29T23:00:16Z

+// KVO observers of the token properties run while the user holds its auth state lock. This checks
+// that an observer can call back into the user on the same thread, including starting another
+// token update, without deadlocking or aborting.
+- (void)testTokenObserver_reentersUserDuringUpdate {


Nice test. Consider adding a second observer to check that nested updates don't deliver notifications out of order (it would see B's notification before A's finishes).

mdmathias · 2026-09-29T23:01:19Z

+        xcodebuild \
+          -scheme GoogleSignIn-Package \
+          -sdk iphonesimulator \
+          -destination "platform=iOS Simulator,name=iPhone 16,OS=18.6" \


Xcode 16.4, iPhone 16 and iOS 18.6 are hardcoded, while the other jobs use a matrix. Could we reuse the matrix values so it doesn't drift from the others? Consider a macOS destination as well.

w-goog and others added 17 commits September 4, 2026 11:29

g-orchestrated: Serialize GIDGoogleUser token access under the object…

c52a562

… lock

g-orchestrated: Changelog: serialize GIDGoogleUser token access

133d5a6

g-orchestrated: Test: GIDGoogleUser token reads stay consistent under…

1f3dcc7

… concurrency

Update push and pr notification to use Cards v2. (#636)

5e23369

Carry nonce and claimsAsJSON through Device Policy flows (#637)

095bb84

Fix crash when EMM emitted non-string errors (#638)

8d9ebca

g-orchestrated: Changelog: serialize GIDGoogleUser token access

8d8f0dd

g-orchestrated: Test: race token updates against public property reads

7f98cfe

g-orchestrated: CI: run the iOS unit tests under Thread Sanitizer

d2a2ec5

g-orchestrated: Replace @synchronized(self) in GIDGoogleUser with pri…

5b4a66b

…vate locks

g-orchestrated: Store GIDGoogleUser tokens as one immutable set

b10b2f9

Merge branch 'main' into fix/googleuser-token-sync

38467ca

g-orchestrated: Replace GIDGoogleUser's update locks with one recursi…

9db8c36

…ve lock

g-orchestrated: Read GIDGoogleUser's auth state under the auth state …

4d197d4

…lock

g-orchestrated: Update auth state under the lock after a token refresh

0ec02bc

g-orchestrated: Guard GIDGoogleUser's profile and encoding with its l…

b30e552

…ocks

g-orchestrated: Test: KVO observers can re-enter GIDGoogleUser during…

ac657be

… an update

w-goog requested a review from mdmathias September 23, 2026 22:27

w-goog added 4 commits September 23, 2026 15:35

g-orchestrated: Assert GIDGoogleUser's lock order and trim its lock c…

2794714

…omments

g-orchestrated: Serve configuration and granted scopes from the token…

6964fab

… snapshot

g-orchestrated: Note that direct authState reads bypass GIDGoogleUser…

aac6854

…'s lock

g-orchestrated: Test: stub the token response scope read when restori…

7c6023c

…ng a user

w-goog changed the title ~~Fix/googleuser token sync~~ Fix data race on GIDGoogleUser tokens Sep 24, 2026

w-goog marked this pull request as ready for review September 24, 2026 00:48

mdmathias reviewed Sep 29, 2026

View reviewed changes

Navigation Menu

Fix data race on `GIDGoogleUser` tokens - #640

Fix data race on `GIDGoogleUser` tokens#640
w-goog wants to merge 21 commits into
mainfrom
fix/googleuser-token-sync

w-goog commented Sep 22, 2026 •

edited

Loading

Uh oh!

mdmathias left a comment

Uh oh!

mdmathias Sep 29, 2026

Uh oh!

mdmathias Sep 29, 2026

Uh oh!

mdmathias Sep 29, 2026

Uh oh!

mdmathias Sep 29, 2026

Uh oh!

mdmathias Sep 29, 2026

Uh oh!

mdmathias Sep 29, 2026

Uh oh!

mdmathias Sep 29, 2026

Uh oh!

mdmathias Sep 29, 2026

Uh oh!

mdmathias Sep 29, 2026

Uh oh!

mdmathias Sep 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		@interface GIDGoogleUser ()

		- (void)getAccessToken:(GIDToken _Nullable _Nullable)accessToken

Conversation

w-goog commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdmathias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

w-goog commented Sep 22, 2026 •

edited

Loading