XML: create text nodes before mutation

Previously, text updates encoded content manually and then passed it to
libxml2. This duplicated libxml2 escaping rules and allocated the
encoded buffer before creating the text node.

The fix is to create a libxml2 text node from the unencoded value before
replacing the copied node's children. libxml2 now owns escaping, and a
failure leaves the original XML tree unchanged.

diff --git a/external/njs_xml_module.c b/external/njs_xml_module.c
index f6687ad89ce86943020d5b1437065ff56b198317..f37f887991d963991ddc28f0a050da51816d5f26 100644 (file)
--- a/external/njs_xml_module.c
+++ b/external/njs_xml_module.c
@@ -108,8 +108,6 @@ static njs_int_t njs_xml_str_to_c_string(njs_vm_t *vm, njs_str_t *str,
     u_char *dst, size_t size);
 static const u_char *njs_xml_value_to_c_string(njs_vm_t *vm, njs_value_t *value,
     u_char *dst, size_t size);
-static njs_int_t njs_xml_encode_special_chars(njs_vm_t *vm, njs_str_t *src,
-    njs_str_t *out);
 static njs_int_t njs_xml_replace_node(njs_vm_t *vm, njs_value_t *value,
     xmlNode *current);
 static void njs_xml_node_cleanup(void *data);
