Implements write operation for flat-file based message storage. It aither appends message to main storage file, or creates temporary file with random name if semaphore acquisition failed.
Details
Use implemented method in NotificationService to write to main storage.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
native/ios/Comm/TemporalMessageStorage/TemporalMessageStorage.mm | ||
---|---|---|
50–53 | It is not the best idea to discard a message because it contained this character. What should be done instead, is that the message should be sanitized by escaping the character (just like \ char is used in strings. Another thing is that I'm not sure if we really need to check / sanitize the message. We're encrypting it so even if it initially contained the character, after encryption it will be replaced by something else. What we should probably do instead is to sanitize the encrypted message, because encryption process could introduce the separator. | |
77 | We should release the lock in @finally because even if an exception was thrown, we should leave the lock in the proper state. |
native/ios/Comm/TemporalMessageStorage/TemporalMessageStorage.mm | ||
---|---|---|
50–53 | Currently we encrypt each message separately but decrypt whole file at once. Therefore encryption takes place after padding message with encryptedDataSeparator so that its length is a multiply of AES block size (128 byte). That is why we must either discard or sanitize message before encryption. To avoid doing so we would need to introduce separator between encrypted messages and decrypt each message separately after splitting file content. |
native/ios/Comm/TemporalMessageStorage/TemporalMessageStorage.mm | ||
---|---|---|
50–53 | The issue is that for an encrypted content we can expect more-or-less linear distribution of byte values. There are 256 byte values, so we should expect 1/256 chance of a byte being invalid, which gives about 40% chance of 128 bytes block having at least one invalid character. For a message with 1000 bytes, there's only 2% that it doesn't contain a separator. |
native/ios/Comm/TemporalMessageStorage/TemporalMessageStorage.mm | ||
---|---|---|
50–53 | One simple solution would be to base64 encode the ciphertext |
native/ios/Comm/TemporalMessageStorage/TemporalMessageStorage.mm | ||
---|---|---|
50–53 | Given the fact that according to TLS specification we are forced to use AES in CBC mode, the issue disappears. Encrypted data will be converted to base64 and separated with newline characters before being written to file so the if statement above will be removed upon very next update of this differential. | |
77 | Do you suggest to wrap [EncryptedFileUtils appendData:message toFileAtPath:self.mainStoragePath error:&err] inside a @try{} .. @catch{} ... @finally{} statement? I understand that doing so will not bring any harm, but appendData implementation already contains try catch statement around volatile file operations. |
native/ios/Comm/TemporaryMessageStorage/TemporaryMessageStorage.mm | ||
---|---|---|
47 ↗ | (On Diff #14277) | Previously when we were implementing a direct write to SQLite database we checked against existence of SQLite database and skipped notifications if is was not already created (even though we could create database in NotificationService). I decided to follow the same rules here and skip notifications before AppDelegate creates file for the first time. |
68–69 ↗ | (On Diff #14277) | What exactly do you mean? Do you mean something like: |
native/ios/Comm/TemporaryMessageStorage/TemporaryMessageStorage.mm | ||
---|---|---|
47 ↗ | (On Diff #14277) | Ok, that makes sense, but creating a new file in this case shouldn't cause any issues, right? The downside of the current implementation is that even after the app is updated to the version with this code, we need to wait with saving until the app is started for the first time. This doesn't sound necessary. |
68–69 ↗ | (On Diff #14277) | Yes, exactly |
native/ios/Comm/TemporaryMessageStorage/TemporaryMessageStorage.mm | ||
---|---|---|
47 ↗ | (On Diff #14277) | Ok, I will introduce file creation in write-operation. |
native/ios/Comm/TemporaryMessageStorage/TemporaryMessageStorage.mm | ||
---|---|---|
47 ↗ | (On Diff #14277) | On the other hand to decision to create new file is based on the result of contentsOfDirectoryAtPath call. We will now have to process that read the contents of the directory and then attempt to create new file. We can have a situation when AppDelegate reads the contents of directory, finds out that there is not file, but before it attempts to create the first one notification service also creates the first one and writes notification. When app delegate next tries to create a new file it will overwrite it contents with empty string. We would then need to introduce additional synchronization. I am not sure that it is worth it. |
native/ios/Comm/TemporaryMessageStorage/TemporaryMessageStorage.mm | ||
---|---|---|
47 ↗ | (On Diff #14277) |
Why would that happen? We're using a timestamp in the name, so it shouldn't be likely. Also, maybe there's a way for _updateCurrentStorage to only touch a file, so that if it didn't exist, it is created, but when it existed, nothing happens. Maybe you have some other ideas on how to avoid skipping when storageContent is empty? |
native/ios/Comm/TemporaryMessageStorage/TemporaryMessageStorage.mm | ||
---|---|---|
47 ↗ | (On Diff #14277) | It might happen since timestamp has precision up to 1 second, so two subsequent calls to _updateCurrentStorage in two processes (NotificationService and AppDelegate) might produce the same name. I will search for ways to create file only if it does not exists. It seems a best option for me. |
native/ios/Comm/TemporaryMessageStorage/TemporaryMessageStorage.mm | ||
---|---|---|
47 ↗ | (On Diff #14277) | This solution to this problem is simple and available from pure C: https://cpp0x.pl/dokumentacja/standard-C/fopen/446. Objective C function [NSFileManager.defaultManager createFileAtPath:<> contents:<> attributes:<> always overwrites file content, while fopen(<>, "a") will do nothing if the file exists. We can safely then use it to create storage file in NotificationService if it does not already exist. |
Use new method in write operation. Create storage file in write operation is not exists