Fix races in/around encryption layer #3429
Conversation
…ealm/realm-core into fsa/sync-before-extending-write-window
|
Hey - looks like you forgot to add a T-* label - could you please add one (if you have access to add labels)? |
|
Is there any explanation for why these errors were mostly seen on Android or is that just by accident? |
|
@cmelchior It's because android uses a different method for extending the file (compared to ordinary Linux). |
…fore-extending-write-window
src/realm/alloc_slab.cpp
Outdated
| @@ -1384,7 +1384,10 @@ void SlabAlloc::resize_file(size_t new_file_size) | |||
| std::lock_guard<Mutex> lock(m_file_mappings->m_mutex); | |||
| REALM_ASSERT_EX(matches_section_boundary(new_file_size), new_file_size, get_file_path_for_assertions()); | |||
| m_file_mappings->m_file.prealloc(new_file_size); // Throws | |||
|
|
|||
| auto from_get_size = m_file_mappings->m_file.get_size(); | |||
There was a problem hiding this comment.
This should be within the REALM_ASSERT expression. Otherwise we will get an unused variable if asserts are off.
| @@ -2314,7 +2320,7 @@ void SharedGroup::low_level_commit(uint_fast64_t new_version) | |||
| // the cleanup process may access the entire ring buffer, so make sure it is mapped. | |||
| // this is not ensured as part of begin_read, which only makes sure that the current | |||
| // last entry in the buffer is available. | |||
| if (grow_reader_mapping(r_info->readers.get_num_entries())) { // throws | |||
| if (grow_reader_mapping(r_info->readers.get_num_entries() - 1)) { // throws | |||
There was a problem hiding this comment.
Was this the "off by one" error you mentioned? Perhaps add a comment why subtracting one as this was obviously not realized in the first place.
| @@ -867,7 +869,11 @@ void GroupWriter::commit(ref_type new_top_ref) | |||
| using type_1 = std::remove_reference<decltype(file_header.m_file_format[0])>::type; | |||
| REALM_ASSERT(!util::int_cast_has_overflow<type_1>(file_format_version)); | |||
| file_header.m_top_ref[slot_selector] = new_top_ref; | |||
| file_header.m_file_format[slot_selector] = type_1(file_format_version); | |||
| if (type_1(file_format_version) != file_header.m_file_format[slot_selector]) { | |||
There was a problem hiding this comment.
What does the condition express? In what cases is the conditon not met?
| @@ -878,7 +884,8 @@ void GroupWriter::commit(ref_type new_top_ref) | |||
|
|
|||
| // Make sure that that all data relating to the new snapshot is written to | |||
| // stable storage before flipping the slot selector | |||
| window->encryption_write_barrier(&file_header, sizeof file_header); | |||
| window->encryption_write_barrier(&file_header.m_top_ref[slot_selector], | |||
There was a problem hiding this comment.
In some cases we just called this function. Is it redundant to call it again?
There was a problem hiding this comment.
With the changes in the encryption layer, we want these calls to precisely mark where changes take place.
| memcpy(m->page_addr(shadow_local_page_ndx) + begin_offset, | ||
| page_addr(local_page_ndx) + begin_offset, | ||
| end_offset - begin_offset); | ||
| } else |
| for (size_t idx = first_accessed_local_page; idx <= last_accessed_local_page && idx < pages_size; ++idx) { | ||
| // Pages written must earlier on have been decrypted | ||
| // by a call to read_barrier(). | ||
| if (first_accessed_local_page < pages_size) { |
There was a problem hiding this comment.
Add a few comments. I assume we handle first, middle and last page.
|
We should refer to tests that validates this fixes the problem. |
Possible fix of #3427
Also fix for #3410
and #3419
Content: