From 77f8e260c04b2402006a4b644c86e1af79c87b7e Mon Sep 17 00:00:00 2001 From: Dmitry Volyntsev Date: Wed, 16 Sep 2026 15:03:40 -0700 Subject: [PATCH] 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. --- external/njs_xml_module.c | 99 ++++++++++++++++++--------------------- external/qjs_xml_module.c | 27 +++++------ test/xml/xml.t.mjs | 28 +++++++++++ 3 files changed, 86 insertions(+), 68 deletions(-) diff --git a/external/njs_xml_module.c b/external/njs_xml_module.c index fce06d90..d58e222c 100644 --- a/external/njs_xml_module.c +++ b/external/njs_xml_module.c @@ -13,12 +13,6 @@ #include -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 29dd1491..e210ad9e 100644 --- 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 ed6078c8..220c4b7e 100644 --- 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, ]) -- 2.47.3