From d9c97cf9a13e7a26206345b774fcfe609fbe4ef3 Mon Sep 17 00:00:00 2001 From: Dmitry Volyntsev Date: Tue, 2 Jun 2026 22:11:58 -0700 Subject: [PATCH] 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. --- external/qjs_fs_module.c | 7 ++----- external/qjs_query_string_module.c | 6 +++++- external/qjs_webcrypto_module.c | 30 +++++++++++++++--------------- external/qjs_xml_module.c | 1 - nginx/ngx_http_js_module.c | 23 ++++++++++++++++++----- nginx/ngx_js_shared_dict.c | 5 ----- nginx/ngx_qjs_fetch.c | 4 ---- src/qjs.c | 18 +++++++++++------- 8 files changed, 51 insertions(+), 43 deletions(-) diff --git a/external/qjs_fs_module.c b/external/qjs_fs_module.c index b9f8a360..49c2e65e 100644 --- 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 8f695677..128a17cd 100644 --- 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 965918be..42ca7975 100644 --- 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 36dfdba8..ebb87657 100644 --- 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 05ca87fe..aadbcc59 100644 --- 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 c57ac903..7d002c4e 100644 --- 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 4aa8f54f..1754f4eb 100644 --- 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 c6c05586..aa083e81 100644 --- 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; } -- 2.47.3