Support nulls in String compare, CreateSharedString (#5060)

include/flatbuffers/flatbuffers.h

@@ -344,6 +344,14 @@ template<typename T> static inline size_t VectorLength(const Vector<T> *v) {
   return v ? v->Length() : 0;
 }
 
+// Lexicographically compare two strings (possibly containing nulls), and
+// return true if the first is less than the second.
+static inline bool StringLessThan(const char *a_data, uoffset_t a_size,
+                                  const char *b_data, uoffset_t b_size) {
+  const auto cmp = memcmp(a_data, b_data, (std::min)(a_size, b_size));
+  return cmp == 0 ? a_size < b_size : cmp < 0;
+}
+
 struct String : public Vector<char> {
   const char *c_str() const { return reinterpret_cast<const char *>(Data()); }
   std::string str() const { return std::string(c_str(), Length()); }
@@ -357,7 +365,7 @@ struct String : public Vector<char> {
   // clang-format on
 
   bool operator<(const String &o) const {
-    return strcmp(c_str(), o.c_str()) < 0;
+    return StringLessThan(this->data(), this->size(), o.data(), o.size());
   }
 };
 
@@ -1834,8 +1842,8 @@ protected:
     bool operator()(const Offset<String> &a, const Offset<String> &b) const {
       auto stra = reinterpret_cast<const String *>(buf_->data_at(a.o));
       auto strb = reinterpret_cast<const String *>(buf_->data_at(b.o));
-      return strncmp(stra->c_str(), strb->c_str(),
-                     (std::min)(stra->size(), strb->size()) + 1) < 0;
+      return StringLessThan(stra->data(), stra->size(),
+                            strb->data(), strb->size());
     }
     const vector_downward *buf_;
   };

tests/test.cpp

@@ -2376,6 +2376,58 @@ void LoadVerifyBinaryTest() {
   }
 }
 
+void CreateSharedStringTest() {
+  flatbuffers::FlatBufferBuilder builder;
+  const auto one1 = builder.CreateSharedString("one");
+  const auto two = builder.CreateSharedString("two");
+  const auto one2 = builder.CreateSharedString("one");
+  TEST_EQ(one1.o, one2.o);
+  const auto onetwo = builder.CreateSharedString("onetwo");
+  TEST_EQ(onetwo.o != one1.o, true);
+  TEST_EQ(onetwo.o != two.o, true);
+
+  // Support for embedded nulls
+  const char chars_b[] = {'a', '\0', 'b'};
+  const char chars_c[] = {'a', '\0', 'c'};
+  const auto null_b1 = builder.CreateSharedString(chars_b, sizeof(chars_b));
+  const auto null_c = builder.CreateSharedString(chars_c, sizeof(chars_c));
+  const auto null_b2 = builder.CreateSharedString(chars_b, sizeof(chars_b));
+  TEST_EQ(null_b1.o != null_c.o, true); // Issue#5058 repro
+  TEST_EQ(null_b1.o, null_b2.o);
+
+  // Put the strings into an array for round trip verification.
+  const flatbuffers::Offset<flatbuffers::String> array[7] = { one1, two, one2, onetwo, null_b1, null_c, null_b2 };
+  const auto vector_offset = builder.CreateVector(array, flatbuffers::uoffset_t(7));
+  MonsterBuilder monster_builder(builder);
+  monster_builder.add_name(two);
+  monster_builder.add_testarrayofstring(vector_offset);
+  builder.Finish(monster_builder.Finish());
+
+  // Read the Monster back.
+  const auto *monster = flatbuffers::GetRoot<Monster>(builder.GetBufferPointer());
+  TEST_EQ_STR(monster->name()->c_str(), "two");
+  const auto *testarrayofstring = monster->testarrayofstring();
+  TEST_EQ(testarrayofstring->size(), flatbuffers::uoffset_t(7));
+  const auto &a = *testarrayofstring;
+  TEST_EQ_STR(a[0]->c_str(), "one");
+  TEST_EQ_STR(a[1]->c_str(), "two");
+  TEST_EQ_STR(a[2]->c_str(), "one");
+  TEST_EQ_STR(a[3]->c_str(), "onetwo");
+  TEST_EQ(a[4]->str(), (std::string(chars_b, sizeof(chars_b))));
+  TEST_EQ(a[5]->str(), (std::string(chars_c, sizeof(chars_c))));
+  TEST_EQ(a[6]->str(), (std::string(chars_b, sizeof(chars_b))));
+
+  // Make sure String::operator< works, too, since it is related to StringOffsetCompare.
+  TEST_EQ((*a[0]) < (*a[1]), true);
+  TEST_EQ((*a[1]) < (*a[0]), false);
+  TEST_EQ((*a[1]) < (*a[2]), false);
+  TEST_EQ((*a[2]) < (*a[1]), true);
+  TEST_EQ((*a[4]) < (*a[3]), true);
+  TEST_EQ((*a[5]) < (*a[4]), false);
+  TEST_EQ((*a[5]) < (*a[4]), false);
+  TEST_EQ((*a[6]) < (*a[5]), true);
+}
+
 int FlatBufferTests() {
   // clang-format off
   #if defined(FLATBUFFERS_MEMORY_LEAK_TRACKING) && \
@@ -2445,6 +2497,7 @@ int FlatBufferTests() {
   ParseProtoBufAsciiTest();
   TypeAliasesTest();
   EndianSwapTest();
+  CreateSharedStringTest();
   JsonDefaultTest();
   FlexBuffersTest();
   UninitializedVectorTest();