Page MenuHomePhabricator

[native] Implement utils to lock MMKV for entire sequence of operations and use to to sync encryption and decryption of notifs
ClosedPublic

Authored by marcin on Aug 2 2024, 8:16 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 9:30 PM
Unknown Object (File)
Wed, Nov 20, 9:30 PM
Unknown Object (File)
Wed, Nov 20, 9:30 PM
Unknown Object (File)
Wed, Nov 20, 9:30 PM
Unknown Object (File)
Wed, Nov 20, 5:10 AM
Unknown Object (File)
Mon, Nov 18, 11:00 AM
Unknown Object (File)
Fri, Nov 8, 11:06 PM
Unknown Object (File)
Fri, Nov 8, 12:23 PM
Subscribers

Details

Summary

This differential implements utils to avoid data races between the main app and the notifs code. By default MMKV synchronizes only for a single operation (read , write or removal). Unfortunately this opens the gate for data races against notif encryption and decryption that happen on different thread/proces and involve reading and writing back the state of the olm session.

MMKV exposes public methods to lock and unlock the database for the sequence of operations. I wasn't aware of them since they are not mentioned in the documentation so I had to dig them up in the code.
They look like these: https://github.com/Tencent/MMKV/blob/v1.3.5/Core/MMKV.cpp#L1190-L1201, https://github.com/Tencent/MMKV/blob/v1.3.5/Core/MMKV.h#L396-L399

Test Plan

Added logs to introduced methods informing when the lock is acquired and when released. Sent e2ee notifs back and forth and examined the logs.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin edited the test plan for this revision. (Show Details)
marcin requested review of this revision.Aug 2 2024, 8:32 AM
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
617 ↗(On Diff #43041)

This change makes all operations necessary to decrypt a notif run atomically and without the risk of corrupoting state after encryption. Unfortunately this means that we might have a network call (call to identity if brand new session creation is necessary) in this critical section. There is a way to enchance this by adopting the approach that we have for web. That is outside of the scope of this diff. and should be handled by separate task. (I will introduce it shortly)

native/cpp/CommonCpp/Tools/CommMMKV.h
35 ↗(On Diff #43041)

The constructor of ScopedCommMMKVLock will acquire the lock while the destructor will release the lock. This is how we implement finally in C++. So the idea is to create instance of this class and perform a sequence of operations in the scope were the instance is valid.

native/ios/Comm/CommMMKV.mm
11 ↗(On Diff #43041)

Interestingly lock and unlock methods are public in Android java wrapper but are not in Objective-C wrapper. However the [[ https://github.com/Tencent/MMKV/blob/v1.3.5/iOS/MMKV/MMKV/libMMKV.mm#L55-L60 | Objective - C wrapper has an instance variable of type mmkv::MMKV ]] so we can use it to access lock and unlock methods.

This is a bit hacky but it is well-known pattern that are using this pattern for CommSecureStore where we redefine EXSecureStore interface with its private methods. This allows us to use them even though they are not publicly available via header file.

Nice work finding these in the MMKV code!

native/ios/Comm/CommMMKV.mm
11 ↗(On Diff #43041)

Can you add a code comment explaining this?

Nice, this logic makes sense to me. I'm wondering, why lock() and unlock() are undocumented in MMKV...

This revision is now accepted and ready to land.Aug 5 2024, 2:35 AM
marcin edited the summary of this revision. (Show Details)

Add comment explaining hacky pattern