Bug #80521 for XML-LibXML: replaceNode() segfaults when copying DTD nodes with ATTLISTs

Wed Oct 31 09:15:50 2012 GUIDO [...] cpan.org - Ticket created

Subject:

replaceNode() segfaults when copying DTD nodes with ATTLISTs

use strict; use XML::LibXML; my $xml = <<EOF; <!DOCTYPE bug> <bug/> EOF $xml = <<EOF; <!DOCTYPE crash [ <!ATTLIST foo bar CDATA "baz"> ]> <crash/> EOF my $src = XML::LibXML->load_xml (string => $xml); my $dest = XML::LibXML->load_xml (string => $xml); my $src_dtd = $src->firstChild; my $dest_dtd = $dest->firstChild; $dest_dtd->replaceNode($src_dtd); __END__ Removing the ATTLIST prevents the crash. Regards, Guido

Wed Nov 07 17:24:40 2012 SHLOMIF [...] cpan.org - Correspondence added

Hi Guido, On Wed Oct 31 09:15:50 2012, GUIDO wrote: Show quoted text

> use strict; > > use XML::LibXML; > > my $xml = <<EOF; > <!DOCTYPE bug> > <bug/> > EOF > > $xml = <<EOF; > <!DOCTYPE crash [ > <!ATTLIST foo bar CDATA "baz"> > ]> > <crash/> > EOF > > my $src = XML::LibXML->load_xml (string => $xml); > my $dest = XML::LibXML->load_xml (string => $xml); > my $src_dtd = $src->firstChild; > my $dest_dtd = $dest->firstChild; > $dest_dtd->replaceNode($src_dtd); > __END__ > > Removing the ATTLIST prevents the crash. > > Regards, > Guido

