XML: release parser context after parsing

Previously, XML parser contexts were retained with their parsed documents.
When parsing failed in the njs engine, no cleanup was registered, leaking
the context and allowing repeated malformed XML input to grow memory usage
in long-lived NGINX worker processes.

The fix is to limit parser contexts to the parse operation and release
them immediately after parsing or error reporting. The VM cleanup now owns
only the parsed document.

diff --git a/external/njs_xml_module.c b/external/njs_xml_module.c
index fce06d907c4d937731b6f8d0e91494c9c4afeceb..d58e222c0c3cc5128752caaaaced9894e54973bf 100644 (file)
--- a/external/njs_xml_module.c
+++ b/external/njs_xml_module.c
@@ -13,12 +13,6 @@
 #include <libxml/xpathInternals.h>
 
 
-typedef struct {
-    xmlDoc         *doc;
-    xmlParserCtxt  *ctx;
-} njs_xml_doc_t;
-
-
 typedef enum {
     XML_NSET_TREE = 0,
     XML_NSET_TREE_NO_COMMENTS,
@@ -130,7 +124,7 @@ static njs_xml_nset_t *njs_xml_nset_create(njs_vm_t *vm, xmlDoc *doc,
 static njs_xml_nset_t *njs_xml_nset_add(njs_xml_nset_t *nset,
     njs_xml_nset_t *add);
 static void njs_xml_nset_cleanup(void *data);
-static void njs_xml_error(njs_vm_t *vm, njs_xml_doc_t *tree, const char *fmt,
+static void njs_xml_error(njs_vm_t *vm, xmlParserCtxt *ctx, const char *fmt,
     ...);
 static njs_int_t njs_xml_init(njs_vm_t *vm);
 
@@ -456,9 +450,10 @@ static njs_int_t
 njs_xml_ext_parse(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
     njs_index_t unused, njs_value_t *retval)
 {
+    xmlDoc            *doc;
     njs_int_t         ret;
     njs_str_t         data;
-    njs_xml_doc_t     *tree;
+    xmlParserCtxt     *ctx;
     njs_mp_cleanup_t  *cln;
 
     ret = njs_vm_value_to_bytes(vm, &data, njs_arg(args, nargs, 1));
@@ -466,50 +461,51 @@ njs_xml_ext_parse(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
         return NJS_ERROR;
     }
 
-    tree = njs_mp_zalloc(njs_vm_memory_pool(vm), sizeof(njs_xml_doc_t));
-    if (njs_slow_path(tree == NULL)) {
-        njs_vm_memory_error(vm);
-        return NJS_ERROR;
-    }
-
-    tree->ctx = xmlNewParserCtxt();
-    if (njs_slow_path(tree->ctx == NULL)) {
+    ctx = xmlNewParserCtxt();
+    if (njs_slow_path(ctx == NULL)) {
         njs_vm_internal_error(vm, "xmlNewParserCtxt() failed");
         return NJS_ERROR;
     }
 
-    tree->doc = xmlCtxtReadMemory(tree->ctx, (char *) data.start, data.length,
-                                  NULL, NULL, XML_PARSE_NOWARNING
-                                              | XML_PARSE_NOERROR);
-    if (njs_slow_path(tree->doc == NULL)) {
-        njs_xml_error(vm, tree, "failed to parse XML");
-        return NJS_ERROR;
+    doc = xmlCtxtReadMemory(ctx, (char *) data.start, data.length, NULL, NULL,
+                            XML_PARSE_NOWARNING | XML_PARSE_NOERROR);
+    if (njs_slow_path(doc == NULL)) {
+        njs_xml_error(vm, ctx, "failed to parse XML");
+        goto failed;
     }
 
+    xmlFreeParserCtxt(ctx);
+
     cln = njs_mp_cleanup_add(njs_vm_memory_pool(vm), 0);
     if (njs_slow_path(cln == NULL)) {
         njs_vm_memory_error(vm);
+        xmlFreeDoc(doc);
         return NJS_ERROR;
     }
 
     cln->handler = njs_xml_doc_cleanup;
-    cln->data = tree;
+    cln->data = doc;
+
+    return njs_vm_external_create(vm, retval, njs_xml_doc_proto_id, doc, 0);
 
-    return njs_vm_external_create(vm, retval, njs_xml_doc_proto_id,
-                                  tree, 0);
+failed:
+
+    xmlFreeParserCtxt(ctx);
+
+    return NJS_ERROR;
 }
 
 
 static njs_int_t
 njs_xml_doc_ext_prop_keys(njs_vm_t *vm, njs_value_t *value, njs_value_t *keys)
 {
-    xmlNode        *node;
-    njs_int_t      ret;
-    njs_value_t    *push;
-    njs_xml_doc_t  *tree;
+    xmlDoc       *doc;
+    xmlNode      *node;
+    njs_int_t    ret;
+    njs_value_t  *push;
 
-    tree = njs_vm_external(vm, njs_xml_doc_proto_id, value);
-    if (njs_slow_path(tree == NULL)) {
+    doc = njs_vm_external(vm, njs_xml_doc_proto_id, value);
+    if (njs_slow_path(doc == NULL)) {
         njs_value_undefined_set(keys);
         return NJS_DECLINED;
     }
@@ -519,7 +515,7 @@ njs_xml_doc_ext_prop_keys(njs_vm_t *vm, njs_value_t *value, njs_value_t *keys)
         return NJS_ERROR;
     }
 
-    for (node = xmlDocGetRootElement(tree->doc);
+    for (node = xmlDocGetRootElement(doc);
          node != NULL;
          node = node->next)
     {
@@ -547,14 +543,14 @@ static njs_int_t
 njs_xml_doc_ext_root(njs_vm_t *vm, njs_object_prop_t *prop, uint32_t atom_id,
      njs_value_t *value, njs_value_t *unused, njs_value_t *retval)
 {
-    xmlNode        *node;
-    njs_int_t      ret;
-    njs_str_t      name;
-    njs_bool_t     any;
-    njs_xml_doc_t  *tree;
-
-    tree = njs_vm_external(vm, njs_xml_doc_proto_id, value);
-    if (njs_slow_path(tree == NULL)) {
+    xmlDoc      *doc;
+    xmlNode     *node;
+    njs_int_t   ret;
+    njs_str_t   name;
+    njs_bool_t  any;
+
+    doc = njs_vm_external(vm, njs_xml_doc_proto_id, value);
+    if (njs_slow_path(doc == NULL)) {
         njs_value_undefined_set(retval);
         return NJS_DECLINED;
     }
@@ -574,7 +570,7 @@ njs_xml_doc_ext_root(njs_vm_t *vm, njs_object_prop_t *prop, uint32_t atom_id,
         name.start = NULL;
     }
 
-    for (node = xmlDocGetRootElement(tree->doc);
+    for (node = xmlDocGetRootElement(doc);
          node != NULL;
          node = node->next)
     {
@@ -1489,18 +1485,18 @@ error:
 static xmlNode *
 njs_xml_external_node(njs_vm_t *vm, njs_value_t *value)
 {
-    xmlNode        *current;
-    njs_xml_doc_t  *tree;
+    xmlDoc   *doc;
+    xmlNode  *current;
 
     current = njs_vm_external(vm, njs_xml_node_proto_id, value);
     if (njs_slow_path(current == NULL)) {
-        tree = njs_vm_external(vm, njs_xml_doc_proto_id, value);
-        if (njs_slow_path(tree == NULL)) {
+        doc = njs_vm_external(vm, njs_xml_doc_proto_id, value);
+        if (njs_slow_path(doc == NULL)) {
             njs_vm_type_error(vm, "\"this\" is not a XMLNode object");
             return NULL;
         }
 
-        current = xmlDocGetRootElement(tree->doc);
+        current = xmlDocGetRootElement(doc);
         if (njs_slow_path(current == NULL)) {
             njs_vm_type_error(vm, "\"this\" is not a XMLNode object");
             return NULL;
@@ -1767,12 +1763,7 @@ njs_xml_node_cleanup(void *data)
 static void
 njs_xml_doc_cleanup(void *data)
 {
-    njs_xml_doc_t  *current;
-
-    current = data;
-
-    xmlFreeDoc(current->doc);
-    xmlFreeParserCtxt(current->ctx);
+    xmlFreeDoc(data);
 }
 
 
@@ -2238,7 +2229,7 @@ njs_xml_nset_cleanup(void *data)
 
 
 static void
-njs_xml_error(njs_vm_t *vm, njs_xml_doc_t *current, const char *fmt, ...)
+njs_xml_error(njs_vm_t *vm, xmlParserCtxt *ctx, const char *fmt, ...)
 {
     u_char          *p, *last;
     va_list         args;
@@ -2251,7 +2242,7 @@ njs_xml_error(njs_vm_t *vm, njs_xml_doc_t *current, const char *fmt, ...)
     p = njs_vsprintf(errstr, last - 1, fmt, args);
     va_end(args);
 
-    err = xmlCtxtGetLastError(current->ctx);
+    err = xmlCtxtGetLastError(ctx);
 
     if (err != NULL) {
         p = njs_sprintf(p, last - 1, " (libxml2: \"%*s\" at %d:%d)",

diff --git a/external/qjs_xml_module.c b/external/qjs_xml_module.c
index 29dd14919d651af6fae61c157f5dead5ffa3882a..e210ad9eda0e382e465a7277558083cbd784cdb9 100644 (file)
--- a/external/qjs_xml_module.c
+++ b/external/qjs_xml_module.c
@@ -22,7 +22,6 @@ struct qjs_xml_retired_s {
 
 typedef struct {
     xmlDoc             *doc;
-    xmlParserCtxt      *ctx;
     qjs_xml_retired_t  *retired;
     int                ref_count;
 } qjs_xml_doc_t;
@@ -138,7 +137,7 @@ static int qjs_xml_list_retire(JSContext *cx, qjs_xml_doc_t *doc,
 static void qjs_xml_list_append(xmlNode *parent, xmlNode *node);
 static int qjs_xml_tree_has_namespaces(xmlNode *node);
 
-static void qjs_xml_error(JSContext *cx, qjs_xml_doc_t *current,
+static void qjs_xml_error(JSContext *cx, xmlParserCtxt *ctx,
     const char *fmt, ...);
 static JSModuleDef *qjs_xml_init(JSContext *ctx, const char *name);
 
@@ -221,6 +220,7 @@ qjs_xml_parse(JSContext *cx, JSValueConst this_val, int argc,
 {
     JSValue        ret;
     qjs_bytes_t    data;
+    xmlParserCtxt  *ctx;
     qjs_xml_doc_t  *tree;
 
     if (qjs_to_bytes(cx, &data, argv[0]) < 0) {
@@ -236,24 +236,27 @@ qjs_xml_parse(JSContext *cx, JSValueConst this_val, int argc,
 
     tree->ref_count = 1;
 
-    tree->ctx = xmlNewParserCtxt();
-    if (tree->ctx == NULL) {
+    ctx = xmlNewParserCtxt();
+    if (ctx == NULL) {
         qjs_bytes_free(cx, &data);
         JS_ThrowInternalError(cx, "xmlNewParserCtxt() failed");
         qjs_xml_doc_free(JS_GetRuntime(cx), tree);
         return JS_EXCEPTION;
     }
 
-    tree->doc = xmlCtxtReadMemory(tree->ctx, (char *) data.start, data.length,
-                                  NULL, NULL, XML_PARSE_NOWARNING
-                                              | XML_PARSE_NOERROR);
+    tree->doc = xmlCtxtReadMemory(ctx, (char *) data.start, data.length,
+                                   NULL, NULL, XML_PARSE_NOWARNING
+                                               | XML_PARSE_NOERROR);
     qjs_bytes_free(cx, &data);
     if (tree->doc == NULL) {
-        qjs_xml_error(cx, tree, "failed to parse XML");
+        qjs_xml_error(cx, ctx, "failed to parse XML");
+        xmlFreeParserCtxt(ctx);
         qjs_xml_doc_free(JS_GetRuntime(cx), tree);
         return JS_EXCEPTION;
     }
 
+    xmlFreeParserCtxt(ctx);
+
     ret = JS_NewObjectClass(cx, QJS_CORE_CLASS_ID_XML_DOC);
     if (JS_IsException(ret)) {
         qjs_xml_doc_free(JS_GetRuntime(cx), tree);
@@ -560,10 +563,6 @@ qjs_xml_doc_free(JSRuntime *rt, qjs_xml_doc_t *current)
         xmlFreeDoc(current->doc);
     }
 
-    if (current->ctx != NULL) {
-        xmlFreeParserCtxt(current->ctx);
-    }
-
     js_free_rt(rt, current);
 }
 
@@ -2427,7 +2426,7 @@ qjs_xml_tree_has_namespaces(xmlNode *node)
 
 
 static void
-qjs_xml_error(JSContext *cx, qjs_xml_doc_t *current, const char *fmt, ...)
+qjs_xml_error(JSContext *cx, xmlParserCtxt *ctx, const char *fmt, ...)
 {
     u_char          *p, *last;
     va_list         args;
@@ -2440,7 +2439,7 @@ qjs_xml_error(JSContext *cx, qjs_xml_doc_t *current, const char *fmt, ...)
     p = njs_vsprintf(errstr, last - 1, fmt, args);
     va_end(args);
 
-    err = xmlCtxtGetLastError(current->ctx);
+    err = xmlCtxtGetLastError(ctx);
 
     if (err != NULL) {
         p = njs_sprintf(p, last - 1, " (libxml2: \"%*s\" at %d:%d)",

diff --git a/test/xml/xml.t.mjs b/test/xml/xml.t.mjs
index ed6078c88944872e841da48e268793471806c2f7..220c4b7e01321910fb0cdd78f08b687a3ad05098 100644 (file)
--- a/test/xml/xml.t.mjs
+++ b/test/xml/xml.t.mjs
@@ -188,6 +188,33 @@ let parse_tsuite = {
           exception: 'Error: failed to parse XML (libxml2: "Start tag expected, \'<\' not found" at 1:1)' },
 ]};
 
+let parse_failure_tsuite = {
+    name: "parse() failures",
+    skip: () => (!has_njs()),
+    T: async (params) => {
+        for (let i = 0; i < params.count; i++) {
+            try {
+                xml.parse(params.doc);
+            } catch (e) {
+                continue;
+            }
+
+            throw Error('missing exception');
+        }
+
+        return 'SUCCESS';
+    },
+
+    opts: {
+        count: 2,
+        doc: 'GARBAGE',
+    },
+
+    tests: [
+        {},
+    ],
+};
+
 let c14n_tsuite = {
     name: "c14n()",
     skip: () => (!has_njs()),
@@ -920,6 +947,7 @@ let modify_tsuite = {
 
 run([
     parse_tsuite,
+    parse_failure_tsuite,
     c14n_tsuite,
     modify_tsuite,
 ])