Commits

Shlomi Fish  committed 02fb4ba Merge

Merged from nwellnhof/dtd-nodes.

  • Participants
  • Parent commits 8630a1b, 4b6fe0b

Comments (0)

Files changed (4)

 Revision history for Perl extension XML::LibXML
 
+2.0114
+    - Fix memory leaks and segfaults related to removal and insertion of
+      DTD nodes.
+        - https://rt.cpan.org/Ticket/Display.html?id=80521
+    - Fix memory leak in $node->removeChildNodes
+
 2.0113          Fri 14 Mar 14:13:05 IST 2014
     - Fix test failures with older libxml2 versions.
         - https://rt.cpan.org/Ticket/Display.html?id=93852
     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.
+     */
+    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);
+} );
+