Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

check type passed to Document#root= #2210

Merged
merged 2 commits into from Mar 23, 2021
Merged

check type passed to Document#root= #2210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2920cf4
prefactor: cleans up Document#root= tests and C code
flavorjones Mar 22, 2021
4031110
fix: Document#root= raises an ArgumentError for a non-Node arg
flavorjones Mar 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -4,6 +4,14 @@ Nokogiri follows [Semantic Versioning](https://semver.org/), please see the [REA

---

## next / unreleased

### Fixed

* [CRuby] Passing non-`Node` objects to `Document#root=` now raises an `ArgumentError` exception. Previously this likely segfaulted. [[#1900](https://github.com/sparklemotion/nokogiri/issues/1900)]
* [JRuby] Passing non-`Node` objects to `Document#root=` now raises an `ArgumentError` exception. Previously this raised a `TypeError` exception.


## 1.11.2 / 2021-03-11

### Fixed
Expand Down
3 changes: 3 additions & 0 deletions ext/java/nokogiri/XmlDocument.java
View file
Open in desktop
Expand Up @@ -442,6 +442,9 @@ private static class DocumentBuilderFactoryHolder
getDocument().getDocumentElement().setUserData(NokogiriHelpers.ROOT_NODE_INVALID, Boolean.TRUE, null);
return new_root;
}
if (!(new_root instanceof XmlNode)) {
throw context.runtime.newArgumentError("expected Nokogiri::XML::Node but received " + new_root.getType());
}
XmlNode newRoot = asXmlNode(context, new_root);

IRubyObject root = root(context);
Expand Down
71 changes: 36 additions & 35 deletions ext/nokogiri/xml_document.c
View file
Open in desktop
Expand Up @@ -141,42 +141,41 @@ url(VALUE self)
* Set the root element on this document
*/
static VALUE
set_root(VALUE self, VALUE root)
rb_xml_document_root_set(VALUE self, VALUE rb_new_root)
{
xmlDocPtr doc;
xmlNodePtr new_root;
xmlNodePtr old_root;

Data_Get_Struct(self, xmlDoc, doc);
xmlDocPtr c_document;
xmlNodePtr c_new_root = NULL, c_current_root;

old_root = NULL;

if (NIL_P(root)) {
old_root = xmlDocGetRootElement(doc);

if (old_root) {
xmlUnlinkNode(old_root);
noko_xml_document_pin_node(old_root);
}
Data_Get_Struct(self, xmlDoc, c_document);

return root;
c_current_root = xmlDocGetRootElement(c_document);
if (c_current_root) {
xmlUnlinkNode(c_current_root);
noko_xml_document_pin_node(c_current_root);
}

Data_Get_Struct(root, xmlNode, new_root);
if (!NIL_P(rb_new_root)) {
if (!rb_obj_is_kind_of(rb_new_root, cNokogiriXmlNode)) {
rb_raise(rb_eArgError,
"expected Nokogiri::XML::Node but received %"PRIsVALUE,
rb_obj_class(rb_new_root));
}

Data_Get_Struct(rb_new_root, xmlNode, c_new_root);

/* If the new root's document is not the same as the current document,
* then we need to dup the node in to this document. */
if (new_root->doc != doc) {
old_root = xmlDocGetRootElement(doc);
if (!(new_root = xmlDocCopyNode(new_root, doc, 1))) {
rb_raise(rb_eRuntimeError, "Could not reparent node (xmlDocCopyNode)");
/* If the new root's document is not the same as the current document,
* then we need to dup the node in to this document. */
if (c_new_root->doc != c_document) {
c_new_root = xmlDocCopyNode(c_new_root, c_document, 1);
if (!c_new_root) {
rb_raise(rb_eRuntimeError, "Could not reparent node (xmlDocCopyNode)");
}
}
}

xmlDocSetRootElement(doc, new_root);
if (old_root) { noko_xml_document_pin_node(old_root); }
return root;
xmlDocSetRootElement(c_document, c_new_root);

return rb_new_root;
}

/*
Expand All @@ -186,17 +185,19 @@ set_root(VALUE self, VALUE root)
* Get the root node for this document.
*/
static VALUE
root(VALUE self)
rb_xml_document_root(VALUE self)
{
xmlDocPtr doc;
xmlNodePtr root;
xmlDocPtr c_document;
xmlNodePtr c_root;

Data_Get_Struct(self, xmlDoc, doc);
Data_Get_Struct(self, xmlDoc, c_document);

root = xmlDocGetRootElement(doc);
c_root = xmlDocGetRootElement(c_document);
if (!c_root) {
return Qnil;
}

if (!root) { return Qnil; }
return noko_xml_node_wrap(Qnil, root) ;
return noko_xml_node_wrap(Qnil, c_root) ;
}

/*
Expand Down Expand Up @@ -666,8 +667,8 @@ noko_init_xml_document()
rb_define_singleton_method(cNokogiriXmlDocument, "read_io", read_io, 4);
rb_define_singleton_method(cNokogiriXmlDocument, "new", new, -1);

rb_define_method(cNokogiriXmlDocument, "root", root, 0);
rb_define_method(cNokogiriXmlDocument, "root=", set_root, 1);
rb_define_method(cNokogiriXmlDocument, "root", rb_xml_document_root, 0);
rb_define_method(cNokogiriXmlDocument, "root=", rb_xml_document_root_set, 1);
rb_define_method(cNokogiriXmlDocument, "encoding", encoding, 0);
rb_define_method(cNokogiriXmlDocument, "encoding=", set_encoding, 1);
rb_define_method(cNokogiriXmlDocument, "version", version, 0);
Expand Down
95 changes: 57 additions & 38 deletions test/xml/test_document.rb
View file
Open in desktop
Expand Up @@ -77,11 +77,6 @@ def test_document_with_initial_space
assert_equal(2, doc.children.size)
end

def test_root_set_to_nil
xml.root = nil
assert_nil(xml.root)
end

def test_million_laugh_attach
doc = Nokogiri::XML(<<~EOF)
<?xml version="1.0"?>
Expand Down Expand Up @@ -146,14 +141,15 @@ def test_collect_namespaces
end

def test_subclass_initialize_modify # testing a segv
Class.new(Nokogiri::XML::Document) do
doc = Class.new(Nokogiri::XML::Document) do
def initialize
super
body_node = Nokogiri::XML::Node.new("body", self)
body_node.content = "stuff"
self.root = body_node
end
end.new
assert_equal("stuff", doc.root.content)
end

def test_create_text_node
Expand Down Expand Up @@ -419,18 +415,6 @@ def test_prepend_child_with_string
assert_equal("quack!", doc.root.children.first.content)
end

def test_move_root_to_document_with_no_root
sender = Nokogiri::XML("<root>foo</root>")
newdoc = Nokogiri::XML::Document.new
newdoc.root = sender.root
end

def test_move_root_with_existing_root_gets_gcd
doc = Nokogiri::XML("<root>test</root>")
doc2 = Nokogiri::XML("<root>#{'x' * 5000000}</root>")
doc2.root = doc.root
end

def test_validate
if Nokogiri.uses_libxml?
assert_equal(45, xml.validate.length)
Expand Down Expand Up @@ -852,20 +836,6 @@ def test_new
assert_nil(doc.root)
end

def test_set_root
doc = nil
doc = Nokogiri::XML::Document.new
assert(doc)
assert(doc.xml?)
assert_nil(doc.root)
node = Nokogiri::XML::Node.new("b", doc) do |n|
n.content = "hello world"
end
assert_equal("hello world", node.content)
doc.root = node
assert_equal(node, doc.root)
end

def test_remove_namespaces
doc = Nokogiri::XML(<<~EOX)
<root xmlns:a="http://a.flavorjon.es/" xmlns:b="http://b.flavorjon.es/">
Expand Down Expand Up @@ -958,6 +928,12 @@ def awesome!
assert(xml.children.respond_to?(:awesome!))
end

def test_can_be_closed
f = File.open(XML_FILE)
Nokogiri::XML(f)
f.close
end

describe "JRuby-only methods" do
describe "Document.wrap" do
if Nokogiri.jruby?
Expand Down Expand Up @@ -1034,12 +1010,6 @@ def awesome!
end
end

def test_can_be_closed
f = File.open(XML_FILE)
Nokogiri::XML(f)
f.close
end

describe "XML::Document.parse" do
# establish baseline behavior for HTML document behavior in
# https://github.com/sparklemotion/nokogiri/issues/2130
Expand Down Expand Up @@ -1148,6 +1118,55 @@ def initialize(*args)
end
end
end

describe "#root=" do
it "assigns a root node" do
doc = Nokogiri::XML::Document.new
node = doc.create_element("div")
assert_nil(doc.root)

doc.root = node

assert_equal(node, doc.root)
end

it "allows removing the root node by setting to nil" do
assert_kind_of(Nokogiri::XML::Node, xml.root)
assert_nil(xml.root = nil)
assert_nil(xml.root)
end

it "supports moving nodes across documents" do
source = Nokogiri::XML("<root>foo</root>")
target = Nokogiri::XML::Document.new
assert_nil(target.root)
target.root = source.root
assert_equal("foo", target.root.content)
end

it "doesn't leak the replaced node" do
skip("only run if NOKOGIRI_GC is set") unless ENV['NOKOGIRI_GC']
doc = Nokogiri::XML("<root>test</root>")
doc2 = Nokogiri::XML("<root>#{'x' * 5000000}</root>")
doc2.root = doc.root
end

it "raises an exception if passed something besides a Node" do
doc = Nokogiri::XML::Document.parse(<<~EOF)
<root>
<div>one</div>
<div>two</div>
<div>three</div>
</root>
EOF
node_set = doc.css("div")
assert_kind_of(Nokogiri::XML::NodeSet, node_set)
e = assert_raises(ArgumentError) do
doc.root = node_set
end
assert_equal("expected Nokogiri::XML::Node but received Nokogiri::XML::NodeSet", e.message);
end
end
end
end
end
Expand Down