From 73d73d6e433f17f39e188f5c03ec176b60719416 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sun, 16 Oct 2022 10:05:23 -0400 Subject: [PATCH] fix: Document#remove_namespaces! use-after-free bug --- ext/nokogiri/xml_document.c | 6 +++++- test/test_compaction.rb | 22 ++++++++++++++++++++ test/test_memory_leak.rb | 41 +++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 1 deletion(-) diff --git a/ext/nokogiri/xml_document.c b/ext/nokogiri/xml_document.c index 216381bfd4..f79ee137bf 100644 --- 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 index e06d36d40d..9e32cf9a2f 100644 --- 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) + + hello from a + hello from b + + hello from c + + + 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 index 8b546c561c..aad3480bb7 100644 --- 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 + + hello from a + hello from b + + hello from c + + + 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 + + hello from a + hello from b + + hello from c + + + 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|