Segfault when evaluating modules that throws an error

[richarddd]

When trying to either define or evaluate a model that throws an error, quickjs segfauls.

I see in the docs that holding on to a unevaluated module is unsupported because quickjs will free it on craches. But i'm not doing that. Check this reproducable:

main.rs

globals.set(
    "importSync",
    Func::from(|ctx: Ctx, value: String| {
        let source = fs::read_to_string(value.clone()).unwrap();
        let _ = Module::evaluate(ctx, value, source)?;
        Ok::<_, Error>(())
    }),
)?;

index.js

importSync("hello.js");

hello.js

throw new Error("error");

It doesn't matter if i throw or if its a type error. Only errors that are handled correctly are parsing errors.

Callstack

frame #0: 0x0000000100769ce7 llrt`list_del(el=0x000004f560166e10) at list.h:75:16 thread backtrace * thread #1, name = 'main', queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8) * frame #0: 0x0000000100769ce7 llrt`list_del(el=0x000004f560166e10) at list.h:75:16 frame #1: 0x000000010077064c llrt`__JS_FreeValueRT(rt=0x000004f5601d0308, v=JSValue @ 0x00007ff7bfef8ca0) at quickjs.c:5513:17 frame #2: 0x0000000100770a99 llrt`__JS_FreeValue(ctx=0x000004f5601b0608, v=JSValue @ 0x00007ff7bfef8cd0) at quickjs.c:5555:5 frame #3: 0x0000000100769d5e llrt`JS_FreeValue(ctx=0x000004f5601b0608, v=JSValue @ 0x00007ff7bfef8d00) at quickjs.h:648:13 frame #4: 0x00000001007723e2 llrt`JS_CallFree(ctx=0x000004f5601b0608, func_obj=JSValue @ 0x00007ff7bfef8d70, this_obj=JSValue @ 0x00007ff7bfef8d60, argc=0, argv=0x0000000000000000) at quickjs.c:18732:5 frame #5: 0x00000001007b8daa llrt`js_evaluate_module(ctx=0x000004f5601b0608, m=0x000004f560270b48) at quickjs.c:28375:19 frame #6: 0x000000010078ea72 llrt`JS_EvalFunctionInternal(ctx=0x000004f5601b0608, fun_obj=JSValue @ 0x00007ff7bfef8f20, this_obj=JSValue @ 0x00007ff7bfef8f10, var_refs=0x0000000000000000, sf=0x0000000000000000) at quickjs.c:33574:19 frame #7: 0x000000010078e942 llrt`JS_EvalFunction(ctx=0x000004f5601b0608, fun_obj=JSValue @ 0x00007ff7bfef8f60) at quickjs.c:33589:12 frame #8: 0x0000000100731c4d llrt`rquickjs_core::value::module::Module::eval(self=0x00007ff7bfef9200) at module.rs:743:23 frame #9: 0x000000010013b416 llrt`llrt::vm::Vm::load_module_from_source(ctx=rquickjs_core::context::ctx::Ctx @ 0x00007ff7bfef91f8, name="index.mjs", source=") at vm.rs:409:24 frame #10: 0x000000010013bf2e llrt`llrt::vm::Vm::load_module(ctx=0x00007ff7bfef9de0, filename="index.mjs") at vm.rs:435:9 frame #11: 0x000000010005b5f3 llrt`llrt::vm::Vm::run_module::{{closure}}::{{closure}}(ctx=rquickjs_core::context::ctx::Ctx @ 0x00007ff7bfef9de0) at vm.rs:440:24 frame #12: 0x000000010005c1c8 llrt`llrt::vm::Vm::run_and_handle_exceptions::{{closure}}::{{closure}}(ctx=rquickjs_core::context::ctx::Ctx @ 0x00007ff7bfef9f60) at vm.rs:451:13 frame #13: 0x00000001002423aa llrt`rquickjs_core::context::async::AsyncContext::with::{{closure}}((null)=0x00007ff7bfefcb30) at async.rs:250:19 frame #14: 0x000000010005b925 llrt`llrt::vm::Vm::run_and_handle_exceptions::{{closure}}((null)=0x00007ff7bfefcb30) at vm.rs:455:10 frame #15: 0x000000010005b4c5 llrt`llrt::vm::Vm::run_module::{{closure}}((null)=0x00007ff7bfefcb30) at vm.rs:443:10 frame #16: 0x00000001000d6348 llrt`llrt::start_cli::{{closure}}((null)=0x00007ff7bfefcb30) at main.rs:123:44 frame #17: 0x00000001000db5b7 llrt`llrt::main::{{closure}}((null)=0x00007ff7bfefcb30) at main.rs:70:28 frame #18: 0x000000010030224e llrt`tokio::runtime::park::CachedParkThread::block_on::{{closure}} at park.rs:282:63 frame #19: 0x00000001003020ed llrt`tokio::runtime::park::CachedParkThread::block_on at coop.rs:107:5 frame #20: 0x000000010030207c llrt`tokio::runtime::park::CachedParkThread::block_on [inlined] tokio::runtime::coop::budget(f=tokio::runtime::park::{impl#4}::block_on::{closure_env#0} @ 0x00007ff7bfefcf50) at coop.rs:73:5 frame #21: 0x0000000100301ff8 llrt`tokio::runtime::park::CachedParkThread::block_on(self=0x00007ff7bfefcfe8, f=llrt::main::{async_block_env#0}::Unresumed @ 0x00007ff7bfefcff0) at park.rs:282:31 frame #22: 0x0000000100156db6 llrt`tokio::runtime::context::blocking::BlockingRegionGuard::block_on(self=0x00007ff7bfefd808, f=llrt::main::{async_block_env#0}::Unresumed @ 0x00007ff7bfefd3e8) at blocking.rs:66:9 frame #23: 0x000000010037d2e5 llrt`tokio::runtime::scheduler::multi_thread::MultiThread::block_on::{{closure}}(blocking=0x00007ff7bfefd808) at mod.rs:87:13 frame #24: 0x0000000100171614 llrt`tokio::runtime::context::runtime::enter_runtime(handle=0x00007ff7bfefebd0, allow_block_in_place=true, f=tokio::runtime::scheduler::multi_thread::{impl#0}::block_on::{closure_env#0} @ 0x00007ff7bfefdc40) at runtime.rs:65:16 frame #25: 0x000000010037d282 llrt`tokio::runtime::scheduler::multi_thread::MultiThread::block_on(self=0x00007ff7bfefeba8, handle=0x00007ff7bfefebd0, future=) at mod.rs:86:9 frame #26: 0x0000000100135276 llrt`tokio::runtime::runtime::Runtime::block_on(self=0x00007ff7bfefeba0, future=llrt::main::{async_block_env#0}::Unresumed @ 0x00007ff7bfefed08) at runtime.rs:349:45 frame #27: 0x0000000100243520 llrt`llrt::main at main.rs:74:5 frame #28: 0x000000010029d1de llrt`core::ops::function::FnOnce::call_once((null)=0x0000000100243430, (null)=()) at function.rs:250:5 frame #29: 0x00000001002934c1 llrt`std::sys_common::backtrace::__rust_begin_short_backtrace(f=0x0000000100243430) at backtrace.rs:135:18 frame #30: 0x0000000100288084 llrt`std::rt::lang_start::{{closure}} at rt.rs:166:18 frame #31: 0x00000001008457fa llrt`std::rt::lang_start_internal [inlined] core::ops::function::impls:: for &F>::call_once at function.rs:284:13 [opt] frame #32: 0x00000001008457ed llrt`std::rt::lang_start_internal [inlined] std::panicking::try::do_call at panicking.rs:500:40 [opt] frame #33: 0x00000001008457ed llrt`std::rt::lang_start_internal [inlined] std::panicking::try at panicking.rs:464:19 [opt] frame #34: 0x00000001008457ed llrt`std::rt::lang_start_internal [inlined] std::panic::catch_unwind at panic.rs:142:14 [opt] frame #35: 0x00000001008457ed llrt`std::rt::lang_start_internal [inlined] std::rt::lang_start_internal::{{closure}} at rt.rs:148:48 [opt] frame #36: 0x00000001008457ed llrt`std::rt::lang_start_internal [inlined] std::panicking::try::do_call at panicking.rs:500:40 [opt] frame #37: 0x00000001008457ed llrt`std::rt::lang_start_internal [inlined] std::panicking::try at panicking.rs:464:19 [opt] frame #38: 0x00000001008457ed llrt`std::rt::lang_start_internal [inlined] std::panic::catch_unwind at panic.rs:142:14 [opt] frame #39: 0x00000001008457ed llrt`std::rt::lang_start_internal at rt.rs:148:20 [opt] frame #40: 0x0000000100288057 llrt`std::rt::lang_start(main=0x0000000100243430, argc=1, argv=0x00007ff7bfeff398, sigpipe=0) at rt.rs:165:17 frame #41: 0x00000001002435d8 llrt`main + 24 frame #42: 0x000000010178152e dyld`start + 462

