Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:15834
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Message-ID: <425319AB.90902@safesearching.com>
Date: Tue, 05 Apr 2005 16:05:15 -0700
User-Agent: Mozilla Thunderbird 1.0.2 (Windows/20050317)
MIME-Version: 1.0
To: internals@lists.php.net
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Possible error with DOMNode::replaceChild() in 5.0.4
From: james@safesearching.com (James Crumpton)

After upgrading to 5.0.4, a script of mine that relies on the DocumentFragment
object suddenly started to segfault. Some debugging of the script found the line
  that was causing the segfault to be a call to DOMNode::replaceChild(). The
script is complex enough that it's turning out to be rather difficult to reduce
the code to a small script that can reproduce the error. I was, however, able to
track the error down with gdb:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1024 (LWP 30537)]
0x403490ac in zif_dom_node_replace_child (ht=2, return_value=0x835c0cc,
     this_ptr=0x835c10c, return_value_used=0)
     at /home/james/php-5.0.4/ext/dom/node.c:1150
1150                                            prevsib->next = newchild;

The problem seems to be related to the fix for bug #32011 (Fixed bug #32011
(Fragments which replaced Nodes are not globaly useable)).

The relevant code from node.c being:

xmlNodePtr fragment, prevsib, nextsib;
fragment = newchild;
prevsib = oldchild->prev;
nextsib = oldchild->next;
newchild = fragment->children;

xmlUnlinkNode(oldchild);

if (prevsib == NULL && nextsib == NULL) {
     nodep->children = newchild;
     nodep->last = fragment->last;
} else {
     if (newchild) {
         prevsib->next = newchild;	<--- segfault is here
         newchild->prev = prevsib;

         fragment->last->next = nextsib;
         if (nextsib) {
             nextsib->prev = fragment->last;
         } else {
             nodep->last = fragment->last;
         }
     }
}

It turns out that I am somehow encountering a situation in which prevsib is 
NULL, which the code doesn't account for.

The following "fix" "works" for me:

1150,1155c1150,1151
<                                       if (prevsib) {
<                                               prevsib->next = newchild;
<                                               newchild->prev = prevsib;
<                                       } else {
<                                               nodep->children = newchild;
<                                       }
---
>                                       prevsib->next = newchild;
>                                       newchild->prev = prevsib;

(Note: fix and works are in quotes because a) i know very very little C, b) i
know nothing about working with zend extensions, and c) the Apache error_log is
reporting memory leaks due to my change :( )

I should also note that the code added for bug #32011 changes the behavior of
DocumentFragments slightly, in that prior to the fix, one was able to reference
the child nodes of a fragment after some node was replaced by the fragment. That
is no longer the case, as after the replacement, the fragment has no child nodes.

Now, that may in fact be the proper way to handle things... however, it was very
handy to be able to replace a single node with multiple top level nodes (what
DocumentFragments are good for in the first place), and then being able to
further process all those top level nodes by referencing the DocumentFragment,
rather than writing a bunch of code to try and figure out which nodes were newly
added.

-james