Possible error with DOMNode::replaceChild() in 5.0.4 View as Markdown

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