Skip to content

Commit

Permalink
work around a TSAN report during multiprocess encryption
Browse files Browse the repository at this point in the history
  • Loading branch information
ironage committed Nov 15, 2023
1 parent 0872968 commit a94e154
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 11 deletions.
23 changes: 21 additions & 2 deletions src/realm/util/encrypted_file_mapping.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,25 @@ size_t check_read(FileDesc fd, off_t pos, void* dst, size_t len)
return ret;
}

inline void memcpy_tsan_safe(char* dst, char* src, size_t num_bytes)
{
#if REALM_SANITIZE_THREAD
// Copying an entire page may overwrite arrays which is being read concurrently by
// another thread. This is benign because it overwrites data from old version with
// the same value as is already there, so the reader sees the correct value. This
// is all by design. However, when doing a memcpy of the entire page, TSAN sees
// this as a race even though we are replacing concurrently read data with the same
// values.
for (size_t i = 0; i < num_bytes; ++i) {
if (src[i] != dst[i]) {
dst[i] = src[i];
}
}
#else
memcpy(dst, src, num_bytes);
#endif
}

} // anonymous namespace

AESCryptor::AESCryptor(const uint8_t* key)
Expand Down Expand Up @@ -438,7 +457,7 @@ size_t AESCryptor::read(FileDesc fd, off_t pos, char* dst, size_t size, WriteObs
// We therefore decrypt to a temporary buffer first and then copy the
// completely decrypted data after.
crypt(mode_Decrypt, pos, m_dst_buffer.get(), m_rw_buffer.get(), reinterpret_cast<const char*>(&iv.iv1));
memcpy(dst, m_dst_buffer.get(), block_size);
memcpy_tsan_safe(dst, m_dst_buffer.get(), block_size);

pos += block_size;
dst += block_size;
Expand Down Expand Up @@ -630,7 +649,7 @@ bool EncryptedFileMapping::copy_up_to_date_page(size_t local_page_ndx) noexcept

size_t shadow_mapping_local_ndx = page_ndx_in_file - m->m_first_page;
if (is(m->m_page_state[shadow_mapping_local_ndx], UpToDate)) {
memcpy(page_addr(local_page_ndx), m->page_addr(shadow_mapping_local_ndx),
memcpy_tsan_safe(page_addr(local_page_ndx), m->page_addr(shadow_mapping_local_ndx),
static_cast<size_t>(1ULL << m_page_shift));
return true;
}
Expand Down
9 changes: 0 additions & 9 deletions test/tsan.suppress
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
# ThreadSanitizer suppressions file for realm-core

# `AESCryptor::read()` and`copy_up_to_date_page()` copy entire pages.
# They may overwrite something which
# is being read concurrently. The reason it is benign, is that whenever there is a
# race, it overwrites with the same value as is already there, so the reader sees
# the correct value. This is all by design.

race:realm::util::AESCryptor::read
race:realm::util::EncryptedFileMapping::copy_up_to_date_page

# Avoid a false positive instance of lock-order-inversion.
# SyncManager::m_sessions_mutex and SyncSession::m_state_mutex are locked
# in this order when a SyncSession is created, and in reverse order when
Expand Down

0 comments on commit a94e154

Please sign in to comment.