search

evgenyneu / keychain-swift

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

fixed threading #118

Open sylvainlaurent opened 4 years ago

sylvainlaurent commented 4 years ago

This PR fixes some threading issues with the library mainly caused by the incorrect assumption that Swift variables are atomic. With this fix, the lock introduced by #68 is no longer useful, assuming that KeyChain is thread-safe as Apple docs say so. Besides, the ConcurrencyTests run fine without the lock (with a fix in the tests themselves).

sylvainlaurent commented 4 years ago

This should fix #63

evgenyneu commented 4 years ago

Thanks for the fix, @sylvainlaurent.

caused by the incorrect assumption that Swift variables are atomic.

Sorry, I don't understand this, I'm too dumb. Which line in code was causing the issue?

sylvainlaurent commented 4 years ago

Here are the threading problem I saw:

1) assigning values to lastQueryParameters in several places : lines 108, 188, 254, 278. When a value is assigned and replaces a previous one, this decreases the reference count of the old reference. Also, when the method exits there's a decrease of reference count too. Decreasing reference counts IS atomic with swift, but when the count reaches 0, the deallocation is NOT atomic as explained here https://github.com/apple/swift/blob/master/docs/proposals/Concurrency.rst (paragraph that contains This program crashes very quickly when it tries to deallocate an already deallocated class instance

2) lastResultCode is also not atomic, though there's probably no crash because there's no reference counting involved

3) In the ConcurrencyTests itself: there are several occurrences of writes = writes + 1. Increasing an int this way is not atomic, and after I removed the NSLock and fixed the lastQueryParameters problem, I saw test errors about having only 998 writes instead of 1000... Here, instead of making the writes variable atomic, I just moved the increment operation in the same thread (queue) for all occurrences...

hope it helps Sylvain

evgenyneu commented 4 years ago

@sylvainlaurent, thanks for explaining.

assigning values to lastQueryParameters in several places : lines 108, 188, 254, 278.

We assign lastQueryParameters variable after lock.lock(). So it should only be ran by one thread at a time, right? Why does it crash there?