QuickJS: fix consumed value cleanup

QuickJS property setter and define APIs consume the JSValue argument on
both success and failure.  This applies to JS_SetProperty(),
JS_SetPropertyStr(), JS_SetPropertyUint32(), JS_DefinePropertyValue(),
JS_DefinePropertyValueStr(), and JS_DefinePropertyValueUint32().

Do not free values after passing them to these APIs, including failure
paths in shared cleanup labels.  Target objects and containers are not
consumed by these calls and still need normal cleanup unless ownership
has been transferred by inserting them into another object.

Also fix adjacent cleanup paths that leaked target objects after a
setter consumed the value being attached.

Reported by R4mbb of KRsecurity.

diff --git a/external/qjs_fs_module.c b/external/qjs_fs_module.c
index b9f8a360742a9600e068754f008481dfc398074c..49c2e65e93d0088ef78cc4de3ad72881c65fa221 100644 (file)
--- a/external/qjs_fs_module.c
+++ b/external/qjs_fs_module.c
@@ -1074,12 +1074,13 @@ done:
 
 
 static JSValue
-qjs_fs_dirent_create(JSContext *cx, JSValueConst name, struct dirent *entry)
+qjs_fs_dirent_create(JSContext *cx, JSValue name, struct dirent *entry)
 {
     JSValue  obj;
 
     obj = JS_NewObjectClass(cx, QJS_CORE_CLASS_ID_FS_DIRENT);
     if (JS_IsException(obj)) {
+        JS_FreeValue(cx, name);
         return JS_EXCEPTION;
     }
 
@@ -1243,7 +1244,6 @@ qjs_fs_readdir(JSContext *cx, JSValueConst this_val, int argc,
 
         if (!types) {
             if (JS_DefinePropertyValueUint32(cx, result, idx++, ename, 0) < 0) {
-                JS_FreeValue(cx, ename);
                 JS_FreeValue(cx, result);
                 goto done;
             }
@@ -1251,13 +1251,11 @@ qjs_fs_readdir(JSContext *cx, JSValueConst this_val, int argc,
         } else {
             v = qjs_fs_dirent_create(cx, ename, entry);
             if (JS_IsException(v)) {
-                JS_FreeValue(cx, ename);
                 JS_FreeValue(cx, result);
                 goto done;
             }
 
             if (JS_DefinePropertyValueUint32(cx, result, idx++, v, 0) < 0) {
-                JS_FreeValue(cx, ename);
                 JS_FreeValue(cx, result);
                 goto done;
             }
@@ -2106,7 +2104,6 @@ process:
                                       JS_PROP_C_W_E) < 0)
         {
             JS_FreeValue(cx, result);
-            JS_FreeValue(cx, buffer);
             result = JS_EXCEPTION;
             goto done;
         }

diff --git a/external/qjs_query_string_module.c b/external/qjs_query_string_module.c
index 8f695677a8c7d437784a46ebd5d875d2c7822a2b..128a17cd547580f7831a9d2f484a65587da250e5 100644 (file)
--- a/external/qjs_query_string_module.c
+++ b/external/qjs_query_string_module.c
@@ -400,6 +400,7 @@ qjs_query_string_append(JSContext *cx, JSValue object, const u_char *key,
 
     if (JS_IsUndefined(prev)) {
         if (JS_SetProperty(cx, object, prop, value) < 0) {
+            value = JS_UNDEFINED;
             goto exception;
         }
 
@@ -414,10 +415,12 @@ qjs_query_string_append(JSContext *cx, JSValue object, const u_char *key,
         JS_FreeValue(cx, length);
 
         if (JS_SetPropertyUint32(cx, prev, len, value) < 0) {
+            value = JS_UNDEFINED;
             goto exception;
         }
 
         JS_FreeValue(cx, prev);
+        prev = JS_UNDEFINED;
 
     } else {
         ret = JS_NewArray(cx);
@@ -426,6 +429,7 @@ qjs_query_string_append(JSContext *cx, JSValue object, const u_char *key,
         }
 
         if (JS_SetPropertyUint32(cx, ret, 0, prev) < 0) {
+            prev = JS_UNDEFINED;
             JS_FreeValue(cx, ret);
             goto exception;
         }
@@ -433,6 +437,7 @@ qjs_query_string_append(JSContext *cx, JSValue object, const u_char *key,
         prev = JS_UNDEFINED;
 
         if (JS_SetPropertyUint32(cx, ret, 1, value) < 0) {
+            value = JS_UNDEFINED;
             JS_FreeValue(cx, ret);
             goto exception;
         }
@@ -440,7 +445,6 @@ qjs_query_string_append(JSContext *cx, JSValue object, const u_char *key,
         value = JS_UNDEFINED;
 
         if (JS_SetProperty(cx, object, prop, ret) < 0) {
-            JS_FreeValue(cx, ret);
             goto exception;
         }
     }

diff --git a/external/qjs_webcrypto_module.c b/external/qjs_webcrypto_module.c
index 965918beabfd580043dd05abe288e9ea959441cc..42ca79757adf0b3fb983e73d19d8d14f1a124b79 100644 (file)
--- a/external/qjs_webcrypto_module.c
+++ b/external/qjs_webcrypto_module.c
@@ -137,7 +137,7 @@ static JSValue qjs_webcrypto_generate_key(JSContext *cx, JSValueConst this_val,
 static int qjs_webcrypto_generate_25519_keypair(JSContext *cx, int pkey_id,
     qjs_webcrypto_key_t *wkey, qjs_webcrypto_algorithm_t *alg,
     unsigned usage, int extractable, unsigned priv_usage,
-    unsigned pub_usage, JSValue key, JSValue *obj_ret);
+    unsigned pub_usage, JSValue *key, JSValue *obj_ret);
 #endif
 static JSValue qjs_webcrypto_import_key(JSContext *cx, JSValueConst this_val,
     int argc, JSValueConst *argv);
@@ -1429,7 +1429,6 @@ qjs_base64url_bignum_set(JSContext *cx, JSValue jwk, const char *key,
     }
 
     if (JS_DefinePropertyValueStr(cx, jwk, key, value, JS_PROP_C_W_E) < 0) {
-        JS_FreeValue(cx, value);
         return -1;
     }
 
@@ -1505,7 +1504,6 @@ qjs_export_jwk_rsa(JSContext *cx, qjs_webcrypto_key_t *key)
     }
 
     if (JS_DefinePropertyValueStr(cx, jwk, "alg", alg, JS_PROP_C_W_E) < 0) {
-        JS_FreeValue(cx, alg);
         goto fail;
     }
 
@@ -1667,7 +1665,6 @@ qjs_export_jwk_asymmetric(JSContext *cx, qjs_webcrypto_key_t *key)
 
     if (JS_DefinePropertyValueStr(cx, jwk, "key_ops", ops, JS_PROP_C_W_E) < 0) {
         JS_FreeValue(cx, jwk);
-        JS_FreeValue(cx, ops);
         return JS_EXCEPTION;
     }
 
@@ -1703,7 +1700,6 @@ qjs_export_jwk_oct(JSContext *cx, qjs_webcrypto_key_t *key)
     }
 
     if (JS_DefinePropertyValueStr(cx, jwk, "k", val, JS_PROP_C_W_E) < 0) {
-        JS_FreeValue(cx, val);
         goto fail;
     }
 
@@ -1737,7 +1733,6 @@ qjs_export_jwk_oct(JSContext *cx, qjs_webcrypto_key_t *key)
     }
 
     if (JS_DefinePropertyValueStr(cx, jwk, "alg", val, JS_PROP_C_W_E) < 0) {
-        JS_FreeValue(cx, val);
         goto fail;
     }
 
@@ -1747,7 +1742,6 @@ qjs_export_jwk_oct(JSContext *cx, qjs_webcrypto_key_t *key)
     }
 
     if (JS_DefinePropertyValueStr(cx, jwk, "key_ops", val, JS_PROP_C_W_E) < 0) {
-        JS_FreeValue(cx, val);
         goto fail;
     }
 
@@ -2666,7 +2660,7 @@ qjs_webcrypto_export_key(JSContext *cx, JSValueConst this_val, int argc,
 static int
 qjs_webcrypto_generate_25519_keypair(JSContext *cx, int pkey_id,
     qjs_webcrypto_key_t *wkey, qjs_webcrypto_algorithm_t *alg, unsigned usage,
-    int extractable, unsigned priv_usage, unsigned pub_usage, JSValue key,
+    int extractable, unsigned priv_usage, unsigned pub_usage, JSValue *key,
     JSValue *obj_ret)
 {
     JSValue              keypub, obj;
@@ -2719,12 +2713,15 @@ qjs_webcrypto_generate_25519_keypair(JSContext *cx, int pkey_id,
         return -1;
     }
 
-    if (JS_SetPropertyStr(cx, obj, "privateKey", key) < 0) {
+    if (JS_SetPropertyStr(cx, obj, "privateKey", *key) < 0) {
+        *key = JS_UNDEFINED;
         JS_FreeValue(cx, keypub);
         JS_FreeValue(cx, obj);
         return -1;
     }
 
+    *key = JS_UNDEFINED;
+
     if (JS_SetPropertyStr(cx, obj, "publicKey", keypub) < 0) {
         JS_FreeValue(cx, obj);
         return -1;
@@ -2752,6 +2749,7 @@ qjs_webcrypto_generate_key(JSContext *cx, JSValueConst this_val,
     options = argv[0];
     key = JS_UNDEFINED;
     keypub = JS_UNDEFINED;
+    obj = JS_UNDEFINED;
 
     alg = qjs_key_algorithm(cx, options);
     if (alg == NULL) {
@@ -2855,12 +2853,14 @@ qjs_webcrypto_generate_key(JSContext *cx, JSValueConst this_val,
         }
 
         if (JS_SetPropertyStr(cx, obj, "privateKey", key) < 0) {
+            key = JS_UNDEFINED;
             goto fail;
         }
 
         key = JS_UNDEFINED;
 
         if (JS_SetPropertyStr(cx, obj, "publicKey", keypub) < 0) {
+            keypub = JS_UNDEFINED;
             goto fail;
         }
 
@@ -2938,12 +2938,14 @@ qjs_webcrypto_generate_key(JSContext *cx, JSValueConst this_val,
         }
 
         if (JS_SetPropertyStr(cx, obj, "privateKey", key) < 0) {
+            key = JS_UNDEFINED;
             goto fail;
         }
 
         key = JS_UNDEFINED;
 
         if (JS_SetPropertyStr(cx, obj, "publicKey", keypub) < 0) {
+            keypub = JS_UNDEFINED;
             goto fail;
         }
 
@@ -2956,12 +2958,11 @@ qjs_webcrypto_generate_key(JSContext *cx, JSValueConst this_val,
                                                  extractable,
                                                  QJS_KEY_USAGE_SIGN,
                                                  QJS_KEY_USAGE_VERIFY,
-                                                 key, &obj);
+                                                 &key, &obj);
         if (n < 0) {
             goto fail;
         }
 
-        key = JS_UNDEFINED;
         break;
 
     case QJS_ALGORITHM_X25519:
@@ -2970,12 +2971,11 @@ qjs_webcrypto_generate_key(JSContext *cx, JSValueConst this_val,
                                                  extractable,
                                                  QJS_KEY_USAGE_DERIVE_KEY
                                                  | QJS_KEY_USAGE_DERIVE_BITS,
-                                                 0, key, &obj);
+                                                 0, &key, &obj);
         if (n < 0) {
             goto fail;
         }
 
-        key = JS_UNDEFINED;
         break;
 #endif
 
@@ -3053,6 +3053,7 @@ fail:
 
     JS_FreeValue(cx, key);
     JS_FreeValue(cx, keypub);
+    JS_FreeValue(cx, obj);
 
     return qjs_promise_result(cx, JS_EXCEPTION);
 }
@@ -5166,7 +5167,6 @@ qjs_webcrypto_key_algorithm(JSContext *cx, JSValueConst this_val)
                                       JS_PROP_C_W_E)
             < 0)
         {
-            JS_FreeValue(cx, pe);
             JS_FreeValue(cx, obj);
             return JS_EXCEPTION;
         }
@@ -5186,6 +5186,7 @@ qjs_webcrypto_key_algorithm(JSContext *cx, JSValueConst this_val)
         if (JS_DefinePropertyValueStr(cx, ret, "name", hash, JS_PROP_C_W_E)
             < 0)
         {
+            JS_FreeValue(cx, ret);
             JS_FreeValue(cx, obj);
             return JS_EXCEPTION;
         }
@@ -5786,7 +5787,6 @@ qjs_key_ops(JSContext *cx, unsigned mask)
 
             if (JS_SetPropertyUint32(cx, ops, i++, value) < 0) {
                 JS_FreeValue(cx, ops);
-                JS_FreeValue(cx, value);
                 return JS_EXCEPTION;
             }
         }

diff --git a/external/qjs_xml_module.c b/external/qjs_xml_module.c
index 36dfdba8027bb79652647836400e575e45123b4b..ebb87657ce96183a1eb0d08296bd465d11a0630a 100644 (file)
--- a/external/qjs_xml_module.c
+++ b/external/qjs_xml_module.c
@@ -637,7 +637,6 @@ qjs_xml_node_tags_handler(JSContext *cx, qjs_xml_node_t *current,
 
         if (JS_SetPropertyUint32(cx, arr, i++, ret) < 0) {
             JS_FreeValue(cx, arr);
-            JS_FreeValue(cx, ret);
             return JS_EXCEPTION;
         }
     }

diff --git a/nginx/ngx_http_js_module.c b/nginx/ngx_http_js_module.c
index 05ca87fe02dcbffac2c979cf6c9ba1b4e1f020c9..aadbcc5956bd64d5a7b923d2ed612c34653144d3 100644 (file)
--- a/nginx/ngx_http_js_module.c
+++ b/nginx/ngx_http_js_module.c
@@ -6231,8 +6231,11 @@ ngx_http_qjs_ext_args(JSContext *cx, JSValueConst this_val)
             return JS_EXCEPTION;
         }
 
+        arr = JS_UNDEFINED;
+
         if (JS_IsUndefined(prev)) {
             if (JS_SetProperty(cx, args, key, val) < 0) {
+                val = JS_UNDEFINED;
                 goto exception;
             }
 
@@ -6240,16 +6243,20 @@ ngx_http_qjs_ext_args(JSContext *cx, JSValueConst this_val)
             length = JS_GetPropertyStr(cx, prev, "length");
 
             if (JS_ToUint32(cx, &len, length)) {
+                JS_FreeValue(cx, length);
                 goto exception;
             }
 
             JS_FreeValue(cx, length);
 
             if (JS_SetPropertyUint32(cx, prev, len, val) < 0) {
+                val = JS_UNDEFINED;
                 goto exception;
             }
 
+            val = JS_UNDEFINED;
             JS_FreeValue(cx, prev);
+            prev = JS_UNDEFINED;
 
         } else {
 
@@ -6259,16 +6266,25 @@ ngx_http_qjs_ext_args(JSContext *cx, JSValueConst this_val)
             }
 
             if (JS_SetPropertyUint32(cx, arr, 0, prev) < 0) {
+                prev = JS_UNDEFINED;
                 goto exception;
             }
 
+            prev = JS_UNDEFINED;
+
             if (JS_SetPropertyUint32(cx, arr, 1, val) < 0) {
+                val = JS_UNDEFINED;
                 goto exception;
             }
 
+            val = JS_UNDEFINED;
+
             if (JS_SetProperty(cx, args, key, arr) < 0) {
+                arr = JS_UNDEFINED;
                 goto exception;
             }
+
+            arr = JS_UNDEFINED;
         }
 
         JS_FreeAtom(cx, key);
