Commits

Nick Wellnhofer committed 001cce1

Fix handling of DTD nodes

When removing a DTD node, it must not be added to a document fragment.
Otherwise it will leak. Also make sure that removed attributes aren't
added to document fragments.

When inserting a DTD node, make sure that the 'intSubset' member of the
document struct is updated and the previous DTD is unlinked and freed.
This fixes memory leaks and potential segfaults. See RT #80521:

https://rt.cpan.org/Ticket/Display.html?id=80521

Also fix a memory leak in $node->removeChildNodes and make sure that
the correct owner is set when inserting nodes from other documents via
$node->insertBefore ro $node->insertAfter.

Comments (0)

Files changed (3)

     return(1);
 }
 
+/* Assumes that the node has a proxy. */
+static void
+LibXML_reparent_removed_node(xmlNodePtr node) {
+    /*
+     * Attribute nodes can't be added to document fragments. Adding
+     * DTD nodes would cause a memory leak. So document and owner are
+     * set to NULL.
+     */
+    if (node->type != XML_ATTRIBUTE_NODE
+        && node->type != XML_DTD_NODE) {
+        ProxyNodePtr docfrag = PmmNewFragment(node->doc);
+        xmlAddChild(PmmNODE(docfrag), node);
+        PmmFixOwner(PmmPROXYNODE(node), docfrag);
+    }
+}
+
+static void
+LibXML_set_int_subset(xmlDocPtr doc, xmlNodePtr dtd) {
+    xmlNodePtr old_dtd = (xmlNodePtr)doc->intSubset;
+    if (old_dtd == dtd) {
+        return;
+    }
+
+    if (old_dtd != NULL) {
+        xmlUnlinkNode(old_dtd);
+
+        if (PmmPROXYNODE(old_dtd) == NULL) {
+            xmlFreeDtd((xmlDtdPtr)old_dtd);
+        }
+    }
+
+    doc->intSubset = (xmlDtdPtr)dtd;
+}
+
 /* ****************************************************************
  * XPathContext helper functions
  * **************************************************************** */
             croak( "Can't import Documents!" );
             XSRETURN_UNDEF;
         }
+        if (node->type == XML_DTD_NODE) {
+            croak("Can't import DTD nodes");
+        }
 
         ret = domImportNode( self, node, 0, 1 );
         if ( ret ) {
             croak( "Can't adopt Documents!" );
             XSRETURN_UNDEF;
         }
