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

Use the correct top_ref when converting from streaming form #1223

Merged
merged 4 commits into from
Oct 20, 2015
Merged

Use the correct top_ref when converting from streaming form #1223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4f84d9a
Use the correct top_ref when converting from streaming form
tgoyne Oct 15, 2015
e76df14
SlabAlloc::attach_file() now always returns the top ref
kspangsege Oct 19, 2015
277b278
Fix for: SlabAlloc::attach_file() now always returns the top ref
kspangsege Oct 19, 2015
60652c4
Fix for: SlabAlloc::attach_file() now always returns the top ref
kspangsege Oct 19, 2015
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
3 changes: 2 additions & 1 deletion release_notes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

### Bugfixes:

* Lorem ipsum.
* Fixed a bug that lead to SharedGroup::compact failing to attach to the newly
written file.

### API breaking changes:

Expand Down
76 changes: 45 additions & 31 deletions src/realm/alloc_slab.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,6 @@ ref_type SlabAlloc::attach_file(const std::string& path, Config& cfg)
size_t initial_size = m_initial_section_size;

size_t initial_size_of_file;
ref_type top_ref = 0;

// The size of a database file must not exceed what can be encoded in
// size_t.
Expand Down Expand Up @@ -483,7 +482,7 @@ ref_type SlabAlloc::attach_file(const std::string& path, Config& cfg)
// undefined behavior.
// The mapping of the first part of the file *must* be contiguous, because
// we do not know if the file was created by a version of the code, that took
// the section boundaries into account. If it wasn't we cannot map it in sections
// the section boundaries into account. If it wasn't we cannot map it in sections
// without risking datastructures that cross a mapping boundary.
// If the file is opened read-only, we cannot extend it. This is not a problem,
// because for a read-only file we assume that it will not change while we use it.
Expand All @@ -500,22 +499,23 @@ ref_type SlabAlloc::attach_file(const std::string& path, Config& cfg)
// actual size of the file.
}

ref_type top_ref;
try {
File::Map<char> map(m_file, File::access_ReadOnly, size); // Throws

if (!cfg.skip_validate) {
// Verify the data structures
validate_buffer(map.get_addr(), initial_size_of_file, path, top_ref, cfg.is_shared); // Throws
validate_buffer(map.get_addr(), initial_size_of_file, path, cfg.is_shared); // Throws
}

if (did_create) {
File::Map<Header> writable_map(m_file, File::access_ReadWrite, sizeof (Header)); // Throws
Header* header = writable_map.get_addr();
header->m_flags |= cfg.server_sync_mode ? flags_ServerSyncMode : 0x0;
Header& header = *writable_map.get_addr();
header.m_flags |= cfg.server_sync_mode ? flags_ServerSyncMode : 0x0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about explicitly setting top_ref in this branch of the if as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you misread the code here. Note that top_ref is already set in both cases, because it is set after the end of the else-block.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intention is certainly that top_ref must be set in every case, so let me know if you see any case where it fails.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as I can see it is not set when did_create is true?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it. The closing-opening brace after the first condition confused me. I had to open it in the editor to understand the code :-)

}
else {
const Header* header = reinterpret_cast<const Header*>(map.get_addr());
bool stored_server_sync_mode = (header->m_flags & flags_ServerSyncMode) != 0;
const Header& header = reinterpret_cast<const Header&>(*map.get_addr());
bool stored_server_sync_mode = (header.m_flags & flags_ServerSyncMode) != 0;
if (cfg.server_sync_mode && !stored_server_sync_mode)
throw InvalidDatabase("Specified Realm file was not created with support for "
"client/server synchronization", path);
Expand All @@ -530,6 +530,14 @@ ref_type SlabAlloc::attach_file(const std::string& path, Config& cfg)
m_file_format = header.m_file_format[select_field];
uint_fast64_t ref = uint_fast64_t(header.m_top_ref[select_field]);
m_file_on_streaming_form = (select_field == 0 && ref == 0xFFFFFFFFFFFFFFFFULL);
if (m_file_on_streaming_form) {
const StreamingFooter& footer =
*(reinterpret_cast<StreamingFooter*>(map.get_addr()+initial_size_of_file) - 1);
top_ref = ref_type(footer.m_top_ref);
}
else {
top_ref = ref_type(ref);
}
}