@@ -998,9 +996,9 @@ njs_xml_node_ext_text(njs_vm_t *vm, njs_object_prop_t *unused, uint32_t unused1,
     njs_value_t *value, njs_value_t *setval, njs_value_t *retval)
 {
     u_char     *text;
-    xmlNode    *current, *copy;
+    xmlNode    *current, *copy, *node;
     njs_int_t  ret;
-    njs_str_t  content, enc;
+    njs_str_t  content;
 
     current = njs_vm_external(vm, njs_xml_node_proto_id, value);
     if (njs_slow_path(current == NULL)) {
@@ -1019,8 +1017,8 @@ njs_xml_node_ext_text(njs_vm_t *vm, njs_object_prop_t *unused, uint32_t unused1,
 
     /* set or delete. */
 
-    enc.start = NULL;
-    enc.length = 0;
+    content.start = NULL;
+    content.length = 0;
 
     if (retval != NULL
         && (setval != NULL && !njs_value_is_null_or_undefined(setval)))
@@ -1032,8 +1030,8 @@ njs_xml_node_ext_text(njs_vm_t *vm, njs_object_prop_t *unused, uint32_t unused1,
 
         njs_value_string_get(vm, setval, &content);
 
-        ret = njs_xml_encode_special_chars(vm, &content, &enc);
-        if (njs_slow_path(ret != NJS_OK)) {
+        if (njs_slow_path(memchr(content.start, '\0', content.length) != NULL)) {
+            njs_vm_type_error(vm, "setval contains NUL");
             return NJS_ERROR;
         }
     }
@@ -1044,7 +1042,25 @@ njs_xml_node_ext_text(njs_vm_t *vm, njs_object_prop_t *unused, uint32_t unused1,
         return NJS_ERROR;
     }
 
-    xmlNodeSetContentLen(copy, enc.start, enc.length);
+    node = NULL;
+    if (retval != NULL && setval != NULL) {
+        node = xmlNewDocTextLen(current->doc, content.start, content.length);
+        if (njs_slow_path(node == NULL)) {
+            xmlFreeNode(copy);
+            njs_vm_internal_error(vm, "xmlNewDocTextLen() failed");
+            return NJS_ERROR;
+        }
+    }
+
+    xmlFreeNodeList(copy->children);
+    copy->children = node;
+    copy->last = node;
+
+    if (node != NULL) {
+        node->parent = copy;
+        node->prev = NULL;
+        node->next = NULL;
+    }
 
     if (retval != NULL) {
         njs_value_undefined_set(retval);
@@ -1428,90 +1444,6 @@ njs_xml_value_to_c_string(njs_vm_t *vm, njs_value_t *value, u_char *dst,
 }
 
 
-static njs_int_t
-njs_xml_encode_special_chars(njs_vm_t *vm, njs_str_t *src, njs_str_t *out)
-{
-    u_char  *p, *dst, *end;
-    size_t   len;
-
-    len = 0;
-    end = src->start + src->length;
-
-    for (p = src->start; p < end; p++) {
-        if (*p == '<' || *p == '>') {
-            len += njs_length("&lt");
-        }
-
-        if (*p == '&' || *p == '\r') {
-            len += njs_length("&amp");
-        }
-
-        if (*p == '"') {
-            len += njs_length("&quot");
-        }
-
-        len += 1;
-    }
-
-    if (njs_fast_path(len == src->length)) {
-        *out = *src;
-        return NJS_OK;
-    }
-
-    out->start = njs_mp_alloc(njs_vm_memory_pool(vm), len);
-    if (njs_slow_path(out->start == NULL)) {
-        njs_vm_memory_error(vm);
-        return NJS_ERROR;
-    }
-
-    dst = out->start;
-
-    for (p = src->start; p < end; p++) {
-        if (*p == '<') {
-            *dst++ = '&';
-            *dst++ = 'l';
-            *dst++ = 't';
-            *dst++ = ';';
-
-        } else if (*p == '>') {
-            *dst++ = '&';
-            *dst++ = 'g';
-            *dst++ = 't';
-            *dst++ = ';';
-
-        } else if (*p == '&') {
-            *dst++ = '&';
-            *dst++ = 'a';
-            *dst++ = 'm';
-            *dst++ = 'p';
-            *dst++ = ';';
-
-        } else if (*p == '"') {
-            *dst++ = '&';
-            *dst++ = 'q';
-            *dst++ = 'u';
-            *dst++ = 'o';
-            *dst++ = 't';
-            *dst++ = ';';
-
-        } else if (*p == '\r') {
-            *dst++ = '&';
-            *dst++ = '#';
-            *dst++ = '1';
-            *dst++ = '3';
-            *dst++ = ';';
-
-        } else {
-            *dst++ = *p;
-        }
-    }
-
-    out->length = len;
-
-    return NJS_OK;
-}
-
-
 static njs_int_t
 njs_xml_replace_node(njs_vm_t *vm, njs_value_t *value, xmlNode *current)
 {

diff --git a/external/qjs_xml_module.c b/external/qjs_xml_module.c
index 408b6b0475d7c29d200b2c3188d0d5b6542945b6..f78468488b3ab8b88cc8f1f437356eb3ff74b8ae 100644 (file)
--- a/external/qjs_xml_module.c
+++ b/external/qjs_xml_module.c
@@ -109,8 +109,6 @@ static qjs_xml_nset_t *qjs_xml_nset_create(JSContext *cx, xmlDoc *doc,
 static qjs_xml_nset_t *qjs_xml_nset_add(qjs_xml_nset_t *nset,
     qjs_xml_nset_t *add);
 static void qjs_xml_nset_free(JSContext *cx, qjs_xml_nset_t *nset);
-static int qjs_xml_encode_special_chars(JSContext *cx, njs_str_t *src,
-    njs_str_t *out);
 static void qjs_xml_replace_node(JSContext *cx, qjs_xml_node_t *node,
     xmlNode *current);
 
@@ -864,12 +862,12 @@ error:
 static int
 qjs_xml_node_text_handler(JSContext *cx, JSValue current, JSValue setval)
 {
-    xmlNode         *copy;
-    njs_str_t       content, enc;
+    xmlNode         *copy, *text;
+    njs_str_t       content;
     qjs_xml_node_t  *node;
 
-    enc.start = NULL;
-    enc.length = 0;
+    content.start = NULL;
+    content.length = 0;
 
     node = JS_GetOpaque(current, QJS_CORE_CLASS_ID_XML_NODE);
     if (node == NULL) {
@@ -882,28 +880,42 @@ qjs_xml_node_text_handler(JSContext *cx, JSValue current, JSValue setval)
             return -1;
         }
 
-        if (qjs_xml_encode_special_chars(cx, &content, &enc) < 0) {
+        if (memchr(content.start, '\0', content.length) != NULL) {
             JS_FreeCString(cx, (char *) content.start);
+            JS_ThrowTypeError(cx, "setval contains NUL");
             return -1;
         }
-
-        JS_FreeCString(cx, (char *) content.start);
     }
 
     copy = xmlDocCopyNode(node->node, node->doc->doc, 1);
     if (copy == NULL) {
-        if (enc.start != NULL) {
-            js_free(cx, enc.start);
+        if (!JS_IsNullOrUndefined(setval)) {
+            JS_FreeCString(cx, (char *) content.start);
         }
 
         JS_ThrowInternalError(cx, "xmlDocCopyNode() failed");
         return -1;
     }
 
-    xmlNodeSetContentLen(copy, enc.start, enc.length);
+    text = NULL;
+    if (!JS_IsNullOrUndefined(setval)) {
+        text = xmlNewDocTextLen(node->doc->doc, content.start, content.length);
+        JS_FreeCString(cx, (char *) content.start);
+        if (text == NULL) {
+            xmlFreeNode(copy);
+            JS_ThrowInternalError(cx, "xmlNewDocTextLen() failed");
+            return -1;
+        }
+    }
 
-    if (enc.start != NULL) {
-        js_free(cx, enc.start);
+    xmlFreeNodeList(copy->children);
+    copy->children = text;
+    copy->last = text;
+
+    if (text != NULL) {
+        text->parent = copy;
+        text->prev = NULL;
+        text->next = NULL;
     }
 
     qjs_xml_replace_node(cx, node, copy);
@@ -1955,92 +1967,6 @@ qjs_xml_nset_free(JSContext *cx, qjs_xml_nset_t *nset)
 }
 
 
-static int
-qjs_xml_encode_special_chars(JSContext *cx, njs_str_t *src, njs_str_t *out)
-{
-    size_t  len;
-    u_char  *p, *dst, *end;
-
-    len = 0;
-    end = src->start + src->length;
-
-    for (p = src->start; p < end; p++) {
-        if (*p == '<' || *p == '>') {
-            len += njs_length("&lt");
-        }
-
-        if (*p == '&' || *p == '\r') {
-            len += njs_length("&amp");
-        }
-
-        if (*p == '"') {
-            len += njs_length("&quot");
-        }
-
-        len += 1;
-    }
-
-    if (len == 0) {
-        out->start = NULL;
-        out->length = 0;
-
-        return 0;
-    }
-
-    out->start = js_malloc(cx, len);
-    if (out->start == NULL) {
-        JS_ThrowOutOfMemory(cx);
-        return -1;
-    }
-
-    dst = out->start;
-
-    for (p = src->start; p < end; p++) {
-        if (*p == '<') {
-            *dst++ = '&';
-            *dst++ = 'l';
-            *dst++ = 't';
-            *dst++ = ';';
-
-        } else if (*p == '>') {
-            *dst++ = '&';
-            *dst++ = 'g';
-            *dst++ = 't';
-            *dst++ = ';';
-
-        } else if (*p == '&') {
-            *dst++ = '&';
-            *dst++ = 'a';
-            *dst++ = 'm';
-            *dst++ = 'p';
-            *dst++ = ';';
-
-        } else if (*p == '"') {
-            *dst++ = '&';
-            *dst++ = 'q';
-            *dst++ = 'u';
-            *dst++ = 'o';
-            *dst++ = 't';
-            *dst++ = ';';
-
-        } else if (*p == '\r') {
-            *dst++ = '&';
-            *dst++ = '#';
-            *dst++ = '1';
-            *dst++ = '3';
-            *dst++ = ';';
-
-        } else {
-            *dst++ = *p;
-        }
-    }
-
-    out->length = len;
-
-    return 0;
-}
-
-
 static void
 qjs_xml_replace_node(JSContext *cx, qjs_xml_node_t *node, xmlNode *current)
 {

diff --git a/test/xml/xml.t.mjs b/test/xml/xml.t.mjs
index 75ce9c4ccab35687dd0b5cb3febdf655b6c3215b..015f7292e31e8a07b64966b189546382f7af0d16 100644 (file)
--- a/test/xml/xml.t.mjs
+++ b/test/xml/xml.t.mjs
@@ -252,6 +252,16 @@ let modify_tsuite = {
             return xml.serializeToString(doc);
           },
           expected: `<note></note>` },
+        { get: (doc) => {
+            try {
+                doc.note.setText('before\0after');
+            } catch (e) {
+                return [e instanceof TypeError, doc.note.$text].toString();
+            }
+
+            return 'missing exception';
+          },
+          expected: 'true,ToveJani' },
         { get: (doc) => {
             doc.note.setText(null);
             return doc.note.$text;