Notify wrappers when namespaces are freed

* Avoid segfault when ns wrapper destructor traverses freed pointer * Also, wrappers only unref contexts they have ref'd
libxmljs · Nov 8, 2015 · 9cbf68f · 9cbf68f
1 parent 9791526
commit 9cbf68f
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 8 deletions.
diff --git a/src/libxmljs.cc b/src/libxmljs.cc
@@ -8,6 +8,7 @@
 #include "xml_document.h"
 #include "xml_node.h"
 #include "xml_sax_parser.h"
+#include "xml_namespace.h"
 
 namespace libxmljs {
 
@@ -97,9 +98,53 @@ char* xmlMemoryStrdupWrap(const char* str)
     return res;
 }
 
-// callback function for `xmlDeregisterNodeDefault`
+void deregisterNsList(xmlNs* ns)
+{
+    while (ns != NULL) {
+        if (ns->_private != NULL) {
+            XmlNamespace* wrapper = static_cast<XmlNamespace*>(ns->_private);
+            wrapper->xml_obj = NULL;
+            ns->_private = NULL;
+        }
+        ns = ns->next;
+    }
+}
+
+void deregisterNodeNamespaces(xmlNode* xml_obj)
+{
+    xmlNs* ns = NULL;
+    if ((xml_obj->type == XML_DOCUMENT_NODE) ||
+#ifdef LIBXML_DOCB_ENABLED
+        (xml_obj->type == XML_DOCB_DOCUMENT_NODE) ||
+#endif
+        (xml_obj->type == XML_HTML_DOCUMENT_NODE)) {
+        ns = reinterpret_cast<xmlDoc*>(xml_obj)->oldNs;
+    }
+    else if ((xml_obj->type == XML_ELEMENT_NODE) ||
+             (xml_obj->type == XML_XINCLUDE_START) ||
+             (xml_obj->type == XML_XINCLUDE_END)) {
+        ns = xml_obj->nsDef;
+    }
+    if (ns != NULL) {
+        deregisterNsList(ns);
+    }
+}
+
+/*
+ * Before libxmljs nodes are freed, they are passed to the deregistration
+ * callback, (configured by `xmlDeregisterNodeDefault`).
+ *
+ * In deregistering each node, we update any wrapper (e.g. `XmlElement`,
+ * `XmlAttribute`) to ensure that when it is destroyed, it doesn't try to
+ * access the freed memory.
+ *
+ * Because namespaces (`xmlNs`) attached to nodes are also freed and may be
+ * wrapped, it is necessary to update any wrappers (`XmlNamespace`) which have
+ * been created for attached namespaces.
+ */
 void xmlDeregisterNodeCallback(xmlNode* xml_obj)
 {
+    deregisterNodeNamespaces(xml_obj);
     if (xml_obj->_private)
     {
         XmlNode* node = static_cast<XmlNode*>(xml_obj->_private);

diff --git a/src/xml_namespace.cc b/src/xml_namespace.cc
@@ -65,28 +65,49 @@ XmlNamespace::XmlNamespace(xmlNs* node) : xml_obj(node)
 {
     xml_obj->_private = this;
 
-    if (xml_obj->context)
+    /*
+     * If a context is present and wrapped, increment its refcount to ensure
+     * that it is considered accessible from javascript for as long as the
+     * namespace is accessible.
+     */
+    if ((xml_obj->context) && (xml_obj->context->_private != NULL))
     {
+        this->context = xml_obj->context;
         // a namespace must be created on a given node
         XmlDocument* doc = static_cast<XmlDocument*>(xml_obj->context->_private);
         doc->ref();
     }
+    else {
+        this->context = NULL;
+    }
 }
 
 XmlNamespace::~XmlNamespace()
 {
-    xml_obj->_private = NULL;
+    /*
+     * `xml_obj` may have been nulled by `xmlDeregisterNodeCallback` when
+     * the `xmlNs` was freed along with an attached node or document.
+     */
+    if (xml_obj != NULL) {
+        xml_obj->_private = NULL;
+    }
 
-    if (xml_obj->context)
+    /*
+     * The context pointer is only set if this wrapper has incremented the
+     * refcount of the context wrapper.
+     */
+    if (this->context != NULL)
     {
-        // release the hold and allow the document to be freed
-        XmlDocument* doc = static_cast<XmlDocument*>(xml_obj->context->_private);
-        doc->unref();
+        if (this->context->_private != NULL) {
+            // release the hold and allow the document to be freed
+            XmlDocument* doc = static_cast<XmlDocument*>(this->context->_private);
+            doc->unref();
+        }
+        this->context = NULL;
     }
 
     // We do not free the xmlNode here. It could still be part of a document
     // It will be freed when the doc is freed
-    // xmlFree(xml_obj);
 }
 
 NAN_METHOD(XmlNamespace::Href) {

diff --git a/src/xml_namespace.h b/src/xml_namespace.h
@@ -14,6 +14,8 @@ class XmlNamespace : public Nan::ObjectWrap {
 
     xmlNs* xml_obj;
 
+    xmlDoc* context; // reference-managed context
+
     static void Initialize(v8::Handle<v8::Object> target);
     static Nan::Persistent<v8::FunctionTemplate> constructor_template;
 

diff --git a/test/ref_integrity.js b/test/ref_integrity.js
@@ -48,3 +48,16 @@ module.exports.double_free = function(assert) {
     assert.ok( children[0].attrs() );
     assert.done();
 };
+
+module.exports.freed_namespace_unwrappable = function(assert) {
+    var doc = libxml.parseXml("<?xml version='1.0' encoding='UTF-8'?><root></root>");
+    var el = new libxml.Element(doc, "foo");
+    var ns = el.namespace("bar", null);
+    el = null;
+    global.gc();
+    ns = null;
+    global.gc();
+    assert.done();
+};
+
+