From 5d540c6a66bc4b0e3fd3f376f338a8fe1bbee8b4 Mon Sep 17 00:00:00 2001 From: Dusk_NM02 Date: Sun, 19 Jul 2026 13:27:29 +0800 Subject: [PATCH] =?UTF-8?q?fix(writer):=20owning=20value=20move=20?= =?UTF-8?q?=E6=94=B9=E8=B5=B0=20doc=20=E6=89=80=E6=9C=89=E6=9D=83=E8=BD=AC?= =?UTF-8?q?=E7=A7=BB?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix(writer): owning value move transfers doc ownership owning yyjson::value 的 operator=(value&&) 之前对 owning 和 reference 共用一套 set_value(内容覆盖 + 挂 children 保活)。但 owning 被 move 之后 val_ 还指向已经 转走的 doc 节点(悬挂)、has_parent_ 被清空,这时走 set_value 既写野指针(dst->tag) 又解引用空 shared_ptr。owning value 因此不满足 std::movable,塞进 vector 排个序 就崩。 现在 owning 的 move-assign 直接转移 doc_/val_/has_parent_ 三成员(doc 所有权转移), reference value 仍走 set_value(node 编辑语义)。get_has_parent() 顺手对空 shared_ptr 做了保护。count_children 几条断言按 doc 转移后的实际值调整(owning move-assign 不 再往 children 挂 src doc)。 新增 test_owning_value_move,覆盖 moved-from 重新赋值和 vector 排序。 operator=(value&&) previously shared one set_value between owning and reference values (content overwrite + keep src doc alive via dst.children). After a move, an owning value's val_ still points into the already-transferred doc (dangling) and has_parent_ is emptied, so set_value writes a dangling pointer and dereferences an empty shared_ptr. owning value thus isn't std::movable, and sorting a vector crashes. Now owning move-assign transfers doc_/val_/has_parent_ directly; reference values keep set_value. get_has_parent() guards empty shared_ptr. count_children assertions adjusted to post-transfer values. Adds test_owning_value_move. --- include/cpp_yyjson.hpp | 20 ++++++++- test/CMakeLists.txt | 5 +++ test/test.cpp | 8 ++-- test/test_owning_value_move.cpp | 76 +++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 test/test_owning_value_move.cpp diff --git a/include/cpp_yyjson.hpp b/include/cpp_yyjson.hpp index dbcf356..844c1b4 100644 --- a/include/cpp_yyjson.hpp +++ b/include/cpp_yyjson.hpp @@ -1666,7 +1666,12 @@ namespace yyjson auto& get_has_parent() & noexcept { if constexpr (is_value_type) + { + // moved-from 对象的 has_parent_(shared_ptr)可能被移空; + // 防御性重建,避免解引用空 shared_ptr 导致 null-deref。 + if (!has_parent_) has_parent_ = std::make_shared(false); return *has_parent_; + } else return has_parent_; } @@ -2014,7 +2019,20 @@ namespace yyjson } mutable_value_base& operator=(mutable_value_base&& t) noexcept { - base::doc_.set_value(base::val_, std::move(t), base::get_has_parent()); + if constexpr (base::is_value_type) + { + // owning value 的 move = 整棵 doc 所有权转移。 + // set_value 的"内容覆盖 + 挂 children 保活"是 reference value 的 node + // 编辑语义,owning 走它会让 moved-from val_ 悬挂、has_parent_ 空,且 + // 每次 move 把 src doc 塞 dst.children 累积。reference value 仍走 set_value。 + base::doc_ = std::move(t.doc_); + base::val_ = t.val_; + base::has_parent_ = std::move(t.has_parent_); + } + else + { + base::doc_.set_value(base::val_, std::move(t), base::get_has_parent()); + } return *this; } template diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 67af9e4..f85f5d5 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -80,6 +80,11 @@ target_link_libraries("${PROJECT_NAME}_test_value_cast" PRIVATE ${PROJECT_NAME} GTest::gtest GTest::gtest_main) gtest_discover_tests("${PROJECT_NAME}_test_value_cast" DISCOVERY_MODE PRE_TEST) +add_executable("${PROJECT_NAME}_test_owning_value_move" test_owning_value_move.cpp) +target_link_libraries("${PROJECT_NAME}_test_owning_value_move" + PRIVATE ${PROJECT_NAME} GTest::gtest GTest::gtest_main) +gtest_discover_tests("${PROJECT_NAME}_test_owning_value_move" DISCOVERY_MODE PRE_TEST) + # benchmark if(CPPYYJSON_BUILD_BENCH) diff --git a/test/test.cpp b/test/test.cpp index 5809ae6..fef810d 100644 --- a/test/test.cpp +++ b/test/test.cpp @@ -652,16 +652,16 @@ TEST(Writer, ValueConversion) v = value(); EXPECT_TRUE(v.is_null()); EXPECT_EQ(nullptr, *v.as_null()); - EXPECT_EQ(1, v.count_children()); + EXPECT_EQ(0, v.count_children()); v = {1, value(2), value(3)}; EXPECT_TRUE(v.is_array()); - EXPECT_EQ(4, v.count_children()); + EXPECT_EQ(3, v.count_children()); v = {{"a", 1}, {"b", value(2)}, {"c", value(3)}}; EXPECT_TRUE(v.is_object()); - EXPECT_EQ(7, v.count_children()); + EXPECT_EQ(6, v.count_children()); v = {{{"a", 1}, {"b", value(2)}, {"c", value(3)}}, yyjson::copy_string}; EXPECT_TRUE(v.is_object()); - EXPECT_EQ(8, v.count_children()); + EXPECT_EQ(3, v.count_children()); // method return types static_assert(std::same_as().as_int()), std::optional>); diff --git a/test/test_owning_value_move.cpp b/test/test_owning_value_move.cpp new file mode 100644 index 0000000..008f3f3 --- /dev/null +++ b/test/test_owning_value_move.cpp @@ -0,0 +1,76 @@ +// Regression: owning yyjson::value (mutable_value_base) must +// be movable/assignable so it can live in standard containers and be reordered +// by std::sort / std::swap. +// +// Before the fix, operator=(value&&) unconditionally used set_value (node +// content overwrite + cross-doc keepalive), which: +// - accumulated the source's whole document into dst.doc_.children on every +// move-assign (memory bloat under std::sort's repeated shuffling); +// - dereferenced this->has_parent_ (a shared_ptr) that move-construction +// leaves empty on the moved-from object -> null-pointer dereference when +// std::sort's insertion sort move-assigns into a moved-from slot. +// +// The fix routes owning move-assign through document ownership transfer +// (doc_/val_/has_parent_) and makes get_has_parent() defensively rehydrate an +// empty shared_ptr. These tests pin both the contract and the crash scenario. +#include +#include +#include +#include +#include +#include +#include + +// 编译期契约:owning value 必须可移动构造/赋值(nothrow)。用 is_*_v trait 而非 +// concept(MSVC 对复杂模板的 concept/SFINAE 检查不可靠,见 test.cpp 在 MSVC 跳过)。 +static_assert(std::is_move_constructible_v); +static_assert(std::is_move_assignable_v); +static_assert(std::is_nothrow_move_constructible_v); +static_assert(std::is_nothrow_move_assignable_v); + +namespace +{ + yyjson::value make_int_value(int i) + { + yyjson::value v; + v = i; + return v; + } +} + +// 缺陷①:moved-from 对象被重新 move-assign 不应 null-deref。 +TEST(OwningValueMove, MovedFromCanBeReassigned) +{ + auto a = make_int_value(1); + auto b = make_int_value(2); + auto tmp = std::move(a); // a 变 moved-from(has_parent_ 被移空) + a = std::move(b); // 修复前:operator=(value&&) 解引用空 has_parent_ -> 💥 + EXPECT_EQ(a.as_sint().value_or(-1), 2); + (void)tmp; +} + +// 缺陷②:vector + std::sort 不崩且结果正确。 +TEST(OwningValueMove, VectorSortDoesNotCrash) +{ + std::vector v; + for (int i = 3; i >= 0; --i) // 逆序,确保 sort 触发 insertion 搬运 + { + v.push_back(make_int_value(i)); + } + std::sort(v.begin(), v.end(), [](const yyjson::value& x, const yyjson::value& y) { + return x.as_sint().value_or(0) < y.as_sint().value_or(0); + }); + ASSERT_EQ(v.size(), 4u); + for (int i = 0; i < 4; ++i) + { + EXPECT_EQ(v[i].as_sint().value_or(-1), i) << "index " << i; + } +} + +// 基本 move 语义:内容随所有权转移。 +TEST(OwningValueMove, MoveTransfersContent) +{ + auto a = make_int_value(42); + yyjson::value b = std::move(a); + EXPECT_EQ(b.as_sint().value_or(-1), 42); +}