commit f6b5ac20dca2e40c4eb210fb0a4548e04bf69329
parent 1584d0612867b2fc0770197b036ef41090121095
Author: Florian Dold <florian@dold.me>
Date: Sat, 8 Jul 2023 00:56:08 +0200
quickjs: fix garbage collection / var ref bug
When a closure escapes from an async function, the variables
it references via a var ref can be falsed collected as garbage
by the cycle collector.
To fix this, suspending functions now close their var refs
whenever control leaves the function, not just on the final exit.
Diffstat:
5 files changed, 220 insertions(+), 1 deletion(-)
diff --git a/quickjs/quickjs.c b/quickjs/quickjs.c
@@ -338,6 +338,10 @@ typedef struct JSStackFrame {
/* only used in generators. Current stack pointer value. NULL if
the function is running. */
JSValue *cur_sp;
+ /* only used in async/generator functions to save the variable references
+ that have been evicted to the heap. */
+ struct JSVarRef **closed_var_refs;
+ int closed_var_refs_count;
} JSStackFrame;
typedef enum {
@@ -5485,7 +5489,8 @@ void __JS_FreeValueRT(JSRuntime *rt, JSValue v)
if (tag == JS_TAG_OBJECT) {
JS_DumpObject(rt, JS_VALUE_GET_OBJ(v));
} else {
- JS_DumpValueShort(rt, v);
+ printf("[<short value>]");
+ //JS_DumpValueShort(rt, v);
printf("\n");
}
}
@@ -15969,6 +15974,94 @@ static int js_op_define_class(JSContext *ctx, JSValue *sp,
return -1;
}
+/* Re-attach variable refs to the local,
+ * on-stack variables when resuming an async/generator function. */
+static void reopen_var_refs(JSRuntime *rt, JSStackFrame *sf)
+{
+ int i;
+ JSVarRef *var_ref;
+ int var_idx;
+
+ for (i = 0; i < sf->closed_var_refs_count; i++) {
+ var_ref = sf->closed_var_refs[i];
+ sf->closed_var_refs[i] = NULL;
+ if (!var_ref) {
+ continue;
+ }
+ var_idx = var_ref->var_idx;
+ assert(var_ref->is_detached);
+
+ var_ref->is_detached = FALSE;
+
+ if (var_ref->is_arg) {
+ JS_FreeValueRT(rt, sf->arg_buf[var_idx]);
+ sf->arg_buf[var_idx] = var_ref->value;
+ var_ref->pvalue = &sf->arg_buf[var_idx];
+ } else {
+ JS_FreeValueRT(rt, sf->var_buf[var_idx]);
+ sf->var_buf[var_idx] = var_ref->value;
+ var_ref->pvalue = &sf->var_buf[var_idx];
+ }
+ remove_gc_object(&var_ref->header);
+ list_add_tail(&var_ref->header.link, &sf->var_ref_list);
+ free_var_ref(rt, var_ref);
+ }
+}
+
+static void close_var_refs_saving(JSRuntime *rt, JSStackFrame *sf)
+{
+ struct list_head *el, *el1;
+ JSVarRef **var_refs;
+ JSVarRef *var_ref;
+ int var_idx;
+ int count = 0;
+ int i;
+
+ if (list_empty(&sf->var_ref_list)) {
+ return;
+ }
+
+ list_for_each_safe(el, el1, &sf->var_ref_list) {
+ count++;
+ }
+ var_refs = sf->closed_var_refs;
+ if (count > sf->closed_var_refs_count) {
+ var_refs = js_realloc_rt(rt, var_refs, count * sizeof(var_refs[0]));
+ if (!var_refs) {
+ // FIXME
+ abort();
+ }
+ for (i = sf->closed_var_refs_count; i < count; i++) {
+ var_refs[i] = NULL;
+ }
+ sf->closed_var_refs = var_refs;
+ sf->closed_var_refs_count = count;
+ }
+
+ i = 0;
+ list_for_each_safe(el, el1, &sf->var_ref_list) {
+ var_ref = list_entry(el, JSVarRef, header.link);
+ var_idx = var_ref->var_idx;
+ assert(!var_ref->is_detached);
+ /* move ownership from stack into varref */
+ if (var_ref->is_arg)
+ var_ref->value = JS_DupValueRT(rt, sf->arg_buf[var_idx]);
+ else
+ var_ref->value = JS_DupValueRT(rt, sf->var_buf[var_idx]);
+ var_ref->pvalue = &var_ref->value;
+ /* the reference is no longer to a local variable */
+ var_ref->is_detached = TRUE;
+ /* delete from stack frame's var_ref_list */
+ list_del(&var_ref->header.link);
+ /* prevent from collecting while async/generator is still active */
+ var_refs[i] = var_ref;
+ var_ref->header.ref_count++;
+ add_gc_object(rt, &var_ref->header, JS_GC_OBJ_TYPE_VAR_REF);
+ i++;
+ }
+ assert(list_empty(&sf->var_ref_list));
+}
+
static void close_var_refs(JSRuntime *rt, JSStackFrame *sf)
{
struct list_head *el, *el1;
@@ -15978,6 +16071,7 @@ static void close_var_refs(JSRuntime *rt, JSStackFrame *sf)
list_for_each_safe(el, el1, &sf->var_ref_list) {
var_ref = list_entry(el, JSVarRef, header.link);
var_idx = var_ref->var_idx;
+ assert(!var_ref->is_detached);
if (var_ref->is_arg)
var_ref->value = JS_DupValueRT(rt, sf->arg_buf[var_idx]);
else
@@ -15985,6 +16079,8 @@ static void close_var_refs(JSRuntime *rt, JSStackFrame *sf)
var_ref->pvalue = &var_ref->value;
/* the reference is no longer to a local variable */
var_ref->is_detached = TRUE;
+ /* delete from stack frame's var_ref_list */
+ list_del(&var_ref->header.link);
add_gc_object(rt, &var_ref->header, JS_GC_OBJ_TYPE_VAR_REF);
}
}
@@ -16256,6 +16352,7 @@ static JSValue __JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
pc = sf->cur_pc;
sf->prev_frame = rt->current_stack_frame;
rt->current_stack_frame = sf;
+ reopen_var_refs(rt, sf);
if (s->throw_flag)
goto exception;
else
@@ -16288,6 +16385,9 @@ static JSValue __JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
if (js_check_stack_overflow(rt, alloca_size))
return JS_ThrowStackOverflow(caller_ctx);
+ sf->closed_var_refs = NULL;
+ sf->closed_var_refs_count = 0;
+
sf->js_mode = b->js_mode;
arg_buf = argv;
sf->arg_count = argc;
@@ -18696,6 +18796,7 @@ static JSValue __JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
generator function. */
if (b->func_kind != JS_FUNC_NORMAL) {
done_generator:
+ close_var_refs_saving(rt, sf);
sf->cur_pc = pc;
sf->cur_sp = sp;
} else {
@@ -19112,6 +19213,15 @@ static void async_func_mark(JSRuntime *rt, JSAsyncFunctionState *s,
for(sp = sf->arg_buf; sp < sf->cur_sp; sp++)
JS_MarkValue(rt, *sp, mark_func);
}
+ if (sf->closed_var_refs)
+ {
+ int i;
+ for (i = 0; i < sf->closed_var_refs_count; i++) {
+ if (sf->closed_var_refs[i]) {
+ mark_func(rt, &sf->closed_var_refs[i]->header);
+ }
+ }
+ }
}
static void async_func_free(JSRuntime *rt, JSAsyncFunctionState *s)
@@ -19132,6 +19242,18 @@ static void async_func_free(JSRuntime *rt, JSAsyncFunctionState *s)
}
js_free_rt(rt, sf->arg_buf);
}
+ if (sf->closed_var_refs)
+ {
+ int i;
+ for (i = 0; i < sf->closed_var_refs_count; i++) {
+ if (sf->closed_var_refs[i]) {
+ free_var_ref(rt, sf->closed_var_refs[i]);
+ }
+ }
+ js_free_rt(rt, sf->closed_var_refs);
+ sf->closed_var_refs = NULL;
+ sf->closed_var_refs_count = 0;
+ }
JS_FreeValueRT(rt, sf->cur_func);
JS_FreeValueRT(rt, s->this_val);
}
diff --git a/quickjs/tests/crash1.js b/quickjs/tests/crash1.js
@@ -0,0 +1,16 @@
+import * as std from "std";
+
+async function waitUntilDone(ws) {
+ let p = {};
+ globalThis.escFun = function escFun() { return p; };
+ p.promise = new Promise(function promIni() {});
+ await p.promise;
+}
+
+waitUntilDone();
+
+//console.log("-------- running gc1");
+//std.gc();
+//console.log("-------- running gc2");
+//std.gc();
+//console.log("-------- end");
diff --git a/quickjs/tests/crash2.js b/quickjs/tests/crash2.js
@@ -0,0 +1,19 @@
+import * as std from "std";
+
+let listener;
+
+async function waitUntilDone(ws) {
+ let p2 = {};
+ listener = (yn) => {
+ return p2;
+ };
+ p2.promise = new Promise(() => {});
+ await p2.promise;
+}
+
+waitUntilDone();
+
+console.log("---- calling gc");
+std.gc();
+console.log("---- calling listener");
+listener();
diff --git a/quickjs/tests/gentest.js b/quickjs/tests/gentest.js
@@ -0,0 +1,18 @@
+function *foo() {
+ let x = 42;
+ console.log("first yield");
+ yield () => x;
+ console.log("second yield");
+ yield v => { x = v; };
+ console.log("my x", x);
+}
+
+const g = foo();
+
+const fun1 = g.next().value;
+const fun2 = g.next().value;
+
+console.log(fun1());
+fun2(41);
+console.log(fun1());
+g.next();
diff --git a/quickjs/tests/promtest3.js b/quickjs/tests/promtest3.js
@@ -0,0 +1,44 @@
+// Running this file crashes vanilla quickjs-2021-03-27
+// due to a use-after-free bug.
+// That happens because innerFun keeps a reference
+// to the local variable p, but p gets erroneously
+// garbage-collected.
+// The outerFun infinitely waits for the promise
+// to finish. Thus, its local variables are kept on the
+// stack. Thus innerFun doesn't directly reference
+// outerFun, and outerFun gets collected
+// by the cycle collector.
+
+import * as os from "os";
+import * as std from "std";
+
+let f;
+
+let promise = new Promise(() => {});
+
+let outerFun = async function outerFun() {
+ let p = { foo: "bar" };
+ function innerFun() {
+ console.log(p.foo);
+ }
+ f = innerFun;
+ // Wait indefinitely
+ await promise;
+}
+outerFun();
+
+// directly
+
+//console.log("cleaning up");
+//console.log("deleting promise");
+//promise = undefined;
+//console.log("deleting outerFun");
+//outerFun = undefined;
+//console.log("deleting f");
+//f = undefined;
+//console.log("exiting");
+//std.exit(42);
+
+
+// via timer
+os.setTimeout(() => { console.log("calling GC"); std.gc(); f(); }, 0);
