fix: Document#remove_namespaces! use-after-free bug

sparklemotion · Oct 16, 2022 · 73d73d6 · 73d73d6
1 parent 5f58b34
commit 73d73d6
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 1 deletion.
diff --git a/ext/nokogiri/xml_document.c b/ext/nokogiri/xml_document.c
@@ -104,7 +104,11 @@ recursively_remove_namespaces_from_node(xmlNodePtr node)
        (node->type == XML_XINCLUDE_START) ||
        (node->type == XML_XINCLUDE_END)) &&
       node->nsDef) {
-    xmlFreeNsList(node->nsDef);
+    xmlNsPtr curr = node->nsDef;
+    while (curr) {
+      noko_xml_document_pin_namespace(curr, node->doc);
+      curr = curr->next;
+    }
     node->nsDef = NULL;
   }
 

diff --git a/test/test_compaction.rb b/test/test_compaction.rb
@@ -39,5 +39,27 @@
 
       doc.at_xpath("//root:first", "root" => "http://example.com/root").namespace_scopes.inspect
     end
+
+    it "remove_namespaces!" do
+      skip unless GC.respond_to?(:verify_compaction_references)
+
+      doc = Nokogiri::XML(<<~XML)
+        <root xmlns:a="http://a.flavorjon.es/" xmlns:b="http://b.flavorjon.es/">
+          <a:foo>hello from a</a:foo>
+          <b:foo>hello from b</b:foo>
+          <container xmlns:c="http://c.flavorjon.es/">
+            <c:foo c:attr='attr-value'>hello from c</c:foo>
+          </container>
+        </root>
+      XML
+
+      namespaces = doc.root.namespaces
+      namespaces.each(&:inspect)
+      doc.remove_namespaces!
+
+      GC.verify_compaction_references(double_heap: true, toward: :empty)
+
+      namespaces.each(&:inspect)
+    end
   end
 end
diff --git a/test/test_memory_leak.rb b/test/test_memory_leak.rb
@@ -191,6 +191,47 @@ def test_leaking_namespace_node_strings_with_prefix
       end
     end
 
+    def test_document_remove_namespaces_with_ruby_objects
+      xml = <<~XML
+        <root xmlns:a="http://a.flavorjon.es/" xmlns:b="http://b.flavorjon.es/">
+          <a:foo>hello from a</a:foo>
+          <b:foo>hello from b</b:foo>
+          <container xmlns:c="http://c.flavorjon.es/">
+            <c:foo c:attr='attr-value'>hello from c</c:foo>
+          </container>
+        </root>
+      XML
+
+      20.times do
+        10_000.times do
+          doc = Nokogiri::XML::Document.parse(xml)
+          doc.namespaces.each(&:inspect)
+          doc.remove_namespaces!
+        end
+        puts MemInfo.rss
+      end
+    end
+
+    def test_document_remove_namespaces_without_ruby_objects
+      xml = <<~XML
+        <root xmlns:a="http://a.flavorjon.es/" xmlns:b="http://b.flavorjon.es/">
+          <a:foo>hello from a</a:foo>
+          <b:foo>hello from b</b:foo>
+          <container xmlns:c="http://c.flavorjon.es/">
+            <c:foo c:attr='attr-value'>hello from c</c:foo>
+          </container>
+        </root>
+      XML
+
+      20.times do
+        20_000.times do
+          doc = Nokogiri::XML::Document.parse(xml)
+          doc.remove_namespaces!
+        end
+        puts MemInfo.rss
+      end
+    end
+
     def test_leaking_dtd_nodes_after_internal_subset_removal
       # see https://github.com/sparklemotion/nokogiri/issues/1784
       100_000.times do |i|