Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix several valgrind failures #7789

Merged
merged 1 commit into from
Jun 8, 2024
Merged

Fix several valgrind failures #7789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

### Fixed
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
* None.
* Valgrind could report a branch on an uninitialized read when opening something that is not an encrypted Realm file as an encrypted Realm file ([PR #7789](https://github.com/realm/realm-core/pull/7789), since v14.10.0).

### Breaking changes
* None.
Expand Down
8 changes: 4 additions & 4 deletions src/realm/util/encrypted_file_mapping.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ bool AESCryptor::constant_time_equals(const Hmac& a, const Hmac& b) const
{
// Constant-time memcmp to avoid timing attacks
uint8_t result = 0;
for (size_t i = 0; i < 224 / 8; ++i)
for (size_t i = 0; i < a.size(); ++i)
result |= a[i] ^ b[i];
return result == 0;
}
Expand All @@ -404,7 +404,7 @@ void AESCryptor::invalidate_ivs() noexcept
AESCryptor::ReadResult AESCryptor::read(FileDesc fd, SizeType pos, char* dst, WriteObserver* observer)
{
uint32_t iv = 0;
Hmac hmac{};
Hmac hmac;
// We're in a single-process scenario (or other processes are only reading),
// so we can trust our in-memory caches and never need to retry
if (!observer || observer->no_concurrent_writer_seen()) {
Expand Down Expand Up @@ -475,12 +475,12 @@ AESCryptor::ReadResult AESCryptor::attempt_read(FileDesc fd, SizeType pos, char*
IVTable& iv = get_iv_table(fd, pos, iv_mode);
iv_out = iv.iv1;
if (iv.iv1 == 0) {
std::fill(hmac.begin(), hmac.end(), 0);
hmac.fill(0);
return ReadResult::Uninitialized;
}

size_t actual = check_read(fd, data_pos_to_file_pos(pos), m_rw_buffer.get());
if (actual == 0) {
if (actual < encryption_page_size) {
return ReadResult::Eof;
}

Expand Down
6 changes: 3 additions & 3 deletions test/test_group.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ TEST(Group_OpenUnencryptedFileWithKey_TwoPages)
Group group;
TableRef table = group.get_or_add_table("table");
auto col = table->add_column(type_String, "str");
table->create_object().set(col, std::string(page_size() - 100, '\1'));
table->create_object().set(col, std::string(4096 - 100, '\1'));
group.write(path);
}

Expand All @@ -135,7 +135,7 @@ TEST(Group_OpenUnencryptedFileWithKey_ThreePages)
Group group;
TableRef table = group.get_or_add_table("table");
auto col = table->add_column(type_String, "str");
std::string data(page_size() - 100, '\1');
std::string data(4096 - 100, '\1');
table->create_object().set<String>(col, data);
table->create_object().set<String>(col, data);
group.write(path);
Expand Down Expand Up @@ -285,7 +285,7 @@ TEST(Group_TableNameTooLong)
{
Group group;
size_t buf_len = 64;
std::unique_ptr<char[]> buf(new char[buf_len]);
std::unique_ptr<char[]> buf(new char[buf_len]());
CHECK_LOGIC_ERROR(group.add_table(StringData(buf.get(), buf_len)), ErrorCodes::InvalidName);
group.add_table(StringData(buf.get(), buf_len - 1));
}
Expand Down
49 changes: 0 additions & 49 deletions test/valgrind.suppress
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,3 @@
{
<AllocSlab::all_files and AllocSlab::all_files_mutex intentionally not destructed to prevent races>
Memcheck:Leak
match-leak-kinds: reachable
fun:_Znwm
fun:_GLOBAL__sub_I_alloc_slab.cpp
fun:call_init.part.0
fun:call_init
fun:_dl_init
...
}
{
<Entries added to the AllocSlab::all_files map are not freed namely `p = std::make_shared<MappedFile>();`>
Memcheck:Leak
match-leak-kinds: reachable
fun:_Znwm
fun:_ZNSt8_Rb_treeISsSt4pairIKSsSt8weak_ptrIN5realm9SlabAlloc10MappedFileEEESt10_Select1stIS7_ESt4lessISsESaIS7_EE22_M_emplace_hint_uniqueIIRKSt21piecewise_construct_tSt5tupleIIRS1_EESI_IIEEEEESt17_Rb_tree_iteratorIS7_ESt23_Rb_tree_const_iteratorIS7_EDpOT_
fun:_ZN5realm9SlabAlloc11attach_fileERKSsRNS0_6ConfigE
fun:_ZN5realm5Group4openERKSsPKcNS0_8OpenModeE
...
}
{
<std weak pointer allocated for entries in AllocSlab::all_files as above>
Memcheck:Leak
match-leak-kinds: reachable
fun:_Znwm
fun:_ZNSt8_Rb_treeISsSt4pairIKSsSt8weak_ptrIN5realm9SlabAlloc10MappedFileEEESt10_Select1stIS7_ESt4lessISsESaIS7_EE22_M_emplace_hint_uniqueIIRKSt21piecewise_construct_tSt5tupleIIRS1_EESI_IIEEEEESt17_Rb_tree_iteratorIS7_ESt23_Rb_tree_const_iteratorIS7_EDpOT_
fun:_ZN5realm9SlabAlloc11attach_fileERKSsRNS0_6ConfigE
fun:_ZN5realm11SharedGroup7do_openERKSsbbNS_18SharedGroupOptionsE
fun:_ZN5realm11SharedGroupC2ERKSsbNS_18SharedGroupOptionsE.constprop.432
...
}
{
<We construct shared pointers with new. This covers two instances: `m_local_mappings.reset(new ...` and `new_mappings.reset(new ...`>
Memcheck:Leak
match-leak-kinds: reachable
fun:_Znwm
fun:_ZN5realm9SlabAlloc11attach_fileERKSsRNS0_6ConfigE
...
}
{
<SlabAlloc makes a deep copy of the realm file_path string which is still reachable as the keys of the all_files map>
Memcheck:Leak
fun:_Znwm
fun:_ZNSs4_Rep9_S_createEmmRKSaIcE
...
fun:_ZN5realm9SlabAlloc11attach_fileERKSsRNS0_6ConfigE
...
}
{
<Supposed valgrind false positives>
Memcheck:Leak
Expand Down
Loading