evgenyneu / keychain-swift

Helper functions for saving text in Keychain securely for iOS, OS X, tvOS and watchOS.
MIT License
2.85k stars 345 forks source link

Race Condition when used in App Extension #164

Open juliand665 opened 1 year ago

juliand665 commented 1 year ago

I've been using KeychainSwift for a project of mine, and started running into issues when I added widgets. It was the most frustrating bug to track down, its reproduction at the mercy of iOS' decisions to update my widgets and of course happening so nondeterministically that I would have to wait for a week to be reasonably confident in any fix. Anyway, the bug was that I would lose data stored in the keychain from time to time.

When iOS spins up your app extension to update widgets, it can launch multiple processes, or the main app process might also be running, with the effect that you have multiple processes interfacing with the keychain at the same time. Because they are separate processes, the mutex mechanism you're using cannot do anything, so operations have to be atomic. However, this is not the case for the set function, because of this line:

https://github.com/evgenyneu/keychain-swift/blob/c1fde55798b164cad44b5e23cfa2f0f1ebcd76af/Sources/KeychainSwift.swift#L93

It does this to unify the code paths for adding a new value and updating an existing value. However, it makes it possible for another process to read the now-deleted value between the delete and add calls. Even without this unfortunate interleaving, it's possible that iOS terminates the process right in between these two as well. I'm not sure which of these caused the issue more often, because I wouldn't expect the former to result in the deletion being written back somehow, but the data would often disappear permanently, so something was definitely up.

Anyway, I ended up fixing the issue for myself by dropping the dependency on KeychainSwift and implementing the necessary logic myself, which turned out to not be as bad as I feared. Figured I'd let you know though!