Fix MSAN issue in #1626 (#1654)

* Fix MSAN issue in #1626

This patch fixes an MSAN issue by changing CZString
initialization.

* add test

* expand tests

* fix build

* Export CZString with JSON_API to fix Windows DLL linker errors

Author: baylesj

include/json/value.h

@@ -50,6 +50,9 @@
 #include <string>
 #include <vector>
 
+// Forward declaration for testing.
+struct ValueTest;
+
 #ifdef JSONCPP_HAS_STRING_VIEW
 #include <string_view>
 #endif
@@ -201,6 +204,7 @@ class JSON_API StaticString {
  */
 class JSON_API Value {
   friend class ValueIteratorBase;
+  friend struct ::ValueTest;
 
 public:
   using Members = std::vector<String>;
@@ -266,7 +270,7 @@ class JSON_API Value {
 private:
 #endif
 #ifndef JSONCPP_DOC_EXCLUDE_IMPLEMENTATION
-  class CZString {
+  class JSON_API CZString {
   public:
     enum DuplicationPolicy { noDuplication = 0, duplicate, duplicateOnCopy };
     CZString(ArrayIndex index);

src/lib_json/json_value.cpp

@@ -253,20 +253,29 @@ Value::CZString::CZString(const CZString& other) {
   cstr_ = (other.storage_.policy_ != noDuplication && other.cstr_ != nullptr
                ? duplicateStringValue(other.cstr_, other.storage_.length_)
                : other.cstr_);
-  storage_.policy_ =
-      static_cast<unsigned>(
-          other.cstr_
-              ? (static_cast<DuplicationPolicy>(other.storage_.policy_) ==
-                         noDuplication
-                     ? noDuplication
-                     : duplicate)
-              : static_cast<DuplicationPolicy>(other.storage_.policy_)) &
-      3U;
-  storage_.length_ = other.storage_.length_;
-}
-
-Value::CZString::CZString(CZString&& other) noexcept
-    : cstr_(other.cstr_), index_(other.index_) {
+  if (other.cstr_) {
+    storage_.policy_ =
+        static_cast<unsigned>(
+            other.cstr_
+                ? (static_cast<DuplicationPolicy>(other.storage_.policy_) ==
+                           noDuplication
+                       ? noDuplication
+                       : duplicate)
+                : static_cast<DuplicationPolicy>(other.storage_.policy_)) &
+        3U;
+    storage_.length_ = other.storage_.length_;
+  } else {
+    index_ = other.index_;
+  }
+}
+
+Value::CZString::CZString(CZString&& other) noexcept : cstr_(other.cstr_) {
+  if (other.cstr_) {
+    storage_.policy_ = other.storage_.policy_;
+    storage_.length_ = other.storage_.length_;
+  } else {
+    index_ = other.index_;
+  }
   other.cstr_ = nullptr;
 }
 
@@ -292,8 +301,16 @@ Value::CZString& Value::CZString::operator=(const CZString& other) {
 }
 
 Value::CZString& Value::CZString::operator=(CZString&& other) noexcept {
+  if (cstr_ && storage_.policy_ == duplicate) {
+    releasePrefixedStringValue(const_cast<char*>(cstr_));
+  }
   cstr_ = other.cstr_;
-  index_ = other.index_;
+  if (other.cstr_) {
+    storage_.policy_ = other.storage_.policy_;
+    storage_.length_ = other.storage_.length_;
+  } else {
+    index_ = other.index_;
+  }
   other.cstr_ = nullptr;
   return *this;
 }

src/test_lib_json/main.cpp

@@ -150,6 +150,8 @@ struct ValueTest : JsonTest::TestCase {
   /// Normalize the representation of floating-point number by stripped leading
   /// 0 in exponent.
   static Json::String normalizeFloatingPointStr(const Json::String& s);
+
+  void runCZStringTests();
 };
 
 Json::String ValueTest::normalizeFloatingPointStr(const Json::String& s) {
@@ -167,6 +169,44 @@ Json::String ValueTest::normalizeFloatingPointStr(const Json::String& s) {
   return normalized + exponent;
 }
 
+void ValueTest::runCZStringTests() {
+  // 1. Copy Constructor (Index)
+  Json::Value::CZString idx1(123);
+  Json::Value::CZString idx2(idx1);
+  JSONTEST_ASSERT_EQUAL(idx2.index(), 123);
+
+  // 2. Move Constructor (Index)
+  Json::Value::CZString idx3(std::move(idx1));
+  JSONTEST_ASSERT_EQUAL(idx3.index(), 123);
+
+  // 3. Move Assignment (Index)
+  Json::Value::CZString idx4(456);
+  idx4 = std::move(idx3);
+  JSONTEST_ASSERT_EQUAL(idx4.index(), 123);
+
+  // 4. Copy Constructor (String)
+  Json::Value::CZString str1("param", 5,
+                             Json::Value::CZString::duplicateOnCopy);
+  Json::Value::CZString str2((str1)); // copy makes it duplicate (owning)
+  JSONTEST_ASSERT_STRING_EQUAL(str2.data(), "param");
+
+  // 5. Move Constructor (String)
+  // Move from Owning string (str2)
+  Json::Value::CZString str3(std::move(str2));
+  JSONTEST_ASSERT_STRING_EQUAL(str3.data(), "param");
+
+  // 6. Move Assignment (String)
+  Json::Value::CZString str4("other", 5,
+                             Json::Value::CZString::duplicateOnCopy);
+  Json::Value::CZString str5((str4)); // owning "other"
+  // Move-assign owning "param" (str3) into owning "other" (str5)
+  // This verifies we don't leak "other" (if fixed) and correctly take "param"
+  str5 = std::move(str3);
+  JSONTEST_ASSERT_STRING_EQUAL(str5.data(), "param");
+}
+
+JSONTEST_FIXTURE_LOCAL(ValueTest, CZStringCoverage) { runCZStringTests(); }
+
 JSONTEST_FIXTURE_LOCAL(ValueTest, checkNormalizeFloatingPointStr) {
   struct TestData {
     std::string in;
@@ -449,6 +489,24 @@ JSONTEST_FIXTURE_LOCAL(ValueTest, resizeArray) {
   }
 }
 
+JSONTEST_FIXTURE_LOCAL(ValueTest, copyMoveArray) {
+  Json::Value array;
+  array.append("item1");
+  array.append("item2");
+
+  // Test Copy Constructor (covers CZString(const CZString&) with index)
+  Json::Value copy(array);
+  JSONTEST_ASSERT_EQUAL(copy.size(), 2);
+  JSONTEST_ASSERT_EQUAL(Json::Value("item1"), copy[0]);
+  JSONTEST_ASSERT_EQUAL(Json::Value("item2"), copy[1]);
+
+  // Test Move Constructor (covers CZString(CZString&&) with index)
+  Json::Value moved(std::move(copy));
+  JSONTEST_ASSERT_EQUAL(moved.size(), 2);
+  JSONTEST_ASSERT_EQUAL(Json::Value("item1"), moved[0]);
+  JSONTEST_ASSERT_EQUAL(Json::Value("item2"), moved[1]);
+}
+
 JSONTEST_FIXTURE_LOCAL(ValueTest, resizePopulatesAllMissingElements) {
   Json::ArrayIndex n = 10;
   Json::Value v;