RNET-1141 multiprocess encryption for writers with different page sizes #7689
Conversation
ironage
force-pushed
the
js/encryption-upgrade-crash
branch
from
e024f60 to
a321722
Compare
Pull Request Test Coverage Report for Build james.stone_543Details
💛 - Coveralls |
without assuming that the page size is the same across devices. In particular, we now count reading blocks of 0's as a valid block rather than returning early. This supports data patterns of zero'd blocks between two valid data blocks instead of always assuming that the zero'd blocks are at the end.
ironage
force-pushed
the
js/encryption-upgrade-crash
branch
from
a321722 to
dbccedd
Compare
| size_t i; | ||
| for (i = 0; i < block_size; ++i) { | ||
| if (m_rw_buffer[i] != 0) { | ||
| break; | ||
| } | ||
| } | ||
| if (i != block_size) { | ||
| // at least one byte wasn't zero | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
| size_t i; | |
| for (i = 0; i < block_size; ++i) { | |
| if (m_rw_buffer[i] != 0) { | |
| break; | |
| } | |
| } | |
| if (i != block_size) { | |
| // at least one byte wasn't zero | |
| return false; | |
| } | |
| return true; | |
| for (size_t i = 0; i < block_size; ++i) { | |
| if (m_rw_buffer[i] != 0) { | |
| return false; | |
| } | |
| } | |
| return true; |
| // space. The code using this doesn't rely on pre-allocated | ||
| // space being zeroed, but we memset anyways to encourage out of bounds | ||
| // reads to error out rather than reading stale data. | ||
| memset(dst, '\0', block_size); |
There was a problem hiding this comment.
We don't zero-fill specifically to avoid masking errors. This makes it so that valgrind etc. can no longer report reads of uninitialized values, and zero specifically has an unfortunate tendency to work by coincidence. In other spots here we fill with 0x55 to reduce the chance of uninitialized data being accidentally valid.
There was a problem hiding this comment.
valid points, I'll change this and document it
|
Yes, good intuition! The middle block of zeros is indeed within a freeblock. Does that suggest something suspicious with the allocation pattern?
|
|
It's just another thing that had to be true for this to be a file in a valid-but-weird state. It does seem like this file was the result of the allocator just never using that space for whatever reason and everything else behaved correctly. Digging into the allocator to figure out why it could have done this might be useful, but it's also entirely plausible that there's some reasonable explanation for its behavior rather than it being a weird lurking bug. |
|
It currently doesn't work, but when it does I think #7698 will incidentally fix this by making us just not care about page size at all in the encryption code. |
|
Superseded due to #7698 |
Fixes realm/realm-dotnet#3592
A customer sent us an encrypted Realm that had an unusual data pattern in it:
This caused a Decryption failed exception when trying to access ref 16384 (block 5), because we could only read 4096 bytes, but 16k was requested. (The file can be read by faking a page size of 4k, but here I have a 16k page size).
I was not able to come up with a test case to create a hole in the data like our customer had, but I can create a close scenario with zeros at the end, by careful writes from a db with 16k page size and a db with 4k page size (see tests).
The fix is to change from returning early to allowing a block with all zeros as a valid read. The assumption still being that these blocks can only be created by fallocate extending the file.
I don't know how we could end up with zero'd blocks as holes in the middle of the file unless something wacky is going on with our allocator, but the changes here at least do allow us to read Realms with this kind of data pattern.
☑️ ToDos
bindgen/spec.yml, if public C++ API changed