[richarddd]

I think i found the issue and it might be a complicated one to solve. I'm trying to implement sync imports (similar to require)

1. When the "root" module is being evaluated it runs the code that calls importSync.
2. importSync then tries to evaluate another module that craches on execution.
3. When that happens, qjs calls js_free_modules that will free all non-evaluated modules and eventually will try to free the root module (since that's still being evaluated) that is till in use. This causes some undefined behaviour and bad memory access.

I don't know if this is even a bug in quickjs (that we shouldn't try to free the module currently under evaluation)

There is no workaround for now. The simplest thing would be to patch quickjs to expose js_get_module_ns. That way we could run JS_RunModule and access the exports by js_get_module_ns.

Whats your take @DelSkayn? :)

[richarddd]

Opened up https://github.com/DelSkayn/rquickjs/pull/199/files that will solve the issue. It takes a different approach and patches quickjs to allow for js_dynamic_import_job both async and sync.

[DelSkayn]

Hmm, I didn't think of evaluating modules while evaluating another module when designing the Module API. This is definitely a problem.

Working around quickjs freeing all modules which aren't properly evaluated is difficult. As the library is now Module::evaluate should be unsafe.

I think you are onto something using the builtin dynamic import functionality. Normally when a module is imported quickjs takes care of resolving and evaluating all modules which are imported as a result of the original import in a single function. This function assumes it knows all the modules which will be imported before it starts evaluating modules, this assumption allows the function to free all unevaluated modules. Dynamically call Module::evaluate breaks this assumption and causes modules to be free to early.

But quickjs must support dynamic imports and looking at the code it solves this by making dynamic imports which error only free all unresolved modules.

If we patch quickjs such that we can evaluated modules like a dynamic import does it would fix this issue and we can make holding onto unevaluated modules safe again at the cost of some modules not being freed when an error happens, which I think is an acceptable tradeoff.

[DelSkayn]

The changes to the library will probably just be changing freeing modules in import code from freeing all unevaluated modules to all unresolved modules and adding a unsafe function to Ctx for freeing unevaluated modules.

We could also add a safe version of that function to Context since calling it requires being outside a Ctx::with closure and locking the runtime so it is impossible to call while holding onto modules which are unevaluated.

[richarddd]

I've opened up a PR for the second approach which basically uses the same code that await import("file.js") calls through its QueueJob but calling it directly: https://github.com/DelSkayn/rquickjs/pull/199

[DelSkayn]

I want to keep this issue open for now since Module::evaluate is currently a safe function which can segfault when called wrong.

That must first be resolved before this issue can be closed.