This bug will require some investigation, but in the meanwhile I converted your test case to a t/*.t file, and I am going to make a new release to fix bug #80395. Regards, -- Shlomi Fish

Subject:

48_replaceNode_DTD_nodes_rT_80521.t

#!/usr/bin/perl use strict; use warnings; use Test::More tests => 1; use XML::LibXML; my $xml = <<'EOF'; <!DOCTYPE crash [ <!ATTLIST foo bar CDATA "baz"> ]> <crash/> EOF my $src = XML::LibXML->load_xml (string => $xml); my $dest = XML::LibXML->load_xml (string => $xml); my $src_dtd = $src->firstChild; my $dest_dtd = $dest->firstChild; $dest_dtd->replaceNode($src_dtd); # TEST ok(1, "Did not crash.");

Wed Nov 07 17:24:41 2012 The RT System itself - Status changed from 'new' to 'open'

Sat Aug 17 10:39:02 2013 yoreek [...] yahoo.com - Correspondence added

Hi Shlomi! On Thu Nov 08 00:24:40 2012, SHLOMIF wrote: Show quoted text

> Hi Guido, > > On Wed Oct 31 09:15:50 2012, GUIDO wrote:

> > use strict; > > > > use XML::LibXML; > > > > my $xml = <<EOF; > > <!DOCTYPE bug> > > <bug/> > > EOF > > > > $xml = <<EOF; > > <!DOCTYPE crash [ > > <!ATTLIST foo bar CDATA "baz"> > > ]> > > <crash/> > > EOF > > > > my $src = XML::LibXML->load_xml (string => $xml); > > my $dest = XML::LibXML->load_xml (string => $xml); > > my $src_dtd = $src->firstChild; > > my $dest_dtd = $dest->firstChild; > > $dest_dtd->replaceNode($src_dtd); > > __END__ > > > > Removing the ATTLIST prevents the crash. > > > > Regards, > > Guido

> > This bug will require some investigation, but in the meanwhile I > converted your test case to a t/*.t file, and I am going to make a new > release to fix bug #80395. > > Regards, > > -- Shlomi Fish

I propose the following patch to fix the problem. -- YOREEK

Subject:

80521.patch

diff -Nura XML-LibXML-2.0101-orig/perl-libxml-mm.c XML-LibXML-2.0101/perl-libxml-mm.c --- XML-LibXML-2.0101-orig/perl-libxml-mm.c 2012-12-04 16:17:54.000000000 +0200 +++ XML-LibXML-2.0101/perl-libxml-mm.c 2013-08-17 17:29:43.192297712 +0300 @@ -748,6 +748,7 @@ PmmREFCNT_dec(oldParent); if ( PmmNODE(nodetofix)->type != XML_ATTRIBUTE_NODE + && PmmNODE(nodetofix)->type != XML_DTD_NODE && PmmNODE(nodetofix)->properties != NULL ) { PmmFixOwnerList( (xmlNodePtr)PmmNODE(nodetofix)->properties, parent );

Mon Aug 19 07:21:49 2013 SHLOMIF [...] cpan.org - Correspondence added

Hi Yoreek, On Sat Aug 17 10:39:02 2013, yoreek wrote: Show quoted text

> Hi Shlomi! > > On Thu Nov 08 00:24:40 2012, SHLOMIF wrote:

> > Hi Guido, > > > > On Wed Oct 31 09:15:50 2012, GUIDO wrote:

> > > use strict; > > > > > > use XML::LibXML; > > > > > > my $xml = <<EOF; > > > <!DOCTYPE bug> > > > <bug/> > > > EOF > > > > > > $xml = <<EOF; > > > <!DOCTYPE crash [ > > > <!ATTLIST foo bar CDATA "baz"> > > > ]> > > > <crash/> > > > EOF > > > > > > my $src = XML::LibXML->load_xml (string => $xml); > > > my $dest = XML::LibXML->load_xml (string => $xml); > > > my $src_dtd = $src->firstChild; > > > my $dest_dtd = $dest->firstChild; > > > $dest_dtd->replaceNode($src_dtd); > > > __END__ > > > > > > Removing the ATTLIST prevents the crash. > > > > > > Regards, > > > Guido

> > > > This bug will require some investigation, but in the meanwhile I > > converted your test case to a t/*.t file, and I am going to make a new > > release to fix bug #80395. > > > > Regards, > > > > -- Shlomi Fish

> > I propose the following patch to fix the problem.

Applied with the test script I wrote as commit 1306:044a3d33f608346d504fa9e254ec84bf93ca6dae, and will be released in the next version. Thanks! Regards, -- Shlomi Fish

Mon Aug 19 07:24:22 2013 SHLOMIF [...] cpan.org - Correspondence added

Resolving so it won't appear in the list. Regards, -- Shlomi Fish On Mon Aug 19 07:21:49 2013, SHLOMIF wrote: Show quoted text

> Hi Yoreek, > > On Sat Aug 17 10:39:02 2013, yoreek wrote:

> > Hi Shlomi! > > > > On Thu Nov 08 00:24:40 2012, SHLOMIF wrote:

> > > Hi Guido, > > > > > > On Wed Oct 31 09:15:50 2012, GUIDO wrote:

> > > > use strict; > > > > > > > > use XML::LibXML; > > > > > > > > my $xml = <<EOF; > > > > <!DOCTYPE bug> > > > > <bug/> > > > > EOF > > > > > > > > $xml = <<EOF; > > > > <!DOCTYPE crash [ > > > > <!ATTLIST foo bar CDATA "baz"> > > > > ]> > > > > <crash/> > > > > EOF > > > > > > > > my $src = XML::LibXML->load_xml (string => $xml); > > > > my $dest = XML::LibXML->load_xml (string => $xml); > > > > my $src_dtd = $src->firstChild; > > > > my $dest_dtd = $dest->firstChild; > > > > $dest_dtd->replaceNode($src_dtd); > > > > __END__ > > > > > > > > Removing the ATTLIST prevents the crash. > > > > > > > > Regards, > > > > Guido

> > > > > > This bug will require some investigation, but in the meanwhile I > > > converted your test case to a t/*.t file, and I am going to make a > > > new > > > release to fix bug #80395. > > > > > > Regards, > > > > > > -- Shlomi Fish

> > > > I propose the following patch to fix the problem.

> > Applied with the test script I wrote as commit > 1306:044a3d33f608346d504fa9e254ec84bf93ca6dae, and will be released in > the next version. Thanks! > > Regards, > > -- Shlomi Fish

Mon Aug 19 07:24:23 2013 SHLOMIF [...] cpan.org - Status changed from 'open' to 'resolved'

Sun Mar 09 09:44:58 2014 NWELLNHOF [...] cpan.org - Correspondence added

This issue is not yet fixed properly. If you run t/48_replaceNode_DTD_nodes_rT_80521.t with valgrind, you'll see a couple of invalid reads and writes. This can also lead to segfaults as shown by these test reports: http://www.cpantesters.org/cpan/report/85e6d56a-878c-11e3-9db2-df2134817ebb http://www.cpantesters.org/cpan/report/3b71f122-7a58-11e3-834e-9cc90440ee7d The reason is that XML::LibXML doesn't unlink DTD nodes properly. DTD nodes require special handling so xmlUnlinkNode should be used. But the code in dom.c implements its own code to unlink nodes which doesn't handle DTD nodes correctly. There's another question of how to properly add a DTD to document. I'm not sure what's the best way so I started a thread on the libxml2 mailing list. Nick

Sun Mar 09 09:45:00 2014 NWELLNHOF [...] cpan.org - Status changed from 'resolved' to 'open'