+        if (node->type == XML_DTD_NODE) {
+            croak("Can't adopt DTD nodes");
+        }
 
         ret = domImportNode( self, node, 1, 1 );
 
         if ( rNode != NULL ) {
             RETVAL = PmmNodeToSv( rNode,
                                   PmmOWNERPO(PmmPROXYNODE(self)) );
-            PmmFixOwner(PmmOWNERPO(SvPROXYNODE(RETVAL)),
-                        PmmOWNERPO(PmmPROXYNODE(self)) );
+            if (rNode->type == XML_DTD_NODE) {
+                LibXML_set_int_subset(self->doc, rNode);
+            }
+            PmmFixOwner(PmmPROXYNODE(rNode), PmmOWNERPO(PmmPROXYNODE(self)));
         }
         else {
-                 XSRETURN_UNDEF;
+            XSRETURN_UNDEF;
         }
     OUTPUT:
         RETVAL
         if ( rNode != NULL ) {
             RETVAL = PmmNodeToSv( rNode,
                                   PmmOWNERPO(PmmPROXYNODE(self)) );
-                PmmFixOwner(PmmOWNERPO(SvPROXYNODE(RETVAL)),
-                            PmmOWNERPO(PmmPROXYNODE(self)) );
+            if (rNode->type == XML_DTD_NODE) {
+                LibXML_set_int_subset(self->doc, rNode);
+            }
+            PmmFixOwner(PmmPROXYNODE(rNode), PmmOWNERPO(PmmPROXYNODE(self)));
         }
         else {
             XSRETURN_UNDEF;
         xmlNodePtr oNode
     PREINIT:
         xmlNodePtr ret = NULL;
-        ProxyNodePtr docfrag = NULL;
     CODE:
        if ( self->type == XML_DOCUMENT_NODE ) {
                 switch ( nNode->type ) {
             XSRETURN_UNDEF;
         }
         else {
-                docfrag = PmmNewFragment( self->doc );
-                /* create document fragment */
-                xmlAddChild( PmmNODE(docfrag), ret );
-                RETVAL = PmmNodeToSv(ret, docfrag);
-
-                if ( nNode->_private != NULL ) {
-                    PmmFixOwner( PmmPROXYNODE(nNode),
-                                 PmmOWNERPO(PmmPROXYNODE(self)) );
-                }
-                PmmFixOwner( SvPROXYNODE(RETVAL), docfrag );
+            LibXML_reparent_removed_node(ret);
+            RETVAL = PmmNodeToSv(ret, PmmOWNERPO(PmmPROXYNODE(ret)));
+            if (nNode->type == XML_DTD_NODE) {
+                LibXML_set_int_subset(nNode->doc, nNode);
+            }
+            if ( nNode->_private != NULL ) {
+                PmmFixOwner( PmmPROXYNODE(nNode),
+                             PmmOWNERPO(PmmPROXYNODE(self)) );
+            }
         }
     OUTPUT:
         RETVAL
         xmlNodePtr nNode
     PREINIT:
         xmlNodePtr ret = NULL;
-        ProxyNodePtr docfrag = NULL;
+        ProxyNodePtr owner = NULL;
     CODE:
         if ( domIsParent( self, nNode ) == 1 ) {
             XSRETURN_UNDEF;
         }
-        if ( self->doc != nNode->doc ) {
-            domImportNode( self->doc, nNode, 1, 1 );
-        }
+        owner = PmmOWNERPO(PmmPROXYNODE(self));
 
         if ( self->type != XML_ATTRIBUTE_NODE ) {
               ret = domReplaceChild( self->parent, nNode, self);
              ret = xmlReplaceNode( self, nNode );
         }
         if ( ret ) {
-            if ( ret->type == XML_ATTRIBUTE_NODE ) {
-                docfrag = NULL;
-            }
-            else {
-                /* create document fragment */
-                docfrag = PmmNewFragment( self->doc );
-                xmlAddChild( PmmNODE(docfrag), ret );
-            }
-
-            RETVAL = PmmNodeToSv(ret, docfrag);
+            LibXML_reparent_removed_node(ret);
+            RETVAL = PmmNodeToSv(ret, PmmOWNERPO(PmmPROXYNODE(ret)));
+            if (nNode->type == XML_DTD_NODE) {
+                LibXML_set_int_subset(nNode->doc, nNode);
+            }
             if ( nNode->_private != NULL ) {
-                PmmFixOwner( PmmPROXYNODE(nNode),
-                             PmmOWNERPO(PmmPROXYNODE(self)));
-            }
-            PmmFixOwner( SvPROXYNODE(RETVAL), docfrag );
+                PmmFixOwner(PmmPROXYNODE(nNode), owner);
+            }
         }
         else {
             croak( "replacement failed" );
             XSRETURN_UNDEF;
         }
         else {
-                ProxyNodePtr docfrag = PmmNewFragment( ret->doc );
-                xmlAddChild( PmmNODE(docfrag), ret );
-                RETVAL = PmmNodeToSv(ret,NULL);
-                PmmFixOwner( SvPROXYNODE(RETVAL), docfrag );
+            LibXML_reparent_removed_node(ret);
+            RETVAL = PmmNodeToSv(ret, NULL);
         }
     OUTPUT:
         RETVAL
         fragment = PmmNODE( docfrag );
         elem = self->children;
         while ( elem ) {
+            xmlNodePtr next = elem->next;
             xmlUnlinkNode( elem );
-            /* this following piece is the function of domAppendChild()
-             * but in this special case we can avoid most of the logic of
-             * that function.
-             */
-            if ( fragment->children != NULL ) {
-                xs_warn("unlink node!\n");
-                domAddNodeToList( elem, fragment->last, NULL );
+            if (elem->type == XML_ATTRIBUTE_NODE
+                || elem->type == XML_DTD_NODE) {
+                if (PmmPROXYNODE(elem) == NULL) {
+                    xmlFreeNode(elem);
+                }
             }
             else {
-                fragment->children = elem;
-                fragment->last     = elem;
-                elem->parent= fragment;
-            }
-            PmmFixOwnerNode( elem, docfrag );
-            elem = elem->next;
+                /* this following piece is the function of domAppendChild()
+                 * but in this special case we can avoid most of the logic of
+                 * that function.
+                 */
+                if ( fragment->children != NULL ) {
+                    xs_warn("unlink node!\n");
+                    domAddNodeToList( elem, fragment->last, NULL );
+                }
+                else {
+                    fragment->children = elem;
+                    fragment->last     = elem;
+                    elem->parent= fragment;
+                }
+                PmmFixOwnerNode( elem, docfrag );
+            }
+            elem = next;
         }
 
         self->children = self->last = NULL;
         if ( self->type != XML_DOCUMENT_NODE
              && self->type != XML_DOCUMENT_FRAG_NODE ) {
             xmlUnlinkNode( self );
-            if ( self->type != XML_ATTRIBUTE_NODE ) {
-                docfrag = PmmNewFragment( self->doc );
-                xmlAddChild( PmmNODE(docfrag), self );
-            }
-            PmmFixOwner( PmmPROXYNODE(self), docfrag );
+            LibXML_reparent_removed_node(self);
         }
 
 SV*
 
         RETVAL = PmmNodeToSv( nNode,
                               PmmOWNERPO(PmmPROXYNODE(self)) );
+        if (nNode->type == XML_DTD_NODE) {
+            LibXML_set_int_subset(self->doc, nNode);
+        }
         PmmFixOwner( SvPROXYNODE(RETVAL), PmmPROXYNODE(self) );
     OUTPUT:
         RETVAL
         xmlNodePtr nNode
     PREINIT:
         xmlNodePtr ret = NULL;
+        ProxyNodePtr owner = NULL;
     CODE:
         if ( nNode->type == XML_DOCUMENT_FRAG_NODE ) {
             croak("Adding document fragments with addSibling not yet supported!");
             XSRETURN_UNDEF;
         }
+        owner = PmmOWNERPO(PmmPROXYNODE(self));
 
         ret = xmlAddSibling( self, nNode );
 
         if ( ret ) {
             RETVAL = PmmNodeToSv(ret,NULL);
-            PmmFixOwner( SvPROXYNODE(RETVAL), PmmOWNERPO(PmmPROXYNODE(self)) );
+            if (nNode->type == XML_DTD_NODE) {
+                LibXML_set_int_subset(self->doc, nNode);
+            }
+            PmmFixOwner(SvPROXYNODE(RETVAL), owner);
         }
         else {
             XSRETURN_UNDEF;
         return;
     }
 
+    if (node->type == XML_DTD_NODE) {
+        /* This clears the doc->intSubset pointer. */
+        xmlUnlinkNode(node);
+        return;
+    }
+
     if ( node->prev != NULL ) {
         node->prev->next = node->next;
     }
 
     if ( move ) {
         return_node = node;
-        if ( node->type != XML_DTD_NODE ) {
-            domUnlinkNode( node );
-        }
+        domUnlinkNode( node );
     }
     else {
         if ( node->type == XML_DTD_NODE ) {
 use strict;
 use warnings;
 
-# Should be 40.
-use Test::More tests => 40;
+# Should be 54.
+use Test::More tests => 54;
 
 use lib './t/lib';
 use TestHelpers;
     is_deeply( [ $dtd->attributes ], [], 'attributes' );
 }
 
+# Remove DTD nodes
+
+sub test_remove_dtd {
+    my ($test_name, $remove_sub) = @_;
+
+    my $parser = XML::LibXML->new;
+    my $doc    = $parser->parse_file('example/dtd.xml');
+    my $dtd    = $doc->internalSubset;
+
+    $remove_sub->($doc, $dtd);
+
+    # TEST*3
+    ok( !$doc->internalSubset, "remove DTD via $test_name" );
+}
+
+test_remove_dtd( "unbindNode", sub {
+    my ($doc, $dtd) = @_;
+    $dtd->unbindNode;
+} );
+test_remove_dtd( "removeChild", sub {
+    my ($doc, $dtd) = @_;
+    $doc->removeChild($dtd);
+} );
+test_remove_dtd( "removeChildNodes", sub {
+    my ($doc, $dtd) = @_;
+    $doc->removeChildNodes;
+} );
+
+# Insert DTD nodes
+
+sub test_insert_dtd {
+    my ($test_name, $insert_sub) = @_;
+
+    my $parser  = XML::LibXML->new;
+    my $src_doc = $parser->parse_file('example/dtd.xml');
+    my $dtd     = $src_doc->internalSubset;
+    my $doc     = $parser->parse_file('example/dtd.xml');
+
+    $insert_sub->($doc, $dtd);
+
+    # TEST*11
+    ok( $doc->internalSubset->isSameNode($dtd), "insert DTD via $test_name" );
+}
+
+test_insert_dtd( "insertBefore internalSubset", sub {
+    my ($doc, $dtd) = @_;
+    $doc->insertBefore($dtd, $doc->internalSubset);
+} );
+test_insert_dtd( "insertBefore documentElement", sub {
+    my ($doc, $dtd) = @_;
+    $doc->insertBefore($dtd, $doc->documentElement);
+} );
+test_insert_dtd( "insertAfter internalSubset", sub {
+    my ($doc, $dtd) = @_;
+    $doc->insertAfter($dtd, $doc->internalSubset);
+} );
+test_insert_dtd( "insertAfter documentElement", sub {
+    my ($doc, $dtd) = @_;
+    $doc->insertAfter($dtd, $doc->documentElement);
+} );
+test_insert_dtd( "replaceChild internalSubset", sub {
+    my ($doc, $dtd) = @_;
+    $doc->replaceChild($dtd, $doc->internalSubset);
+} );
+test_insert_dtd( "replaceChild documentElement", sub {
+    my ($doc, $dtd) = @_;
+    $doc->replaceChild($dtd, $doc->documentElement);
+} );
+test_insert_dtd( "replaceNode internalSubset", sub {
+    my ($doc, $dtd) = @_;
+    $doc->internalSubset->replaceNode($dtd);
+} );
+test_insert_dtd( "replaceNode documentElement", sub {
+    my ($doc, $dtd) = @_;
+    $doc->documentElement->replaceNode($dtd);
+} );
+test_insert_dtd( "appendChild", sub {
+    my ($doc, $dtd) = @_;
+    $doc->appendChild($dtd);
+} );
+test_insert_dtd( "addSibling internalSubset", sub {
+    my ($doc, $dtd) = @_;
+    $doc->internalSubset->addSibling($dtd);
+} );
+test_insert_dtd( "addSibling documentElement", sub {
+    my ($doc, $dtd) = @_;
+    $doc->documentElement->addSibling($dtd);
+} );
+