From 6202519ca60c71ed85fb60f9b7acae4cdefb6cb2 Mon Sep 17 00:00:00 2001
From: Arseny Kapoulkine <arseny.kapoulkine@gmail.com>
Date: Tue, 17 Sep 2019 20:34:40 -0700
Subject: [PATCH] tests: Add memory safety tests for remove_children/attributes

The newly added tests make sure that during node/attribute destruction
we deallocate a few memory pages; this makes sure that we don't read
node data after it's being destroyed.

Also clean up formatting/style in the remove_* implementation a bit.
---
 src/pugixml.cpp           |  4 ++--
 tests/test_dom_modify.cpp | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/src/pugixml.cpp b/src/pugixml.cpp
index 39e07f1..af33ef8 100644
--- a/src/pugixml.cpp
+++ b/src/pugixml.cpp
@@ -6066,7 +6066,7 @@ namespace pugi
 		impl::xml_allocator& alloc = impl::get_allocator(_root);
 		if (!alloc.reserve()) return false;
 
-		for (xml_attribute_struct* attr = _root->first_attribute; attr;)
+		for (xml_attribute_struct* attr = _root->first_attribute; attr; )
 		{
 			xml_attribute_struct* next = attr->next_attribute;
 
@@ -6109,7 +6109,7 @@ namespace pugi
 		{
 			xml_node_struct* next = child->next_sibling;
 
-			destroy_node(child, alloc);
+			impl::destroy_node(child, alloc);
 
 			child = next;
 		}
diff --git a/tests/test_dom_modify.cpp b/tests/test_dom_modify.cpp
index d2f1a3c..319555a 100644
--- a/tests/test_dom_modify.cpp
+++ b/tests/test_dom_modify.cpp
@@ -505,6 +505,23 @@ TEST_XML(dom_node_remove_attributes, "<node a1='v1' a2='v2' a3='v3'><child a4='v
 	CHECK_NODE(node, STR("<node><child/></node>"));
 }
 
+TEST_XML(dom_node_remove_attributes_lots, "<node/>")
+{
+	xml_node node = doc.child(STR("node"));
+
+	// this test makes sure we generate at least 2 pages (64K) worth of attribute data
+	// so that we can trigger page deallocation to make sure code is memory safe
+	for (size_t i = 0; i < 10000; ++i)
+		node.append_attribute(STR("a")) = STR("v");
+
+	CHECK_STRING(node.attribute(STR("a")).value(), STR("v"));
+
+	CHECK(node.remove_attributes());
+
+	CHECK_STRING(node.attribute(STR("a")).value(), STR(""));
+	CHECK_NODE(node, STR("<node/>"));
+}
+
 TEST_XML(dom_node_prepend_child, "<node>foo<child/></node>")
 {
 	CHECK(xml_node().prepend_child() == xml_node());
@@ -733,6 +750,23 @@ TEST_XML(dom_node_remove_children, "<node><n1/><n2/><n3/><child><n4/></child></n
 	CHECK_NODE(node, STR("<node/>"));
 }
 
+TEST_XML(dom_node_remove_children_lots, "<node/>")
+{
+	xml_node node = doc.child(STR("node"));
+
+	// this test makes sure we generate at least 2 pages (64K) worth of node data
+	// so that we can trigger page deallocation to make sure code is memory safe
+	for (size_t i = 0; i < 10000; ++i)
+		node.append_child().set_name(STR("n"));
+
+	CHECK(node.child(STR("n")));
+
+	CHECK(node.remove_children());
+
+	CHECK(!node.child(STR("n")));
+	CHECK_NODE(node, STR("<node/>"));
+}
+
 TEST_XML(dom_node_remove_child_complex, "<node id='1'><n1 id1='1' id2='2'/><n2/><n3/><child><n4/></child></node>")
 {
 	CHECK(doc.child(STR("node")).remove_child(STR("n1")));