From 1fdd47240fb6e0999e8aacb566fdddb2ee4fa6bd Mon Sep 17 00:00:00 2001 From: GoBobr <2378876+GoBobr@users.noreply.github.com> Date: Sat, 26 Sep 2026 10:40:33 +0200 Subject: [PATCH 1/2] Fix document corruption when reassigning Node handles (#1275) Node::operator=(const Node&) conflates two different operations: 1. Document mutation: assigning through a handle obtained from operator[] or iteration is expected to modify the document and create an alias (node["key"] = other). 2. Handle reassignment: assigning to a handle that was copied from another plain handle (e.g. a named variable or the root returned by Load) is expected to rebind the handle only. Previously both paths called AssignNode, which mutates the underlying detail::node via set_ref(). When the target node was shared with other Node handles (most importantly the document root), this silently rewrote the document and aliased unrelated handles together, making subsequent assignments no-ops and producing InvalidNode/BadConversion errors (#1275, #391, #516, #208). Track the origin of each Node handle with a private m_isProxy flag: handles created by document access (operator[], iterator dereference) are "views" and keep the legacy mutating semantics; handles created by copying a plain handle are "references" and operator= rebinds them without touching the document. No public API changes. The full test suite passes (1070 tests) including the new regression tests, which fail without this change. --- include/yaml-cpp/node/detail/iterator.h | 16 ++++++++--- include/yaml-cpp/node/impl.h | 20 ++++++++++++-- include/yaml-cpp/node/node.h | 6 +++++ test/node/node_test.cpp | 35 +++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 6 deletions(-) diff --git a/include/yaml-cpp/node/detail/iterator.h b/include/yaml-cpp/node/detail/iterator.h index 24866c2d8..b20048f3a 100644 --- a/include/yaml-cpp/node/detail/iterator.h +++ b/include/yaml-cpp/node/detail/iterator.h @@ -102,10 +102,18 @@ class iterator_base { value_type operator*() const YAML_ATTRIBUTE_LIFETIME_BOUND { const typename base_type::value_type& v = *m_iterator; - if (v.pNode) - return value_type(Node(*v, m_pMemory)); - if (v.first && v.second) - return value_type(Node(*v.first, m_pMemory), Node(*v.second, m_pMemory)); + if (v.pNode) { + value_type result(Node(*v, m_pMemory)); + result.m_isProxy = true; + return result; + } + if (v.first && v.second) { + value_type result(Node(*v.first, m_pMemory), Node(*v.second, m_pMemory)); + result.m_isProxy = true; + result.first.m_isProxy = true; + result.second.m_isProxy = true; + return result; + } return value_type(); } diff --git a/include/yaml-cpp/node/impl.h b/include/yaml-cpp/node/impl.h index 3f0d32387..22206354a 100644 --- a/include/yaml-cpp/node/impl.h +++ b/include/yaml-cpp/node/impl.h @@ -281,6 +281,18 @@ inline void Node::AssignNode(const Node& rhs) { throw InvalidNode(m_invalidKey); rhs.EnsureNodeExists(); + if (!m_isProxy) { + // PROTOTYPE B: this handle is a reference (created by copying a plain + // handle, e.g. a named variable). Reassigning it rebinds the handle only; + // the document is left untouched, so other handles sharing the same + // underlying node (e.g. the document root) are not affected. + m_pMemory = rhs.m_pMemory; + m_pNode = rhs.m_pNode; + return; + } + + // Legacy semantics: this handle is a view into the document, so assignment + // mutates the document (creates an alias to rhs). if (!m_pNode) { m_pNode = rhs.m_pNode; m_pMemory = rhs.m_pMemory; @@ -377,7 +389,9 @@ template inline Node Node::operator[](const Key& key) { EnsureNodeExists(); detail::node& value = m_pNode->get(key, m_pMemory); - return Node(value, m_pMemory); + Node result(value, m_pMemory); + result.m_isProxy = true; + return result; } template @@ -403,7 +417,9 @@ inline Node Node::operator[](const Node& key) { key.EnsureNodeExists(); m_pMemory->merge(*key.m_pMemory); detail::node& value = m_pNode->get(*key.m_pNode, m_pMemory); - return Node(value, m_pMemory); + Node result(value, m_pMemory); + result.m_isProxy = true; + return result; } inline bool Node::remove(const Node& key) { diff --git a/include/yaml-cpp/node/node.h b/include/yaml-cpp/node/node.h index 9d4294a31..1ebdb0800 100644 --- a/include/yaml-cpp/node/node.h +++ b/include/yaml-cpp/node/node.h @@ -155,6 +155,12 @@ class YAML_CPP_API Node { std::string m_invalidKey; mutable detail::shared_memory_holder m_pMemory; mutable detail::node* m_pNode; + // True if this handle was created by document access (operator[] / iterator + // dereference), i.e. it acts as a *view* into the document; assignment + // through it mutates the document (creates aliases). Handles obtained by + // copying a plain handle are *references*; assigning to them only rebinds + // the handle and never touches the document. + mutable bool m_isProxy = false; }; YAML_CPP_API bool operator==(const Node& lhs, const Node& rhs); diff --git a/test/node/node_test.cpp b/test/node/node_test.cpp index f5481685a..a9187b98f 100644 --- a/test/node/node_test.cpp +++ b/test/node/node_test.cpp @@ -5,6 +5,7 @@ #include "yaml-cpp/node/emit.h" #include "yaml-cpp/node/impl.h" #include "yaml-cpp/node/iterator.h" +#include "yaml-cpp/yaml.h" // IWYU pragma: keep (Load) #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -800,6 +801,40 @@ TEST(NodeTest, TempMapVariableAlias) { EXPECT_EQ(node["key"], node["other"]); } +TEST(NodeTest, ReassigningHandleDoesNotCorruptDocument) { + // https://github.com/jbeder/yaml-cpp/issues/1275 + Node cfg = Load("Test:\n Level1: \"Some string\"\n"); + Node currentNode = cfg; + currentNode = currentNode["Test"]; + EXPECT_EQ("Some string", currentNode["Level1"].as()); + + // Rebinding a plain handle to the root must not modify the document. + currentNode = cfg; + EXPECT_EQ(NodeType::Map, cfg.Type()); + EXPECT_EQ(1, cfg.size()); + EXPECT_TRUE(cfg["Test"].IsMap()); + + currentNode = currentNode["Test"]; + EXPECT_EQ("Some string", currentNode["Level1"].as()); + EXPECT_EQ("Some string", cfg["Test"]["Level1"].as()); + EXPECT_EQ("Test:\n Level1: Some string", Dump(cfg)); +} + +TEST(NodeTest, ReassigningHandleToSubnodeKeepsDocumentIntact) { + Node cfg = Load("a:\n b: 1\n"); + Node sub = cfg["a"]; + Node walker = cfg; + walker = sub; + EXPECT_EQ(1, walker["b"].as()); + + // The document must not be modified by the reassignments above. + EXPECT_EQ(NodeType::Map, cfg.Type()); + EXPECT_EQ(1, cfg.size()); + EXPECT_EQ(1, cfg["a"]["b"].as()); + EXPECT_TRUE(cfg["a"] == sub); +} + + TEST(NodeTest, Bool) { Node node; node[true] = false; From 900d4af2a167d07148c6af62e46b82da05a0c8f5 Mon Sep 17 00:00:00 2001 From: GoBobr <2378876+GoBobr@users.noreply.github.com> Date: Sun, 27 Sep 2026 10:39:10 +0200 Subject: [PATCH 2/2] Track view vs reference semantics consistently in Node handles Node::operator= has two distinct meanings that were previously conflated in AssignNode(): - assigning through a handle that refers to an element of a document (operator[] or iterator dereference) mutates the document, creating an alias - the documented alias-building behavior; - assigning through a plain handle (a named variable, the root of a loaded document) should rebind the handle only, leaving the document untouched. Previously both paths went through set_ref(), so reassigning a handle copied from the document root silently re-wrote the root, corrupting the document (issue #1275). Key changes: - View flag (m_isView) is set in the Node constructor via a tagged ViewNode ctor, not poked into the object after construction. Fixes two holes where the flag was previously lost: - Node n(*it) now matches Node n = *it (the explicit iterator_value ctor dropped the flag); - constNode["key"] now matches node["key"]. - reset() always yields a plain-reference handle - documented escape hatch from view semantics. - Flag stored as bit-field next to m_isValid: sizeof(Node) unchanged. - Flag set for both iterator branches from one place. - Tests: 2 regression tests for #1275 + 4 pinning the previously inconsistent paths; semantics documented in the Tutorial. Fixes #1275. --- docs/Tutorial.md | 31 ++++++++++++++++ include/yaml-cpp/node/detail/iterator.h | 11 ++---- include/yaml-cpp/node/impl.h | 43 +++++++++++----------- include/yaml-cpp/node/node.h | 22 +++++++---- test/node/node_test.cpp | 49 +++++++++++++++++++++++++ 5 files changed, 119 insertions(+), 37 deletions(-) diff --git a/docs/Tutorial.md b/docs/Tutorial.md index 5ebd42815..9ffd73888 100644 --- a/docs/Tutorial.md +++ b/docs/Tutorial.md @@ -119,6 +119,37 @@ self: *1 *3 : *2 ``` +# Views and References # + +Assignment through a `YAML::Node` handle means one of two things, depending on +where the handle came from: + +* A handle returned by `operator[]` or by dereferencing an iterator is a + **view** of an element of a collection. Assigning through a view *modifies + the containing document*: the element is re-pointed at the assigned node, + creating an alias if that node is part of some document. All the + alias-building examples above work this way. + +* Any other handle — for example a named variable initialized with + `YAML::Node n = doc;`, or the root node returned by `YAML::Load` — is a + plain **reference**. Assigning through a reference *rebinds* the handle; no + document is modified. This is handy for walking a document with a + cursor-like variable: + +```cpp +YAML::Node cfg = YAML::Load("Test:\n Level1: Some string\n"); +YAML::Node currentNode = cfg; +currentNode = currentNode["Test"]; // descend +currentNode = cfg; // go back up - works at any time +``` + +To turn a view back into a plain reference, use `reset()`: + +```cpp +YAML::Node view = doc["a"]; +view.reset(doc); // further assignments rebind instead of modifying the document +``` + # How Sequences Turn Into Maps # Sequences can be turned into maps by asking for non-integer keys. For example, diff --git a/include/yaml-cpp/node/detail/iterator.h b/include/yaml-cpp/node/detail/iterator.h index b20048f3a..bc2ac27ea 100644 --- a/include/yaml-cpp/node/detail/iterator.h +++ b/include/yaml-cpp/node/detail/iterator.h @@ -103,16 +103,11 @@ class iterator_base { value_type operator*() const YAML_ATTRIBUTE_LIFETIME_BOUND { const typename base_type::value_type& v = *m_iterator; if (v.pNode) { - value_type result(Node(*v, m_pMemory)); - result.m_isProxy = true; - return result; + return value_type(Node(Node::ViewNode, *v, m_pMemory)); } if (v.first && v.second) { - value_type result(Node(*v.first, m_pMemory), Node(*v.second, m_pMemory)); - result.m_isProxy = true; - result.first.m_isProxy = true; - result.second.m_isProxy = true; - return result; + return value_type(Node(Node::ViewNode, *v.first, m_pMemory), + Node(Node::ViewNode, *v.second, m_pMemory)); } return value_type(); } diff --git a/include/yaml-cpp/node/impl.h b/include/yaml-cpp/node/impl.h index 22206354a..c58dc970a 100644 --- a/include/yaml-cpp/node/impl.h +++ b/include/yaml-cpp/node/impl.h @@ -26,10 +26,12 @@ namespace YAML { inline Node::Node() - : m_isValid(true), m_invalidKey{}, m_pMemory(nullptr), m_pNode(nullptr) {} + : m_isValid(true), m_isView(false), m_invalidKey{}, m_pMemory(nullptr), + m_pNode(nullptr) {} inline Node::Node(NodeType::value type) : m_isValid(true), + m_isView(false), m_invalidKey{}, m_pMemory(std::make_shared()), m_pNode(&m_pMemory->create_node()) { @@ -39,6 +41,7 @@ inline Node::Node(NodeType::value type) template inline Node::Node(const T& rhs) : m_isValid(true), + m_isView(false), m_invalidKey{}, m_pMemory(std::make_shared()), m_pNode(&m_pMemory->create_node()) { @@ -46,21 +49,22 @@ inline Node::Node(const T& rhs) } inline Node::Node(const detail::iterator_value& rhs) - : m_isValid(rhs.m_isValid), - m_invalidKey(rhs.m_invalidKey), - m_pMemory(rhs.m_pMemory), - m_pNode(rhs.m_pNode) {} + : Node(static_cast(rhs)) {} inline Node::Node(const Node&) = default; inline Node::Node(Zombie) - : m_isValid(false), m_invalidKey{}, m_pMemory{}, m_pNode(nullptr) {} + : m_isValid(false), m_isView(false), m_invalidKey{}, m_pMemory{}, m_pNode(nullptr) {} inline Node::Node(Zombie, const std::string& key) - : m_isValid(false), m_invalidKey(key), m_pMemory{}, m_pNode(nullptr) {} + : m_isValid(false), m_isView(false), m_invalidKey(key), m_pMemory{}, m_pNode(nullptr) {} inline Node::Node(detail::node& node, detail::shared_memory_holder pMemory) - : m_isValid(true), m_invalidKey{}, m_pMemory(pMemory), m_pNode(&node) {} + : m_isValid(true), m_isView(false), m_invalidKey{}, m_pMemory(std::move(pMemory)), m_pNode(&node) {} + +// Element of a document (operator[] / iterator dereference): a *view*. +inline Node::Node(View, detail::node& node, detail::shared_memory_holder pMemory) + : m_isValid(true), m_isView(true), m_invalidKey{}, m_pMemory(std::move(pMemory)), m_pNode(&node) {} inline Node::~Node() = default; @@ -241,6 +245,7 @@ inline Node& Node::operator=(const Node& rhs) { inline void Node::reset(const YAML::Node& rhs) { if (!m_isValid || !rhs.m_isValid) throw InvalidNode(m_invalidKey); + m_isView = false; m_pMemory = rhs.m_pMemory; m_pNode = rhs.m_pNode; } @@ -281,18 +286,16 @@ inline void Node::AssignNode(const Node& rhs) { throw InvalidNode(m_invalidKey); rhs.EnsureNodeExists(); - if (!m_isProxy) { - // PROTOTYPE B: this handle is a reference (created by copying a plain - // handle, e.g. a named variable). Reassigning it rebinds the handle only; - // the document is left untouched, so other handles sharing the same - // underlying node (e.g. the document root) are not affected. + if (!m_isView) { + // Plain reference: rebinding this handle must not modify the document + // (e.g. the root) that it pointed into. m_pMemory = rhs.m_pMemory; m_pNode = rhs.m_pNode; return; } - // Legacy semantics: this handle is a view into the document, so assignment - // mutates the document (creates an alias to rhs). + // View: assignment mutates the document (re-points the element at rhs, + // creating an alias), as in `node["key"] = other;` if (!m_pNode) { m_pNode = rhs.m_pNode; m_pMemory = rhs.m_pMemory; @@ -382,16 +385,14 @@ inline const Node Node::operator[](const Key& key) const { if (!value) { return Node(ZombieNode, key_to_string(key)); } - return Node(*value, m_pMemory); + return Node(ViewNode, *value, m_pMemory); } template inline Node Node::operator[](const Key& key) { EnsureNodeExists(); detail::node& value = m_pNode->get(key, m_pMemory); - Node result(value, m_pMemory); - result.m_isProxy = true; - return result; + return Node(ViewNode, value, m_pMemory); } template @@ -417,9 +418,7 @@ inline Node Node::operator[](const Node& key) { key.EnsureNodeExists(); m_pMemory->merge(*key.m_pMemory); detail::node& value = m_pNode->get(*key.m_pNode, m_pMemory); - Node result(value, m_pMemory); - result.m_isProxy = true; - return result; + return Node(ViewNode, value, m_pMemory); } inline bool Node::remove(const Node& key) { diff --git a/include/yaml-cpp/node/node.h b/include/yaml-cpp/node/node.h index 1ebdb0800..36dad6973 100644 --- a/include/yaml-cpp/node/node.h +++ b/include/yaml-cpp/node/node.h @@ -89,10 +89,15 @@ class YAML_CPP_API Node { void SetStyle(EmitterStyle::value style); // assignment + // Assigning through a handle from operator[] or iterator dereference (a + // *view*) mutates the document, creating an alias. Assigning through any + // other handle (a *reference*, e.g. `Node n = doc;`) rebinds the handle + // only; see the Tutorial. bool is(const Node& rhs) const; template Node& operator=(const T& rhs); Node& operator=(const Node& rhs); + // Rebinds to `rhs` without modifying any document. void reset(const Node& rhs = Node()); // size/iterator @@ -136,7 +141,12 @@ class YAML_CPP_API Node { enum Zombie { ZombieNode }; explicit Node(Zombie); explicit Node(Zombie, const std::string&); + // Wraps the root of a freshly built document (NodeBuilder::Root()). explicit Node(detail::node& node, detail::shared_memory_holder pMemory); + // Same, but wraps an element of a document (operator[] / iterator), so the + // handle is a *view*: assigning through it mutates the document. + enum View { ViewNode }; + Node(View, detail::node& node, detail::shared_memory_holder pMemory); void EnsureNodeExists() const; void Invalidate(); @@ -150,17 +160,15 @@ class YAML_CPP_API Node { void AssignNode(const Node& rhs); private: - bool m_isValid; + // Bit-fields, so that m_isView does not change sizeof(Node). + bool m_isValid : 1; + // True if this handle refers to an element of a document (operator[] or + // iterator dereference): assigning through it mutates the document. + bool m_isView : 1; // String representation of invalid key, if the node is invalid. std::string m_invalidKey; mutable detail::shared_memory_holder m_pMemory; mutable detail::node* m_pNode; - // True if this handle was created by document access (operator[] / iterator - // dereference), i.e. it acts as a *view* into the document; assignment - // through it mutates the document (creates aliases). Handles obtained by - // copying a plain handle are *references*; assigning to them only rebinds - // the handle and never touches the document. - mutable bool m_isProxy = false; }; YAML_CPP_API bool operator==(const Node& lhs, const Node& rhs); diff --git a/test/node/node_test.cpp b/test/node/node_test.cpp index a9187b98f..ae9118f83 100644 --- a/test/node/node_test.cpp +++ b/test/node/node_test.cpp @@ -834,6 +834,55 @@ TEST(NodeTest, ReassigningHandleToSubnodeKeepsDocumentIntact) { EXPECT_TRUE(cfg["a"] == sub); } +// Assigning through a handle obtained from a const Node must have the same +// (document-mutating) semantics as through a non-const Node. +TEST(NodeTest, ConstAccessYieldsViewSemantics) { + Node node; + node["foo"] = "value"; + const Node& cnode = node; + Node view = cnode["foo"]; + view = Node(42); + EXPECT_EQ(42, node["foo"].as()); +} + +// Direct- and copy-initialization from an iterator_value must yield handles +// with identical (document-mutating) semantics. +TEST(NodeTest, IteratorValueCopyAndDirectInitAreEquivalent) { + { + Node node = Load("[1, 2, 3]"); + Node copyInit = *node.begin(); // copy-initialization + copyInit = Node(100); + EXPECT_EQ(100, node[0].as()); + } + { + Node node = Load("[1, 2, 3]"); + Node directInit(*node.begin()); // direct-initialization + directInit = Node(200); + EXPECT_EQ(200, node[0].as()); + } +} + +// reset() must produce a plain reference, so that assigning through the +// handle afterwards rebinds instead of mutating the document. +TEST(NodeTest, ResetTurnsAViewIntoAReference) { + Node node; + node["foo"] = "value"; + Node view = node["foo"]; + view.reset(); + view = Node(99); // rebinds, must not touch the document + EXPECT_EQ("value", node["foo"].as()); +} + +// The root of a freshly loaded document is a plain reference: assigning it +// rebinds and must not rewrite the document it pointed into. +TEST(NodeTest, LoadedRootAssignmentRebinds) { + Node cfg = Load("a:\n b: 1\n"); + Node cfg2 = Load("c:\n d: 2\n"); + cfg = cfg2; // rebinding; must not modify either document + EXPECT_EQ(1, cfg2.size()); + EXPECT_EQ(2, cfg["c"]["d"].as()); + EXPECT_EQ("c:\n d: 2", Dump(cfg2)); +} TEST(NodeTest, Bool) { Node node;