Page MenuHomePhabricator
Differential D8905

Introduce thread-safety to NotificationsCryptoModule during cuncurrent access
ClosedPublic
Actions

Authored by marcin on Aug 22 2023, 3:54 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 4, 3:50 AM
Unknown Object (File)
Sun, Oct 27, 11:11 PM
Unknown Object (File)
Fri, Oct 18, 3:06 AM
Unknown Object (File)
Fri, Oct 18, 3:06 AM
Unknown Object (File)
Fri, Oct 18, 3:06 AM
Unknown Object (File)
Fri, Oct 18, 3:06 AM
Unknown Object (File)
Fri, Oct 18, 3:06 AM
Unknown Object (File)
Fri, Oct 18, 3:06 AM
View All Files
Subscribers
None

Details

Reviewers
tomek
ashoat
bartek
Commits
rCOMM70cf45852540: Introduce thread-safety to NotificationsCryptoModule during cuncurrent access
Summary

Current implementation of atomic write to notification olm session writes might fail when called from multiple threads due to reasons explained in: https://phab.comm.dev/D8795#inline-56969. This differential aims to mitigate the issue.

Test Plan
  1. Change notification encryption on the keyserver to encrypt field by field: https://gist.github.com/marcinwasowicz/a18b31f8bee1d56cc50fb71809077dcb
  2. Let notification decryption run concurrently in NSE using dispatch queues:https://gist.github.com/marcinwasowicz/4cca17794123b110134bf393d21e0b14
  3. Ensure that error occur before this diff but notification is correctly decrypted and displayed after this diff.

My logs after applying this diff looked like this:

NotificationService	default	15:44:48.290206+0200	COMM: <NSThread: 0x14541a1c0>{number = 2, name = (null)} started decrypt
NotificationService	default	15:44:48.290256+0200	COMM: <NSThread: 0x145321e10>{number = 3, name = (null)} started decrypt
NotificationService	default	15:44:48.290268+0200	COMM: <NSThread: 0x14541a440>{number = 4, name = (null)} started decrypt
NotificationService	default	15:44:48.290647+0200	Not internal release, disabling SIRL
NotificationService	default	15:44:48.290667+0200	System Keychain Always Supported set via feature flag to disabled
NotificationService	default	15:44:48.290709+0200	Adding securityd connection to pool, total now 1
NotificationService	default	15:44:48.290806+0200	Adding securityd connection to pool, total now 2
NotificationService	default	15:44:48.290946+0200	Adding securityd connection to pool, total now 3
NotificationService	default	15:44:48.291805+0200	Requesting app group container lookup; personaid = 1000, type = DEFAULT, name = 76D364EF-0FF9-4233-9BE8-73A0AF6750BE, origin [pid = 132, personaid = 199], proximate [pid = 32, personaid = 199], identifier = <private>, euid = 501, uid = 501, platform = 2
NotificationService	default	15:44:48.291855+0200	Requesting app group container lookup; personaid = 1000, type = DEFAULT, name = 76D364EF-0FF9-4233-9BE8-73A0AF6750BE, origin [pid = 132, personaid = 199], proximate [pid = 32, personaid = 199], identifier = <private>, euid = 501, uid = 501, platform = 2
NotificationService	default	15:44:48.291930+0200	Requesting app group container lookup; personaid = 1000, type = DEFAULT, name = 76D364EF-0FF9-4233-9BE8-73A0AF6750BE, origin [pid = 132, personaid = 199], proximate [pid = 32, personaid = 199], identifier = <private>, euid = 501, uid = 501, platform = 2
NotificationService	default	15:44:48.292774+0200	Consuming sandbox extension; path = [<private>], key = 0
NotificationService	default	15:44:48.292813+0200	Revoking sandbox extension; key = 0
NotificationService	default	15:44:48.292839+0200	container_create_or_lookup_app_group_path_by_app_group_identifier: success
NotificationService	default	15:44:48.293096+0200	Consuming sandbox extension; path = [<private>], key = 0
NotificationService	default	15:44:48.293118+0200	Revoking sandbox extension; key = 0
NotificationService	default	15:44:48.293191+0200	container_create_or_lookup_app_group_path_by_app_group_identifier: success
NotificationService	default	15:44:48.293543+0200	Consuming sandbox extension; path = [<private>], key = 0
NotificationService	default	15:44:48.293569+0200	Revoking sandbox extension; key = 0
NotificationService	default	15:44:48.293601+0200	container_create_or_lookup_app_group_path_by_app_group_identifier: success
NotificationService	default	15:44:48.294760+0200	COMM: <NSThread: 0x145321e10>{number = 3, name = (null)} ended decrypt
NotificationService	default	15:44:48.295073+0200	COMM: <NSThread: 0x14541a440>{number = 4, name = (null)} ended decrypt
NotificationService	default	15:44:48.295171+0200	COMM: <NSThread: 0x14541a1c0>{number = 2, name = (null)} ended decrypt

All threads successfully decrypt their content and don't run sequentially.

Before applying this diff:

