Fix document corruption when reassigning Node handles (#1275)

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,

include/yaml-cpp/node/detail/iterator.h

@@ -102,10 +102,13 @@ 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) {
+      return value_type(Node(Node::ViewNode, *v, m_pMemory));
+    }
+    if (v.first && v.second) {
+      return value_type(Node(Node::ViewNode, *v.first, m_pMemory),
+                        Node(Node::ViewNode, *v.second, m_pMemory));
+    }
     return value_type();
   }
 

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<detail::memory_holder>()),
       m_pNode(&m_pMemory->create_node()) {
@@ -39,28 +41,30 @@ inline Node::Node(NodeType::value type)
 template <typename T>
 inline Node::Node(const T& rhs)
     : m_isValid(true),
+      m_isView(false),
       m_invalidKey{},
       m_pMemory(std::make_shared<detail::memory_holder>()),
       m_pNode(&m_pMemory->create_node()) {
   Assign(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<const Node&>(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,6 +286,16 @@ inline void Node::AssignNode(const Node& rhs) {
     throw InvalidNode(m_invalidKey);
   rhs.EnsureNodeExists();
 
+  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;
+  }
+
+  // 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;
@@ -370,14 +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 <typename Key>
 inline Node Node::operator[](const Key& key) {
   EnsureNodeExists();
   detail::node& value = m_pNode->get(key, m_pMemory);
-  return Node(value, m_pMemory);
+  return Node(ViewNode, value, m_pMemory);
 }
 
 template <typename Key>
@@ -403,7 +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);
-  return Node(value, m_pMemory);
+  return Node(ViewNode, value, m_pMemory);
 }
 
 inline bool Node::remove(const Node& key) {

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 <typename T>
   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,7 +160,11 @@ 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;

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,89 @@ 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<std::string>());
+
+  // 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<std::string>());
+  EXPECT_EQ("Some string", cfg["Test"]["Level1"].as<std::string>());
+  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<int>());
+
+  // 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<int>());
+  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<int>());
+}
+
+// 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<int>());
+  }
+  {
+    Node node = Load("[1, 2, 3]");
+    Node directInit(*node.begin());  // direct-initialization
+    directInit = Node(200);
+    EXPECT_EQ(200, node[0].as<int>());
+  }
+}
+
+// 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<std::string>());
+}
+
+// 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<int>());
+  EXPECT_EQ("c:\n  d: 2", Dump(cfg2));
+}
+
 TEST(NodeTest, Bool) {
   Node node;
   node[true] = false;