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
diff --git a/include/json/value.h b/include/json/value.h
index 5f65443..a7a39a1 100644
--- a/include/json/value.h
+++ b/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 Value {
friend class ValueIteratorBase;
+ friend struct ::ValueTest;
public:
using Members = std::vector<String>;
@@ -266,7 +270,7 @@
private:
#endif
#ifndef JSONCPP_DOC_EXCLUDE_IMPLEMENTATION
- class CZString {
+ class JSON_API CZString {
public:
enum DuplicationPolicy { noDuplication = 0, duplicate, duplicateOnCopy };
CZString(ArrayIndex index);
diff --git a/src/lib_json/json_value.cpp b/src/lib_json/json_value.cpp
index a875d28..74f7789 100644
--- a/src/lib_json/json_value.cpp
+++ b/src/lib_json/json_value.cpp
@@ -253,20 +253,29 @@
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_;
+ 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_), 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=(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;
}
diff --git a/src/test_lib_json/main.cpp b/src/test_lib_json/main.cpp
index f19ca2f..d6938c1 100644
--- a/src/test_lib_json/main.cpp
+++ b/src/test_lib_json/main.cpp
@@ -150,6 +150,8 @@
/// 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 @@
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, 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;