NotificationService	default	15:48:42.754225+0200	COMM: <NSThread: 0x109b20290>{number = 2, name = (null)} started decrypt
NotificationService	default	15:48:42.754256+0200	COMM: <NSThread: 0x12801b240>{number = 3, name = (null)} started decrypt
NotificationService	default	15:48:42.754343+0200	COMM: <NSThread: 0x128304340>{number = 4, name = (null)} started decrypt
NotificationService	default	15:48:42.754865+0200	Not internal release, disabling SIRL
NotificationService	default	15:48:42.754918+0200	System Keychain Always Supported set via feature flag to disabled
NotificationService	default	15:48:42.755006+0200	Adding securityd connection to pool, total now 1
NotificationService	default	15:48:42.755136+0200	Adding securityd connection to pool, total now 2
NotificationService	default	15:48:42.755182+0200	Adding securityd connection to pool, total now 3
NotificationService	default	15:48:42.756612+0200	Requesting app group container lookup; personaid = 1000, type = DEFAULT, name = 76D364EF-0FF9-4233-9BE8-73A0AF6750BE, origin [pid = 132, personaid = 199], proximate [pid = 32, personaid = 199], identifier = <private>, euid = 501, uid = 501, platform = 2
NotificationService	default	15:48:42.756647+0200	Requesting app group container lookup; personaid = 1000, type = DEFAULT, name = 76D364EF-0FF9-4233-9BE8-73A0AF6750BE, origin [pid = 132, personaid = 199], proximate [pid = 32, personaid = 199], identifier = <private>, euid = 501, uid = 501, platform = 2
NotificationService	default	15:48:42.756706+0200	Requesting app group container lookup; personaid = 1000, type = DEFAULT, name = 76D364EF-0FF9-4233-9BE8-73A0AF6750BE, origin [pid = 132, personaid = 199], proximate [pid = 32, personaid = 199], identifier = <private>, euid = 501, uid = 501, platform = 2
NotificationService	default	15:48:42.758270+0200	Consuming sandbox extension; path = [<private>], key = 0
NotificationService	default	15:48:42.758338+0200	Revoking sandbox extension; key = 0
NotificationService	default	15:48:42.758374+0200	container_create_or_lookup_app_group_path_by_app_group_identifier: success
NotificationService	default	15:48:42.759088+0200	Consuming sandbox extension; path = [<private>], key = 0
NotificationService	default	15:48:42.759113+0200	Revoking sandbox extension; key = 0
NotificationService	default	15:48:42.759136+0200	container_create_or_lookup_app_group_path_by_app_group_identifier: success
NotificationService	default	15:48:42.760148+0200	Consuming sandbox extension; path = [<private>], key = 0
NotificationService	default	15:48:42.760188+0200	Revoking sandbox extension; key = 0
NotificationService	default	15:48:42.760227+0200	container_create_or_lookup_app_group_path_by_app_group_identifier: success
NotificationService	default	15:48:42.760673+0200	COMM: <NSThread: 0x12801b240>{number = 3, name = (null)} ended decrypt

Only one thread was able to decrypt.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin created this revision.Aug 22 2023, 3:54 AM
marcin added a parent revision: D8638: Possible solution for broken iOS notifications.Aug 22 2023, 4:01 AM
marcin added a child revision: D8906: Correct error handling across Objective-C and C++.
Harbormaster returned this revision to the author for changes because remote builds failed.Aug 22 2023, 4:06 AM
Harbormaster failed remote builds in B21993: Diff 30202!
marcin updated this revision to Diff 30206.Aug 22 2023, 4:06 AM

Rebase to update commit message

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 22 2023, 4:22 AM
Harbormaster failed remote builds in B21997: Diff 30206!
marcin updated this revision to Diff 30210.Aug 22 2023, 5:13 AM
  1. Use smalle suffix length
  2. Remove whitespace from suffix
  3. Fix compilation error
marcin edited the test plan for this revision. (Show Details)Aug 22 2023, 5:29 AM
Harbormaster completed remote builds in B22001: Diff 30210.Aug 22 2023, 5:30 AM
marcin requested review of this revision.Aug 22 2023, 5:30 AM
marcin edited the test plan for this revision. (Show Details)Aug 22 2023, 6:53 AM
ashoat accepted this revision.Aug 22 2023, 10:09 AM
ashoat added 1 blocking reviewer(s): tomek.

Makes sense, but the comment seems confusing

Would also love if @tomek could take a look

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
27 ↗(On Diff #30210)

Why does this say 36, but the value below is 8?

tomek accepted this revision.Aug 22 2023, 10:29 AM
tomek added inline comments.
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
93–98 ↗(On Diff #30210)

By doing that we're risking that the name will get shorter. This sounds serious because it reduces the number of possible file names, but only a little bit. But still, it could be better to avoid generating strings with spaces at all.

A more serious issue with the number of strings is that I'm not sure if all the filesystems with which this code might interact are case-sensitive. We probably would be safer assuming that this isn't guaranteed.

We probably should just use generateRandomHexString with the appropriate length, e.g. 16 or even 32.

This revision is now accepted and ready to land.Aug 22 2023, 10:29 AM
marcin added inline comments.Aug 23 2023, 1:53 AM
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
27 ↗(On Diff #30210)

Definitely my oversight.

93–98 ↗(On Diff #30210)

Thanks for catching this - I will update the implementation to use 32 characters hex string.

marcin updated this revision to Diff 30255.Aug 23 2023, 7:20 AM
marcin edited the test plan for this revision. (Show Details)

Use hex string for temporary file suffix

Harbormaster completed remote builds in B22039: Diff 30255.Aug 23 2023, 7:37 AM
tomek accepted this revision.Aug 24 2023, 2:27 AM
marcin updated this revision to Diff 30838.Sep 7 2023, 8:39 AM

Rebase before landing

Harbormaster completed remote builds in B22426: Diff 30838.Sep 7 2023, 8:57 AM
Closed by commit rCOMM70cf45852540: Introduce thread-safety to NotificationsCryptoModule during cuncurrent access (authored by marcin). · Explain WhySep 7 2023, 9:03 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMM70cf45852540: Introduce thread-safety to NotificationsCryptoModule during cuncurrent access.

Revision Contents
Changeset List

PathSize
native/
cpp/
CommonCpp/
Notifications/
BackgroundDataStorage/
15 lines

Diff 30842

View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp

Loading...