@@ -6286,6 +6302,8 @@ exception:
     JS_FreeAtom(cx, key);
     JS_FreeValue(cx, val);
     JS_FreeValue(cx, prev);
+    JS_FreeValue(cx, arr);
+    JS_FreeValue(cx, args);
 
     return JS_EXCEPTION;
 }
@@ -7016,7 +7034,6 @@ ngx_http_qjs_ext_request_form_get(JSContext *cx, JSValueConst this_val,
         if (JS_DefinePropertyValueUint32(cx, array, n++, value, JS_PROP_C_W_E)
             < 0)
         {
-            JS_FreeValue(cx, value);
             JS_FreeCString(cx, name);
             JS_FreeValue(cx, array);
             return JS_EXCEPTION;
@@ -7143,7 +7160,6 @@ ngx_http_qjs_request_form_entry_value(JSContext *cx,
     }
 
     if (JS_SetPropertyStr(cx, object, "name", value) < 0) {
-        JS_FreeValue(cx, value);
         JS_FreeValue(cx, object);
         return JS_EXCEPTION;
     }
@@ -7964,7 +7980,6 @@ ngx_http_qjs_ext_raw_headers(JSContext *cx, JSValueConst this_val, int out)
         if (JS_DefinePropertyValueUint32(cx, array, idx++, elem,
                                          JS_PROP_C_W_E) < 0)
         {
-            JS_FreeValue(cx, elem);
             JS_FreeValue(cx, array);
             return JS_EXCEPTION;
         }
@@ -7976,7 +7991,6 @@ ngx_http_qjs_ext_raw_headers(JSContext *cx, JSValueConst this_val, int out)
         }
 
         if (JS_DefinePropertyValueUint32(cx, elem, 0, key, JS_PROP_C_W_E) < 0) {
-            JS_FreeValue(cx, key);
             JS_FreeValue(cx, array);
             return JS_EXCEPTION;
         }
@@ -7988,7 +8002,6 @@ ngx_http_qjs_ext_raw_headers(JSContext *cx, JSValueConst this_val, int out)
         }
 
         if (JS_DefinePropertyValueUint32(cx, elem, 1, val, JS_PROP_C_W_E) < 0) {
-            JS_FreeValue(cx, val);
             JS_FreeValue(cx, array);
             return JS_EXCEPTION;
         }

