polotek/libxmljs — pull request #163 by skabbes (2012-09-05 06:20:33+00:00)
Title: Prevent SEGFAULT of XmlNodes after they are detached from the tree

[This](https://github.com/polotek/libxmljs/commit/4f7255d4c5d1c8e2a393560603def146cf758186) commit introduced a bug which allows detached XmlNodes to be double free'd. I've done some research, and this commit is definitely necessary since `xmlUnlinkNode` does **not** free that memory, and neither does `xmlFreeDoc`. However, we need a mechanism to prevent this behavior, since it is very difficult for end users to debug segfaults (no stack traces provided by Node).

Repro Steps are laid out in the included test case, but basically go like this:

1. Access children of node `a` (so that those nodes are being reference tracked by v8)
2. Remove node `a` from the dom tree (which also remove all children nodes, including attributes)
3. Trigger a garbage collection after all those variables are no longer in scope
4. Calling xmlFreeNode in `a`'s destructor recursively destroys the xml tree of its children (since `a->parent == NULL`)
5. Calling xmlFreeNode on a's children cause them to be freed again (since `child->parent != NULL`)
6. However, `xml_obj->doc->_private` is no longer a valid reference to an XmlDocument, so the unref causes a SEGFAULT

### Possible Solutions

* Simply check `xml_obj->type == -1`, since libxml [automatically resets all memory to -1 after it free's it](http://git.gnome.org/browse/libxml2/tree/xmlmemory.c?id=v2.7.8#n438). However, this is very much implementation dependent and doesn't allow us to property `unref` the parent document (since the doc pointer is also overwritten)

* Store a pointer to the xmlDoc on the XmlNode object and compare that to `xml_obj->doc` before calling xmlFreeNode on that node. This assumes that the xmlDoc will never be free'd before any of its children nodes (should be correct). It also means that we need to maintain that pointer for every operation which may change the node's document (addPrevSibling, addNextSibling off the top of my head)

* Keep track of which nodes have been removed in XmlDocument, and free them explicitly when ~XmlDocument is called. The code would have to be smart enough to remove nodes from this mapping when they are re-added to the tree.

* Immediately call xmlUnlinkNode and xmlFreeNode inside `XmlNode::remove` and make the api enforce that you cannot reuse any removed node or any of its children.

I'm not super familiar with libxml or v8 or what's possible with them, so maybe someone here who understands the problem can weigh in @shtylman?