Skip to content

Fix data race on GIDGoogleUser tokens - #640

Open
w-goog wants to merge 21 commits into
mainfrom
fix/googleuser-token-sync
Open

w-goog wants to merge 21 commits into
mainfrom
fix/googleuser-token-sync

Conversation

@w-goog

@w-goog w-goog commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

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.

@w-goog
w-goog requested a review from mdmathias September 23, 2026 22:27
@w-goog w-goog changed the title Fix/googleuser token sync Fix data race on GIDGoogleUser tokens Sep 24, 2026
@w-goog
w-goog marked this pull request as ready for review September 24, 2026 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants