Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Weird memory corruption (?) bug #577

Closed
bnoordhuis opened this issue Oct 6, 2024 · 2 comments · Fixed by #578
Closed

Weird memory corruption (?) bug #577

bnoordhuis opened this issue Oct 6, 2024 · 2 comments · Fixed by #578
Labels
bug Something isn't working

Comments

@bnoordhuis
Copy link
Contributor

When I patch qjs to run multiple files (importantly, each in its own runtime/context):

bnoordhuis@zoidberg:~/src/quickjs$ d @
diff --git a/qjs.c b/qjs.c
index 5631543..fb321ef 100644
--- a/qjs.c
+++ b/qjs.c
@@ -481,6 +481,7 @@ int main(int argc, char **argv)
         }
     }
 
+nextfile:
     if (trace_memory) {
         js_trace_malloc_init(&trace_data);
         rt = JS_NewRuntime2(&trace_mf, &trace_data);
@@ -566,6 +567,9 @@ int main(int argc, char **argv)
     JS_FreeContext(ctx);
     JS_FreeRuntime(rt);
 
+    if (++optind < argc)
+        goto nextfile;
+
     if (empty_run && dump_memory) {
         clock_t t[5];
         double best[5] = {0};

I get this weird error:

$ build/debug/qjs tests/test_builtin.js tests/test_std.js
TypeError: not a function
    at test_file1 (tests/test_std.js:46:5)
    at <anonymous> (tests/test_std.js:307:1)

The offending lines in test_std.js are these:

f = std.tmpfile();
str = "hello world\n";
f.puts(str);  // <- not a function

When I print(Object.getOwnPropertyNames(f.__proto__)), I get this:

postMessage,onmessage,constructor

You guessed it, f.__proto__.constructor.name is "Worker"!

Not sure what's going on yet but it's definitely something fishy.

@bnoordhuis bnoordhuis added the bug Something isn't working label Oct 6, 2024
@bnoordhuis
Copy link
Contributor Author

I think I figured it out. js_std_init() and js_os_init() call JS_NewClassID() and that function only does something if the in/out param is zero. It is the first time, but not the second time!

@bnoordhuis
Copy link
Contributor Author

Making js_std_file_class_id and js_worker_class_id members of JSThreadState doesn't work because js_std_file_finalizer and js_worker_finalizer run from JS_FreeRuntime, after js_std_free_handlers which frees JSThreadState.

I suppose the solution is to switch to thread-local storage. Having js_std_file_class_id and js_worker_class_id as static globals is not thread-safe; I can tell by adding js_init_module_os calls to run-test262 and enabling TSan.

bnoordhuis added a commit to bnoordhuis/quickjs that referenced this issue Oct 6, 2024
`JS_NewClassID(rt, &class_id)` where `class_id` is a global variable
is unsafe when called from multiple threads but that is exactly what
quickjs-libc.c did.

Change the class ids in quickjs-libc.c to thread-locals, and change
the function prototype of JS_NewClassID to hopefully make it harder
for downstream users to introduce such bugs.

The gcc48 and tcc buildbots don't support _Thread_local and that is
why this commit removes them.

I toyed with the idea of porting the uv_key_create/uv_key_get/etc.
functions from libuv but because quickjs-libc doesn't know when it's
safe to free the thread-local keys, that leads to memory leaks, and
support for ancient or niche compilers isn't worth that.

Fixes: quickjs-ng#577
bnoordhuis added a commit to bnoordhuis/quickjs that referenced this issue Oct 7, 2024
`JS_NewClassID(rt, &class_id)` where `class_id` is a global variable
is unsafe when called from multiple threads but that is exactly what
quickjs-libc.c did.

Add a new JS_AddRuntimeFinalizer function that lets quickjs-libc
store the class ids in JSRuntimeState and defer freeing the memory
until the runtime is destroyed. Necessary because object finalizers
such as js_std_file_finalizer need to know the class id.

js_std_free_handlers is now a no-op but is left in (for now) for
compatibility with bellard/quickjs.

Fixes: quickjs-ng#577
bnoordhuis added a commit to bnoordhuis/quickjs that referenced this issue Oct 7, 2024
`JS_NewClassID(rt, &class_id)` where `class_id` is a global variable
is unsafe when called from multiple threads but that is exactly what
quickjs-libc.c did.

Add a new JS_AddRuntimeFinalizer function that lets quickjs-libc
store the class ids in JSRuntimeState and defer freeing the memory
until the runtime is destroyed. Necessary because object finalizers
such as js_std_file_finalizer need to know the class id and run after
js_std_free_handlers runs.

Fixes: quickjs-ng#577
bluesky950520 pushed a commit to bluesky950520/quickjs that referenced this issue Mar 14, 2025
`JS_NewClassID(rt, &class_id)` where `class_id` is a global variable
is unsafe when called from multiple threads but that is exactly what
quickjs-libc.c did.

Add a new JS_AddRuntimeFinalizer function that lets quickjs-libc
store the class ids in JSRuntimeState and defer freeing the memory
until the runtime is destroyed. Necessary because object finalizers
such as js_std_file_finalizer need to know the class id and run after
js_std_free_handlers runs.

Fixes: quickjs-ng/quickjs#577
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant