Fix C14N document subset canonicalization with namespace nodes

Remove the xns->next = NULL hack from rxml_xpath_object_wrap. In XPath
results, libxml2 stores the parent element pointer in xns->next (see
xmlXPathNodeSetAddNs). Zeroing it broke xmlC14NIsNodeInNodeset which
relies on that pointer to match namespace nodes in the input node set.

Instead, guard Namespace#next to detect the XPath convention by checking
whether next points to a namespace (type == XML_LOCAL_NAMESPACE).

When canonicalize receives an XPath::Object for :nodes, pass the raw
xmlNodeSet directly to xmlC14NDocDumpMemory instead of roundtripping
through Ruby objects, which dropped namespace nodes.

Enable W3C C14N spec examples 3.5 (entity references) and 3.7 (document
subsets) which were previously commented out or marked TODO.

Author: cfis

ext/libxml/ruby_xml_document.c

@@ -275,42 +275,50 @@ rxml_document_canonicalize(int argc, VALUE *argv, VALUE self)
     //o_ns_prefixes will free at end of block
 
     o_nodes = rb_hash_aref(option_hash, ID2SYM(rb_intern("nodes")));
-    if (!NIL_P(o_nodes)) 
+    if (!NIL_P(o_nodes))
     {
-      int i;
-      int p = 0; // index of pointer array
-      VALUE * list_in = NULL;
-      long node_list_size = 0;
-
       if (CLASS_OF(o_nodes) == cXMLXPathObject)
       {
-        o_nodes = rb_funcall(o_nodes, rb_intern("to_a"), 0);
+        /* Use the raw xmlNodeSet directly to preserve namespace nodes
+           which cannot survive a roundtrip through Ruby objects */
+        rxml_xpath_object *rxpop;
+        TypedData_Get_Struct(o_nodes, rxml_xpath_object, &rxml_xpath_object_data_type, rxpop);
+        if (rxpop->xpop->nodesetval)
+        {
+          nodeset.nodeNr = rxpop->xpop->nodesetval->nodeNr;
+          nodeset.nodeMax = rxpop->xpop->nodesetval->nodeMax;
+          nodeset.nodeTab = rxpop->xpop->nodesetval->nodeTab;
+        }
       }
       else
       {
-        Check_Type(o_nodes, T_ARRAY);
-      }
-      list_in = RARRAY_PTR(o_nodes);
-      node_list_size = RARRAY_LEN(o_nodes);
+        int i;
+        int p = 0;
+        VALUE *list_in = NULL;
+        long node_list_size = 0;
 
-      for (i=0; i < node_list_size; ++i)
-      {
-        if (p >= C14N_NODESET_LIMIT) { break; }
+        Check_Type(o_nodes, T_ARRAY);
+        list_in = RARRAY_PTR(o_nodes);
+        node_list_size = RARRAY_LEN(o_nodes);
 
-        if (RTEST(list_in[i])) 
+        for (i=0; i < node_list_size; ++i)
         {
-          xmlNodePtr node_ptr;
-          TypedData_Get_Struct(list_in[i], xmlNode, &rxml_node_data_type, node_ptr);
-          node_ptr_array[p] = node_ptr;
-          p++;
+          if (p >= C14N_NODESET_LIMIT) { break; }
+
+          if (RTEST(list_in[i]))
+          {
+            xmlNodePtr node_ptr;
+            TypedData_Get_Struct(list_in[i], xmlNode, &rxml_node_data_type, node_ptr);
+            node_ptr_array[p] = node_ptr;
+            p++;
+          }
         }
-      }
 
-      // Need to set values in nodeset struct
-      nodeset.nodeNr = (node_list_size > C14N_NODESET_LIMIT ?
-                        C14N_NODESET_LIMIT :
-                        (int)node_list_size);
-      nodeset.nodeTab = node_ptr_array;
+        nodeset.nodeNr = (node_list_size > C14N_NODESET_LIMIT ?
+                          C14N_NODESET_LIMIT :
+                          (int)node_list_size);
+        nodeset.nodeTab = node_ptr_array;
+      }
     }
   }//option_hash
 

