From 4f84d9a94305175cea282f4ac70d809b3d246406 Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Thu, 15 Oct 2015 16:58:07 -0700 Subject: [PATCH 1/4] Use the correct top_ref when converting from streaming form When skip_validate is true, SlabAlloc::attach_file() always returned 0 for the top_ref. This was fine when it's not the session initiator as the return value was ignored in favor of the ref from the lock file, but SharedGroup::compact() actually uses the return value. --- release_notes.md | 3 ++- src/realm/alloc_slab.cpp | 4 ++++ test/test_shared.cpp | 7 +++++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/release_notes.md b/release_notes.md index 1e3b7db56f0..277d5e06a9b 100644 --- a/release_notes.md +++ b/release_notes.md @@ -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: diff --git a/src/realm/alloc_slab.cpp b/src/realm/alloc_slab.cpp index ef9326c6cf7..945bf8d468a 100644 --- a/src/realm/alloc_slab.cpp +++ b/src/realm/alloc_slab.cpp @@ -575,6 +575,10 @@ ref_type SlabAlloc::attach_file(const std::string& path, Config& cfg) m_file_on_streaming_form = false; writable_map.sync(); } + + if (cfg.skip_validate) { + top_ref = footer->m_top_ref; + } } fcg.release(); // Do not close diff --git a/test/test_shared.cpp b/test/test_shared.cpp index 2a6e8348b94..62c8c2c54b2 100644 --- a/test/test_shared.cpp +++ b/test/test_shared.cpp @@ -274,13 +274,20 @@ TEST(Shared_CompactingOnTheFly) sg2.commit(); } CHECK_EQUAL(true, sg2.compact()); + ReadTransaction rt2(sg2); + TestTableShared::ConstRef table = rt2.get_table("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("test"); + CHECK(table); + CHECK_EQUAL(table->size(), 100); rt2.get_group().verify(); } } From e76df14cd64f15a7f14f3f85066650e469ee335c Mon Sep 17 00:00:00 2001 From: Kristian Spangsege Date: Mon, 19 Oct 2015 18:33:30 +0200 Subject: [PATCH 2/4] SlabAlloc::attach_file() now always returns the top ref --- src/realm/alloc_slab.cpp | 75 +++++++++++++++++++++------------------- src/realm/alloc_slab.hpp | 29 +++++++++------- 2 files changed, 56 insertions(+), 48 deletions(-) diff --git a/src/realm/alloc_slab.cpp b/src/realm/alloc_slab.cpp index 945bf8d468a..585f49a1b3d 100644 --- a/src/realm/alloc_slab.cpp +++ b/src/realm/alloc_slab.cpp @@ -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. @@ -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. @@ -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 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
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; } else { - const Header* header = reinterpret_cast(map.get_addr()); - bool stored_server_sync_mode = (header->m_flags & flags_ServerSyncMode) != 0; + const Header& header = reinterpret_cast(*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); @@ -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(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(); @@ -544,41 +552,38 @@ 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(m_data); - static_cast(header); - + const Header& header = *reinterpret_cast(m_data); + const StreamingFooter& footer = + *(reinterpret_cast(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(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
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
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(); } - - if (cfg.skip_validate) { - top_ref = footer->m_top_ref; - } } fcg.release(); // Do not close @@ -594,9 +599,8 @@ 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 + ref_type top_ref = validate_buffer(data, size, path, is_shared); // Throws { const Header& header = reinterpret_cast(*data); @@ -636,8 +640,8 @@ 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) +ref_type SlabAlloc::validate_buffer(const char* data, size_t size, const std::string& path, + bool is_shared) { // Verify that size is sane and 8-byte aligned if (REALM_UNLIKELY(size < sizeof (Header) || size % 8 != 0)) @@ -682,7 +686,8 @@ void SlabAlloc::validate_buffer(const char* data, size_t size, const std::string if (REALM_UNLIKELY(ref >= size)) throw InvalidDatabase("Bad Realm file header (#3)", path); - top_ref = ref_type(ref); + ref_type top_ref = ref_type(ref); + return top_ref; } diff --git a/src/realm/alloc_slab.hpp b/src/realm/alloc_slab.hpp index d9d503f9176..ba3297a0a96 100644 --- a/src/realm/alloc_slab.hpp +++ b/src/realm/alloc_slab.hpp @@ -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 @@ -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. @@ -397,8 +398,10 @@ 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); + /// + /// \return The top ref as read from the header or footer. + ref_type validate_buffer(const char* data, size_t len, const std::string& path, + bool is_shared); class ChunkRefEq; class ChunkRefEndEq; From 277b27878aff4997889546f5343d228407ddfa40 Mon Sep 17 00:00:00 2001 From: Kristian Spangsege Date: Mon, 19 Oct 2015 18:59:53 +0200 Subject: [PATCH 3/4] Fix for: SlabAlloc::attach_file() now always returns the top ref --- src/realm/alloc_slab.cpp | 18 ++++++++++++------ src/realm/alloc_slab.hpp | 5 +---- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/realm/alloc_slab.cpp b/src/realm/alloc_slab.cpp index 585f49a1b3d..3ac9f2b1e67 100644 --- a/src/realm/alloc_slab.cpp +++ b/src/realm/alloc_slab.cpp @@ -600,14 +600,23 @@ ref_type SlabAlloc::attach_buffer(char* data, size_t size) // Verify the data structures std::string path; // No path bool is_shared = false; - ref_type top_ref = validate_buffer(data, size, path, is_shared); // Throws + validate_buffer(data, size, path, is_shared); // Throws + ref_type top_ref; { const Header& header = reinterpret_cast(*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(map.get_addr()+initial_size_of_file) - 1); + top_ref = ref_type(footer.m_top_ref); + } + else { + top_ref = ref_type(ref); + } } m_data = data; @@ -640,8 +649,8 @@ void SlabAlloc::attach_empty() } -ref_type SlabAlloc::validate_buffer(const char* data, size_t size, const std::string& path, - bool is_shared) +void SlabAlloc::validate_buffer(const char* data, size_t size, const std::string& path, + bool is_shared) { // Verify that size is sane and 8-byte aligned if (REALM_UNLIKELY(size < sizeof (Header) || size % 8 != 0)) @@ -685,9 +694,6 @@ ref_type SlabAlloc::validate_buffer(const char* data, size_t size, const std::st throw InvalidDatabase("Bad Realm file header (#2)", path); if (REALM_UNLIKELY(ref >= size)) throw InvalidDatabase("Bad Realm file header (#3)", path); - - ref_type top_ref = ref_type(ref); - return top_ref; } diff --git a/src/realm/alloc_slab.hpp b/src/realm/alloc_slab.hpp index ba3297a0a96..634cf074ddd 100644 --- a/src/realm/alloc_slab.hpp +++ b/src/realm/alloc_slab.hpp @@ -398,10 +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. - /// - /// \return The top ref as read from the header or footer. - ref_type validate_buffer(const char* data, size_t len, const std::string& path, - bool is_shared); + void validate_buffer(const char* data, size_t len, const std::string& path, bool is_shared); class ChunkRefEq; class ChunkRefEndEq; From 60652c4f6635e6e469ed9763b703b46d666adb6a Mon Sep 17 00:00:00 2001 From: Kristian Spangsege Date: Mon, 19 Oct 2015 19:02:14 +0200 Subject: [PATCH 4/4] Fix for: SlabAlloc::attach_file() now always returns the top ref --- src/realm/alloc_slab.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/realm/alloc_slab.cpp b/src/realm/alloc_slab.cpp index 3ac9f2b1e67..6ccd50e6a4e 100644 --- a/src/realm/alloc_slab.cpp +++ b/src/realm/alloc_slab.cpp @@ -610,8 +610,7 @@ ref_type SlabAlloc::attach_buffer(char* data, size_t size) 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(map.get_addr()+initial_size_of_file) - 1); + const StreamingFooter& footer = *(reinterpret_cast(data+size) - 1); top_ref = ref_type(footer.m_top_ref); } else {