Repository navigation
Stop blocking on the main thread while holding the SPKI cache lock - #364
Conversation
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks locking/concurrency behavior in the certificate-pinning SPKI cache, a human look would still be worthwhile.
Beyond the inline findings (none posted), I also checked whether the async protected-data check could leave persistenceScheduled/cache-snapshot coalescing racy or reintroduce a main-thread deadlock — the flag is only ever read/written while on lockQueue, and the main-queue check is now dispatch_async rather than dispatch_sync, so the lock-holding thread is never blocked. I also considered the window between the async protected-data check and the actual disk write (could a device lock event land in between and cause a failed write under NSFileProtectionComplete) — this window already existed in the prior synchronous code and isn't meaningfully widened in a way that changes behavior beyond the existing TSKLog/NSAssert failure path.
Extended reasoning...
The change replaces a synchronous, lock-held isProtectedDataAvailable() + disk-write path in TSKSPKIHashCache.m with an async check plus a dedicated serial persistence queue, adding a persistenceScheduled coalescing flag and a waitForPendingSubjectPublicKeyInfoDiskWrites sync point used by resetSubjectPublicKeyInfoDiskCache and by the two updated test files. It touches concurrency-sensitive code underlying certificate pinning (not crypto logic itself, no injection/auth/data-exposure surface), is a single self-contained commit with accompanying test coverage for the new behavior, and has no outstanding reviewer objections in the timeline — but the subtlety of the locking/coalescing logic in security-adjacent code is enough to warrant a second human pass rather than auto-approval.
#348 serialized the cache with lockQueue but kept persistence inside it, where isProtectedDataAvailable() dispatch_syncs to the main queue. A cache miss off the main thread then deadlocks if the main thread is waiting on that work or enters the cache itself. Persist asynchronously instead: check protected data without blocking, snapshot the cache under the lock, and write it on a serial utility queue, coalescing writes that are already pending.
285cd6c
c9e3e26 to
285cd6c
Compare
#348 serialized the cache with lockQueue but kept persistence inside it, where isProtectedDataAvailable() dispatch_syncs to the main queue. A cache miss off the main thread then deadlocks if the main thread is waiting on that work or enters the cache itself.
Persist asynchronously instead: check protected data without blocking, snapshot the cache under the lock, and write it on a serial utility queue, coalescing writes that are already pending.