ext/libxml/ruby_xml_namespace.c

@@ -137,10 +137,14 @@ static VALUE rxml_namespace_next(VALUE self)
 {
   xmlNsPtr xns;
   TypedData_Get_Struct(self, xmlNs, &rxml_namespace_type, xns);
-  if (xns == NULL || xns->next == NULL)
+  /* Guard against libxml2's XPath hack where xns->next stores a parent
+     element pointer instead of the next namespace (see xmlXPathNodeSetAddNs
+     in xpath.c).  xmlNs.type and xmlNode.type share the same struct offset,
+     so checking the type is safe even when next points to an xmlNode. */
+  if (xns == NULL || xns->next == NULL || xns->next->type != XML_LOCAL_NAMESPACE)
     return (Qnil);
-  else
-    return rxml_namespace_wrap(xns->next);
+
+  return rxml_namespace_wrap(xns->next);
 }
 
 void rxml_init_namespace(void)

ext/libxml/ruby_xml_xpath_object.c

@@ -64,7 +64,7 @@ static void rxml_xpath_object_mark(void *data)
   rb_gc_mark(rxpop->nsnodes);
 }
 
-static const rb_data_type_t rxml_xpath_object_data_type = {
+const rb_data_type_t rxml_xpath_object_data_type = {
   .wrap_struct_name = "LibXML::XML::XPath::Object",
   .function = { .dmark = rxml_xpath_object_mark, .dfree = rxml_xpath_object_free },
   .flags = RUBY_TYPED_FREE_IMMEDIATELY,
@@ -82,7 +82,11 @@ VALUE rxml_xpath_object_wrap(VALUE document, xmlDocPtr xdoc, xmlXPathObjectPtr x
   rxpopp->xdoc = xdoc;
   rxpopp->xpop = xpop;
 
-  /* Find all the extra namespace nodes and wrap them. */
+  /* Find all the extra namespace nodes and wrap them.  In XPath results,
+     libxml2 stores the parent element pointer in xns->next (a hack -- see
+     xmlXPathNodeSetAddNs in xpath.c).  We leave that pointer intact so
+     that xmlC14NIsNodeInNodeset can match namespace nodes for C14N.
+     Namespace#next is guarded to detect this hack and return nil. */
   if (xpop->nodesetval && xpop->nodesetval->nodeNr)
   {
     for (i = 0; i < xpop->nodesetval->nodeNr; i++)
@@ -91,12 +95,6 @@ VALUE rxml_xpath_object_wrap(VALUE document, xmlDocPtr xdoc, xmlXPathObjectPtr x
       if (xnode != NULL && xnode->type == XML_NAMESPACE_DECL)
       {
         VALUE ns = Qnil;
-        xmlNsPtr xns = (xmlNsPtr)xnode;
-
-        /* Get rid of libxml's -> next hack.  The issue here is
-           the rxml_namespace code assumes that ns->next refers
-           to another namespace. */
-        xns->next = NULL;
 
         /* Specify a custom free function here since by default
            namespace nodes will not be freed */

ext/libxml/ruby_xml_xpath_object.h

@@ -2,6 +2,7 @@
 #define __RXML_XPATH_OBJECT__
 
 extern VALUE cXMLXPathObject;
+extern const rb_data_type_t rxml_xpath_object_data_type;
 
 typedef struct rxml_xpath_object
 {

test/test_canonicalize.rb

@@ -67,26 +67,23 @@ def test_canonicalize_with_w3c_c14n_3_4
 
   # (www.w3.org) 3.5 Entity References
   # http://www.w3.org/TR/xml-c14n#Example-Entities
-  # (2012-02-20) Failing likely due to a logic error
-  #   - libxml2(c14n.c:1788) XML_ENTITY_REF_NODE is invalid node for parsing.
   def test_canonicalize_with_w3c_c14n_3_5
-    #given_doc = LibXML::XML::Document.file(self.path('c14n/given/example-5.xml'))
+    # NOENT resolves entity references, DTDLOAD loads the external entity
+    options = LibXML::XML::Parser::Options::NOENT | LibXML::XML::Parser::Options::DTDLOAD
+    given_doc = LibXML::XML::Document.file(self.path('c14n/given/example-5.xml'), options: options)
 
     # With Comments
-    #expected_with_comments = IO.read(self.path('c14n/result/with-comments/example-5'))
-
-    # TODO - CANNOT COMPLETE TEST unless libxml2 supports additional node types.
-    #assert_equal(expected_with_comments, given_doc.canonicalize(:comments => true))
+    expected_with_comments = IO.read(self.path('c14n/result/with-comments/example-5'))
+    assert_equal(expected_with_comments, given_doc.canonicalize(:comments => true))
 
     # Without Comments
-    #expected_without_comments = IO.read(self.path('c14n/result/without-comments/example-5'))
-    # TODO - CANNOT COMPLETE TEST unless libxml2 supports additional node types.
-    #assert_equal(expected_without_comments, given_doc.canonicalize(:comments => false))
-    #expected_1_1_without_comments = IO.read(self.path('c14n/result/1-1-without-comments/example-5'))
-    #mode = LibXML::XML::Document::XML_C14N_1_1
-
-    # TODO - CANNOT COMPLETE TEST unless libxml2 supports additional node types.
-    #assert_equal(expected_1_1_without_comments, given_doc.canonicalize(:mode => mode))
+    expected_without_comments = IO.read(self.path('c14n/result/without-comments/example-5'))
+    assert_equal(expected_without_comments, given_doc.canonicalize(:comments => false))
+
+    # Without Comments (XML_C14N_1_1)
+    expected_1_1_without_comments = IO.read(self.path('c14n/result/1-1-without-comments/example-5'))
+    mode = LibXML::XML::Document::XML_C14N_1_1
+    assert_equal(expected_1_1_without_comments, given_doc.canonicalize(:mode => mode))
   end
 
   # (www.w3.org) 3.6 UTF-8 Encoding
@@ -104,16 +101,22 @@ def test_canonicalize_with_w3c_c14n_3_6
   # (www.w3.org) 3.7 Document Subsets
   # http://www.w3.org/TR/xml-c14n#Example-DocSubsets
   def test_canonicalize_with_w3c_c14n_3_7
-    # Non Canonicalized Document
-    given_doc = LibXML::XML::Document.file(self.path('c14n/given/example-7.xml'))
+    # Parse with DTDATTR and DTDLOAD so default attributes and ID types
+    # from the internal DTD subset are materialized
+    options = LibXML::XML::Parser::Options::DTDATTR | LibXML::XML::Parser::Options::DTDLOAD
+    given_doc = LibXML::XML::Document.file(self.path('c14n/given/example-7.xml'),
+                                           options: options)
     expected = IO.read(self.path('c14n/result/without-comments/example-7'))
 
-    e1_node = given_doc.find_first('ietf:e1', 'ietf:http://www.ietf.org')
+    ctx = given_doc.context
+    ctx.register_namespace('ietf', 'http://www.ietf.org')
 
-    # Select current node, all child nodes, all attributes and namespace nodes
-    subdoc_nodes = e1_node.find("(.//.|.//@id|namespace::*)")
+    # W3C C14N spec example 3.7 XPath expression for document subsets
+    subdoc_nodes = ctx.find(
+      "(//. | //@* | //namespace::*)" \
+      "[self::ietf:e1 or (parent::ietf:e1 and not(self::text() or self::e2))" \
+      " or count(id('E3')|ancestor-or-self::node()) = count(ancestor-or-self::node())]")
 
-    # TODO - This fails because the namespace nodes aren't taken into account
     assert_equal(expected, given_doc.canonicalize(:nodes => subdoc_nodes))
   end
 end