Decryption buffer #2616
Decryption buffer #2616
Conversation
|
Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2616/1/Diff_Coverage |
|
Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2616/1/Performance_Report |
| // decrypt to a temporary buffer first because some implementations may | ||
| // require several iterations on a block before producing a final result | ||
| // and our shared mappings might be read by other readers at any time | ||
| crypt(mode_Decrypt, pos, m_dst_buffer.get(), m_rw_buffer.get(), reinterpret_cast<const char*>(&iv.iv1)); |
There was a problem hiding this comment.
I couldn't understand this... how about something along the lines of:
We may expect some address ranges of the destination buffer of AESCryptor::read() to stay unmodified, i.e. being overwritten with the same bytes as already present, and may have read-access to these from other threads while decrypting is taking place.
However, some implementations of AES_cbc_encrypt() (in particular OpenSSL) will intermediately put garbled bytes at the destination during operation which will lead to crashes.
We therefore decrypt to a temporary buffer which we memcpy() at the end.
|
Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2616/2/Performance_Report |
|
Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2616/2/Diff_Coverage |
The openssl implementation of AES_cbc_encrypt seems to use the destination as scratch and must make multiple iterations on a block before the decrypted result is produced. If we have multiple readers looking at the same mapping and a writer which updates that page in the file (at a later memory location) then one reader will find that the page is out of date and decrypt from the file again. This will work for the reader triggering the decryption, but the other reader that is using the same memory concurrently will have a chance to read the partially decrypted data.
The fix is to decrypt to a temporary buffer and copy the fully decrypted data afterwards. The memory is then guaranteed to be the same for any concurrent readers.
This explains why we can produce the bug on windows, but only when using the openssl implementation and not after switching to native windows encryption. It also explains why we never observed the bug on iOS devices, or on mac development computers. This may also explain why the last fix helped reduce these crashes (by not refreshing an already refreshed page) but that was a nice optimisation anyway.
The performance impact should not be noticeable. For example, running the whole unit test suite with and without the fix takes the same amount of time.
Possible fix for #2537