Trying to intern a string multiple times may lead to duplicates

CHANGELOG.md

@@ -12,6 +12,7 @@
 * Subscription names correctly distinguish an empty string from a nullptr ([#5160](https://github.com/realm/realm-core/pull/5160), since v11.8.0)
 * Converting floats/doubles into Decimal128 would yield imprecise results ([#5184](https://github.com/realm/realm-core/pull/5184), since v6.1.0)
 * Fix some warnings when building with Xcode 13.3.
+* Using accented characters in class and field names may end the session ([#5196](https://github.com/realm/realm-core/pull/5196), since v10.2.0)
 
 ### Breaking changes
 * None.

src/realm/string_data.hpp

@@ -166,7 +166,7 @@ class StringData {
     explicit operator bool() const noexcept;
     explicit operator std::string_view() const noexcept
     {
-        return std::string_view(m_data);
+        return std::string_view(m_data, m_size);
     }
 
     /// If the StringData is NULL, the hash is 0. Otherwise, the function

src/realm/sync/changeset_encoder.cpp

@@ -224,12 +224,14 @@ void ChangesetEncoder::operator()(const Instruction::SetErase& instr)
 
 InternString ChangesetEncoder::intern_string(StringData str)
 {
-    auto it = m_intern_strings_rev.find(str);
+    auto it = m_intern_strings_rev.find(static_cast<std::string_view>(str));
     if (it == m_intern_strings_rev.end()) {
         size_t index = m_intern_strings_rev.size();
         // FIXME: Assert might be able to be removed after refactoring of changeset_parser types?
         REALM_ASSERT_RELEASE_EX(index <= std::numeric_limits<uint32_t>::max(), index);
-        it = m_intern_strings_rev.insert({std::string{str}, uint32_t(index)}).first;
+        bool inserted;
+        std::tie(it, inserted) = m_intern_strings_rev.insert({std::string{str}, uint32_t(index)});
+        REALM_ASSERT_RELEASE_EX(inserted, str);
 
         StringBufferRange range = add_string_range(str);
         set_intern_string(uint32_t(index), range);
@@ -248,7 +250,7 @@ void ChangesetEncoder::set_intern_string(uint32_t index, StringBufferRange range
 
 StringBufferRange ChangesetEncoder::add_string_range(StringData data)
 {
-    m_string_range = data;
+    m_string_range = static_cast<std::string_view>(data);
     REALM_ASSERT(data.size() <= std::numeric_limits<uint32_t>::max());
     return StringBufferRange{0, uint32_t(data.size())};
 }

src/realm/sync/changeset_encoder.hpp

@@ -70,7 +70,7 @@ struct ChangesetEncoder : InstructionHandler {
 
     Buffer m_buffer;
     util::metered::map<std::string, uint32_t> m_intern_strings_rev;
-    StringData m_string_range;
+    std::string_view m_string_range;
 };
 
 // Implementation

src/realm/sync/changeset_parser.cpp

@@ -30,6 +30,9 @@ struct ChangesetParser::State {
 
     StringBuffer m_buffer;
     util::metered::set<uint32_t> m_valid_interned_strings;
+    // Cannot use StringData as key type since m_input_begin may start pointing
+    // to a new chunk of memory.
+    util::metered::set<std::string> m_intern_strings;
 
 
     void parse_one(); // Throws
@@ -290,9 +293,18 @@ void ChangesetParser::State::parse_one()
     if (t == InstrTypeInternString) {
         uint32_t index = read_int<uint32_t>();
         StringData str = read_string();
+        auto it1 = m_intern_strings.find(static_cast<std::string_view>(str));
+        if (it1 != m_intern_strings.end()) {
+            parser_error("Unexpected intern string");
+        }
+        auto it2 = m_valid_interned_strings.find(index);
+        if (it2 != m_valid_interned_strings.end()) {
+            parser_error("Unexpected intern index");
+        }
         StringBufferRange range = m_handler.add_string_range(str);
         m_handler.set_intern_string(index, range);
         m_valid_interned_strings.emplace(index);
+        m_intern_strings.emplace(std::string{str});
         return;
     }
 

test/test_changeset_encoding.cpp

@@ -259,3 +259,21 @@ TEST(ChangesetEncoding_Clear)
     CHECK_EQUAL(changeset, parsed);
     CHECK(**changeset.begin() == instr);
 }
+
+TEST(ChangesetEncoding_AccentWords)
+{
+    sync::ChangesetEncoder encoder;
+
+    encoder.intern_string("Prógram");
+    encoder.intern_string("Program");
+    // Bug #5193 caused "Program" to not be found as an intern string
+    // although it was just created before.
+    encoder.intern_string("Program");
+    auto& buffer = encoder.buffer();
+
+    using realm::_impl::SimpleNoCopyInputStream;
+    SimpleNoCopyInputStream stream{buffer.data(), buffer.size()};
+    Changeset parsed;
+    // This will throw if a string is interned twice.
+    CHECK_NOTHROW(parse_changeset(stream, parsed));
+}

test/test_sync.cpp

@@ -5733,6 +5733,56 @@ TEST(Sync_BadChangeset)
 }
 
 
+TEST(Sync_GoodChangeset_AccentCharacterInFieldName)
+{
+    TEST_DIR(dir);
+    TEST_CLIENT_DB(db);
+
+    bool did_fail = false;
+    {
+        ClientServerFixture::Config config;
+        config.disable_upload_compaction = true;
+        ClientServerFixture fixture(dir, test_context, std::move(config));
+        fixture.start();
+
+        {
+            Session session = fixture.make_bound_session(db);
+            session.wait_for_download_complete_or_client_stopped();
+        }
+
+        {
+            WriteTransaction wt(db);
+            TableRef table = wt.add_table("class_table");
+            table->add_column(type_Int, "prógram");
+            table->add_column(type_Int, "program");
+            auto obj = table->create_object();
+            obj.add_int("program", 42);
+            wt.commit();
+        }
+
+        auto listener = [&](ConnectionState state, const Session::ErrorInfo* error_info) {
+            if (state != ConnectionState::disconnected)
+                return;
+            REALM_ASSERT(error_info);
+            std::error_code ec = error_info->error_code;
+            bool is_fatal = error_info->is_fatal;
+            CHECK_EQUAL(sync::ProtocolError::bad_changeset, ec);
+            CHECK(is_fatal);
+            fixture.stop();
+            did_fail = true;
+        };
+
+        Session session = fixture.make_session(db);
+        session.set_connection_state_change_listener(listener);
+        fixture.bind_session(session, "/test");
+
+        session.wait_for_upload_complete_or_client_stopped();
+        session.wait_for_download_complete_or_client_stopped();
+    }
+    CHECK_NOT(did_fail);
+}
+
+
 namespace issue2104 {
 
 class IntegrationReporter : public _impl::ServerHistory::IntegrationReporter {