diff --git a/nginx/ngx_js_shared_dict.c b/nginx/ngx_js_shared_dict.c
index c57ac90385d2d1bed006acb586d7054881c9b68a..7d002c4e5eee919dcd1d7e1e3e7fb85241de6e91 100644 (file)
--- a/nginx/ngx_js_shared_dict.c
+++ b/nginx/ngx_js_shared_dict.c
@@ -3600,7 +3600,6 @@ ngx_qjs_ext_shared_dict_items(JSContext *cx, JSValueConst this_val,
 
         if (JS_DefinePropertyValueUint32(cx, kv, 0, v, JS_PROP_C_W_E) < 0) {
             ngx_rwlock_unlock(&dict->sh->rwlock);
-            JS_FreeValue(cx, v);
             JS_FreeValue(cx, kv);
             JS_FreeValue(cx, arr);
             return JS_EXCEPTION;
@@ -3610,7 +3609,6 @@ ngx_qjs_ext_shared_dict_items(JSContext *cx, JSValueConst this_val,
 
         if (JS_DefinePropertyValueUint32(cx, kv, 1, v, JS_PROP_C_W_E) < 0) {
             ngx_rwlock_unlock(&dict->sh->rwlock);
-            JS_FreeValue(cx, v);
             JS_FreeValue(cx, kv);
             JS_FreeValue(cx, arr);
             return JS_EXCEPTION;
@@ -3618,7 +3616,6 @@ ngx_qjs_ext_shared_dict_items(JSContext *cx, JSValueConst this_val,
 
         if (JS_DefinePropertyValueUint32(cx, arr, i++, kv, JS_PROP_C_W_E) < 0) {
             ngx_rwlock_unlock(&dict->sh->rwlock);
-            JS_FreeValue(cx, kv);
             JS_FreeValue(cx, arr);
             return JS_EXCEPTION;
         }
@@ -3709,7 +3706,6 @@ ngx_qjs_ext_shared_dict_keys(JSContext *cx, JSValueConst this_val, int argc,
                                          JS_PROP_C_W_E) < 0)
         {
             ngx_rwlock_unlock(&dict->sh->rwlock);
-            JS_FreeValue(cx, key);
             JS_FreeValue(cx, arr);
             return JS_EXCEPTION;
         }
@@ -4433,7 +4429,6 @@ ngx_qjs_ngx_shared_dict_init(JSContext *cx, const char *name)
     if (JS_SetPropertyStr(cx, global_obj, "SharedMemoryError", shared_ctor)
         < 0)
     {
-        JS_FreeValue(cx, shared_ctor);
         JS_FreeValue(cx, global_obj);
         return NULL;
     }

diff --git a/nginx/ngx_qjs_fetch.c b/nginx/ngx_qjs_fetch.c
index 4aa8f54f119bca6487a54ec839345ee6b59c3111..1754f4ebc2898675d9d2e0894992b7e29998a506 100644 (file)
--- a/nginx/ngx_qjs_fetch.c
+++ b/nginx/ngx_qjs_fetch.c
@@ -1251,7 +1251,6 @@ ngx_qjs_fetch_error(ngx_js_http_t *http, const char *err)
     if (JS_SetPropertyStr(fetch->cx, fetch->response_value, "message",
                           reason) < 0)
     {
-        JS_FreeValue(fetch->cx, reason);
         goto done;
     }
 
@@ -1516,7 +1515,6 @@ ngx_qjs_headers_ext_keys(JSContext *cx, JSValue value)
             ret = JS_DefinePropertyValueUint32(cx, keys, n, item,
                                                JS_PROP_C_W_E);
             if (ret < 0) {
-                JS_FreeValue(cx, item);
                 goto fail;
             }
 
@@ -1609,7 +1607,6 @@ ngx_qjs_headers_get(JSContext *cx, JSValue this_val, ngx_str_t *name,
                                                JS_PROP_C_W_E);
             if (ret < 0) {
                 JS_FreeValue(cx, retval);
-                JS_FreeValue(cx, value);
                 return JS_EXCEPTION;
             }
 
@@ -2556,7 +2553,6 @@ ngx_qjs_fetch_init(JSContext *cx, const char *name)
         if (JS_SetPropertyStr(cx, global_obj, classes[i]->class_name, class)
             < 0)
         {
-            JS_FreeValue(cx, class);
             JS_FreeValue(cx, proto);
             JS_FreeValue(cx, global_obj);
             return NULL;

diff --git a/src/qjs.c b/src/qjs.c
index c6c05586f6050fa425c77becc3641e600306ee33..aa083e819e66ef7bf98d554488391d178ab02e5f 100644 (file)
--- a/src/qjs.c
+++ b/src/qjs.c
@@ -308,7 +308,6 @@ qjs_add_intrinsic_njs(JSContext *cx, JSValueConst global)
     }
 
     if (JS_SetPropertyStr(cx, global, "njs", obj) < 0) {
-        JS_FreeValue(cx, obj);
         return -1;
     }
 
@@ -452,16 +451,22 @@ qjs_process_env(JSContext *ctx, JSValueConst this_val)
         ret = JS_DefinePropertyValue(ctx, obj, atom, str, JS_PROP_C_W_E);
         JS_FreeAtom(ctx, atom);
         if (ret < 0) {
-error:
-            JS_FreeValue(ctx, name);
-            JS_FreeValue(ctx, str);
-            return JS_EXCEPTION;
+            str = JS_UNDEFINED;
+            goto error;
         }
 
         JS_FreeValue(ctx, name);
     }
 
     return obj;
+
+error:
+
+    JS_FreeValue(ctx, name);
+    JS_FreeValue(ctx, str);
+    JS_FreeValue(ctx, obj);
+
+    return JS_EXCEPTION;
 }
 
 
@@ -563,7 +568,6 @@ qjs_process_object(JSContext *ctx, int argc, const char **argv)
         }
 
         if (JS_DefinePropertyValueUint32(ctx, val, i, str, JS_PROP_C_W_E) < 0) {
-            JS_FreeValue(ctx, str);
             JS_FreeValue(ctx, val);
             return JS_EXCEPTION;
         }
@@ -579,7 +583,7 @@ qjs_process_object(JSContext *ctx, int argc, const char **argv)
                                njs_nitems(qjs_process_proto));
 
     if (JS_SetPropertyStr(ctx, obj, "argv", val) < 0) {
-        JS_FreeValue(ctx, val);
+        JS_FreeValue(ctx, obj);
         return JS_EXCEPTION;
     }