m_data = map.release();
Expand All @@ -544,34 +552,35 @@ ref_type SlabAlloc::attach_file(const std::string& path, Config& cfg)
throw InvalidDatabase("Realm file decryption failed", path);
}

// make sure that any call to begin_read cause any slab to be placed in free lists correctly
// make sure that any call to begin_read cause any slab to be placed in free
// lists correctly
m_free_space_state = free_space_Invalid;

// make sure the database is not on streaming format. This has to be done at
// session initialization, even if it means writing the database during open.
if (cfg.session_initiator && m_file_on_streaming_form) {

Header* header = reinterpret_cast<Header*>(m_data);
static_cast<void>(header);

const Header& header = *reinterpret_cast<Header*>(m_data);
const StreamingFooter& footer =
*(reinterpret_cast<StreamingFooter*>(m_data+initial_size_of_file) - 1);
// Don't compare file format version fields as they are allowed to differ.
// Also don't compare reserved fields (todo, is it correct to ignore?)
REALM_ASSERT_3(header->m_flags, == , streaming_header.m_flags);
REALM_ASSERT_3(header->m_mnemonic[0], == , streaming_header.m_mnemonic[0]);
REALM_ASSERT_3(header->m_mnemonic[1], == , streaming_header.m_mnemonic[1]);
REALM_ASSERT_3(header->m_mnemonic[2], == , streaming_header.m_mnemonic[2]);
REALM_ASSERT_3(header->m_mnemonic[3], == , streaming_header.m_mnemonic[3]);
REALM_ASSERT_3(header->m_top_ref[0], == , streaming_header.m_top_ref[0]);
REALM_ASSERT_3(header->m_top_ref[1], == , streaming_header.m_top_ref[1]);

StreamingFooter* footer = reinterpret_cast<StreamingFooter*>(m_data+initial_size_of_file) - 1;
REALM_ASSERT_3(footer->m_magic_cookie, ==, footer_magic_cookie);
REALM_ASSERT_3(header.m_flags, == , streaming_header.m_flags);
REALM_ASSERT_3(header.m_mnemonic[0], == , streaming_header.m_mnemonic[0]);
REALM_ASSERT_3(header.m_mnemonic[1], == , streaming_header.m_mnemonic[1]);
REALM_ASSERT_3(header.m_mnemonic[2], == , streaming_header.m_mnemonic[2]);
REALM_ASSERT_3(header.m_mnemonic[3], == , streaming_header.m_mnemonic[3]);
REALM_ASSERT_3(header.m_top_ref[0], == , streaming_header.m_top_ref[0]);
REALM_ASSERT_3(header.m_top_ref[1], == , streaming_header.m_top_ref[1]);

REALM_ASSERT_3(footer.m_magic_cookie, ==, footer_magic_cookie);
{
File::Map<Header> writable_map(m_file, File::access_ReadWrite, sizeof (Header)); // Throws
Header* writable_header = writable_map.get_addr();
writable_header->m_top_ref[1] = footer->m_top_ref;
File::Map<Header> writable_map(m_file, File::access_ReadWrite,
sizeof (Header)); // Throws
Header& writable_header = *writable_map.get_addr();
writable_header.m_top_ref[1] = footer.m_top_ref;
writable_map.sync();
writable_header->m_flags |= flags_SelectBit; // keep bit 1 used for server sync mode unchanged
// keep bit 1 used for server sync mode unchanged
writable_header.m_flags |= flags_SelectBit;
m_file_on_streaming_form = false;
writable_map.sync();
}
Expand All @@ -590,16 +599,23 @@ ref_type SlabAlloc::attach_buffer(char* data, size_t size)

// Verify the data structures
std::string path; // No path
ref_type top_ref;
bool is_shared = false;
validate_buffer(data, size, path, top_ref, is_shared); // Throws
validate_buffer(data, size, path, is_shared); // Throws

ref_type top_ref;
{
const Header& header = reinterpret_cast<const Header&>(*data);
int select_field = ((header.m_flags & SlabAlloc::flags_SelectBit) != 0 ? 1 : 0);
m_file_format = header.m_file_format[select_field];
uint_fast64_t ref = uint_fast64_t(header.m_top_ref[select_field]);
m_file_on_streaming_form = (select_field == 0 && ref == 0xFFFFFFFFFFFFFFFFULL);
if (m_file_on_streaming_form) {
const StreamingFooter& footer = *(reinterpret_cast<StreamingFooter*>(data+size) - 1);
top_ref = ref_type(footer.m_top_ref);
}
else {
top_ref = ref_type(ref);
}
}

m_data = data;
Expand Down Expand Up @@ -633,7 +649,7 @@ void SlabAlloc::attach_empty()


void SlabAlloc::validate_buffer(const char* data, size_t size, const std::string& path,
ref_type& top_ref, bool is_shared)
bool is_shared)
{
// Verify that size is sane and 8-byte aligned
if (REALM_UNLIKELY(size < sizeof (Header) || size % 8 != 0))
Expand Down Expand Up @@ -677,8 +693,6 @@ void SlabAlloc::validate_buffer(const char* data, size_t size, const std::string
throw InvalidDatabase("Bad Realm file header (#2)", path);
if (REALM_UNLIKELY(ref >= size))
throw InvalidDatabase("Bad Realm file header (#3)", path);

top_ref = ref_type(ref);
}


Expand Down
26 changes: 13 additions & 13 deletions src/realm/alloc_slab.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,18 @@ class SlabAlloc: public Allocator {

~SlabAlloc() noexcept override;
SlabAlloc();

struct Config {
bool is_shared = false;
bool read_only = false;
bool no_create = false;
bool skip_validate = false;
bool server_sync_mode = false;
bool session_initiator = false;
bool clear_file = false;
const char* encryption_key = 0;
};

/// Attach this allocator to the specified file.
///
/// When used by free-standing Group instances, no concurrency is
Expand Down Expand Up @@ -120,17 +132,6 @@ class SlabAlloc: public Allocator {
/// \return The `ref` of the root node, or zero if there is none.
///
/// \throw util::File::AccessError
struct Config {
bool is_shared = false;
bool read_only = false;
bool no_create = false;
bool skip_validate = false;
bool server_sync_mode = false;
bool session_initiator = false;
bool clear_file = false;
const char* encryption_key = 0;
};

ref_type attach_file(const std::string& path, Config& cfg);

/// Attach this allocator to the specified memory buffer.
Expand Down Expand Up @@ -397,8 +398,7 @@ class SlabAlloc: public Allocator {
/// Throws InvalidDatabase if the file is not a Realm file, if the file is
/// corrupted, or if the specified encryption key is incorrect. This
/// function will not detect all forms of corruption, though.
void validate_buffer(const char* data, size_t len, const std::string& path,
ref_type& top_ref, bool is_shared);
void validate_buffer(const char* data, size_t len, const std::string& path, bool is_shared);

class ChunkRefEq;
class ChunkRefEndEq;
Expand Down
7 changes: 7 additions & 0 deletions test/test_shared.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,13 +274,20 @@ TEST(Shared_CompactingOnTheFly)
sg2.commit();
}
CHECK_EQUAL(true, sg2.compact());

ReadTransaction rt2(sg2);
TestTableShared::ConstRef table = rt2.get_table<TestTableShared>("test");
CHECK(table);
CHECK_EQUAL(table->size(), 100);
rt2.get_group().verify();
sg2.close();
}
{
SharedGroup sg2(path, true, SharedGroup::durability_Full, crypt_key());
ReadTransaction rt2(sg2);
TestTableShared::ConstRef table = rt2.get_table<TestTableShared>("test");
CHECK(table);
CHECK_EQUAL(table->size(), 100);
rt2.get_group().verify();
}
}
Expand Down