From a27705ae9784ef0c099f122022f66bc093fdaf82 Mon Sep 17 00:00:00 2001 From: Mees Delzenne Date: Fri, 18 Aug 2023 15:05:39 +0200 Subject: [PATCH 1/2] Don't free resolved modules and add tests --- core/src/context/ctx.rs | 13 +++++++ core/src/value/module.rs | 37 ++++++++++++++++++++ sys/patches/dynamic_import_sync.patch | 36 +++++++++++++++++-- sys/src/bindings/x86_64-unknown-linux-gnu.rs | 3 ++ 4 files changed, 86 insertions(+), 3 deletions(-) diff --git a/core/src/context/ctx.rs b/core/src/context/ctx.rs index aebc9ae8..524be8cd 100644 --- a/core/src/context/ctx.rs +++ b/core/src/context/ctx.rs @@ -407,6 +407,19 @@ impl<'js> Ctx<'js> { pub fn as_raw(&self) -> NonNull { self.ctx } + + /// Frees modules which aren't eveluated. + /// + /// When a module is compiled and the compilation results in an error the module can already + /// have resolved several modules. Originally quickjs freed all these module when compiling + /// (but not when a it was dynamically imported), this library patched that behaviour out + /// because it proved to be hard to make safe. This function will free those modules. + /// + /// # Safety + /// Caller must ensure that this method is not called from a module being evaluated. + pub unsafe fn free_unevaluated_modules(&self) { + qjs::JS_FreeUnevaluatedModules(self.ctx.as_ptr()) + } } #[cfg(test)] diff --git a/core/src/value/module.rs b/core/src/value/module.rs index 6eb04206..cfe14c36 100644 --- a/core/src/value/module.rs +++ b/core/src/value/module.rs @@ -906,6 +906,7 @@ where #[cfg(test)] mod test { + use super::*; use crate::*; @@ -999,6 +1000,42 @@ mod test { }); } + #[test] + fn holding_onto_unevaluated() { + let runtime = Runtime::new().unwrap(); + let ctx = Context::full(&runtime).unwrap(); + ctx.with(|ctx| { + let module = unsafe { + Module::unsafe_declare( + ctx.clone(), + "test", + "export function add(a,b){ return a + b }", + ) + .unwrap() + }; + // Error + ctx.compile("test2", "throw new Error(1)").ok(); + + unsafe { module.eval().unwrap() } + }); + } + + #[test] + fn eval_crashing_module_inside_module() { + let runtime = Runtime::new().unwrap(); + let ctx = Context::full(&runtime).unwrap(); + + ctx.with(|ctx| { + let globals = ctx.globals(); + let eval_crashing = |ctx: Ctx| ctx.compile("test2", "throw new Error(1)").map(|_| ()); + let function = Function::new(ctx.clone(), eval_crashing).unwrap(); + globals.set("eval_crashing", function).unwrap(); + + let res = ctx.compile("test", " eval_crashing(); "); + assert!(res.is_err()) + }); + } + #[test] fn from_javascript() { test_with(|ctx| { diff --git a/sys/patches/dynamic_import_sync.patch b/sys/patches/dynamic_import_sync.patch index c02e80d1..13c67524 100644 --- a/sys/patches/dynamic_import_sync.patch +++ b/sys/patches/dynamic_import_sync.patch @@ -1,5 +1,5 @@ diff --git a/quickjs.c b/quickjs.c -index 48aeffc..9e8c97f 100644 +index 48aeffc..6862b8c 100644 --- a/quickjs.c +++ b/quickjs.c @@ -1192,6 +1192,8 @@ static void js_free_module_def(JSContext *ctx, JSModuleDef *m); @@ -136,11 +136,41 @@ index 48aeffc..9e8c97f 100644 static __exception JSAtom js_parse_from_clause(JSParseState *s) { JSAtom module_name; +@@ -33532,7 +33584,7 @@ static JSValue JS_EvalFunctionInternal(JSContext *ctx, JSValue fun_obj, + ret_val = js_evaluate_module(ctx, m); + if (JS_IsException(ret_val)) { + fail: +- js_free_modules(ctx, JS_FREE_MODULE_NOT_EVALUATED); ++ js_free_modules(ctx, JS_FREE_MODULE_NOT_RESOLVED); + return JS_EXCEPTION; + } + } else { +@@ -33542,6 +33594,10 @@ static JSValue JS_EvalFunctionInternal(JSContext *ctx, JSValue fun_obj, + return ret_val; + } + ++void JS_FreeUnevaluatedModules(JSContext *ctx){ ++ js_free_modules(ctx, JS_FREE_MODULE_NOT_EVALUATED); ++} ++ + JSValue JS_EvalFunction(JSContext *ctx, JSValue fun_obj) + { + return JS_EvalFunctionInternal(ctx, fun_obj, ctx->global_obj, NULL, NULL); diff --git a/quickjs.h b/quickjs.h -index d4a5cd3..991a663 100644 +index d4a5cd3..0ad5b30 100644 --- a/quickjs.h +++ b/quickjs.h -@@ -1039,6 +1039,8 @@ int JS_SetModuleExport(JSContext *ctx, JSModuleDef *m, const char *export_name, +@@ -866,6 +866,9 @@ void JS_SetModuleLoaderFunc(JSRuntime *rt, + JSValue JS_GetImportMeta(JSContext *ctx, JSModuleDef *m); + JSAtom JS_GetModuleName(JSContext *ctx, JSModuleDef *m); + ++void JS_FreeUnevaluatedModules(JSContext *ctx); ++ ++ + /* JS Job support */ + + typedef JSValue JSJobFunc(JSContext *ctx, int argc, JSValueConst *argv); +@@ -1039,6 +1042,8 @@ int JS_SetModuleExport(JSContext *ctx, JSModuleDef *m, const char *export_name, int JS_SetModuleExportList(JSContext *ctx, JSModuleDef *m, const JSCFunctionListEntry *tab, int len); diff --git a/sys/src/bindings/x86_64-unknown-linux-gnu.rs b/sys/src/bindings/x86_64-unknown-linux-gnu.rs index c4ead954..cd86dd45 100644 --- a/sys/src/bindings/x86_64-unknown-linux-gnu.rs +++ b/sys/src/bindings/x86_64-unknown-linux-gnu.rs @@ -1881,6 +1881,9 @@ extern "C" { extern "C" { pub fn JS_GetModuleName(ctx: *mut JSContext, m: *mut JSModuleDef) -> JSAtom; } +extern "C" { + pub fn JS_FreeUnevaluatedModules(ctx: *mut JSContext); +} pub type JSJobFunc = ::std::option::Option< unsafe extern "C" fn( ctx: *mut JSContext, From 49c928b2246431ff17a0d1002813e98fdd751c6f Mon Sep 17 00:00:00 2001 From: Mees Delzenne Date: Fri, 18 Aug 2023 15:24:34 +0200 Subject: [PATCH 2/2] Update bindings --- sys/src/bindings/aarch64-apple-darwin.rs | 3 +++ sys/src/bindings/aarch64-unknown-linux-musl.rs | 3 +++ sys/src/bindings/i686-pc-windows-gnu.rs | 3 +++ sys/src/bindings/i686-pc-windows-msvc.rs | 3 +++ sys/src/bindings/i686-unknown-linux-gnu.rs | 3 +++ sys/src/bindings/x86_64-apple-darwin.rs | 3 +++ sys/src/bindings/x86_64-pc-windows-gnu.rs | 3 +++ sys/src/bindings/x86_64-pc-windows-msvc.rs | 3 +++ sys/src/bindings/x86_64-unknown-linux-musl.rs | 3 +++ 9 files changed, 27 insertions(+) diff --git a/sys/src/bindings/aarch64-apple-darwin.rs b/sys/src/bindings/aarch64-apple-darwin.rs index d2b5c741..239045d7 100644 --- a/sys/src/bindings/aarch64-apple-darwin.rs +++ b/sys/src/bindings/aarch64-apple-darwin.rs @@ -1882,6 +1882,9 @@ extern "C" { extern "C" { pub fn JS_GetModuleName(ctx: *mut JSContext, m: *mut JSModuleDef) -> JSAtom; } +extern "C" { + pub fn JS_FreeUnevaluatedModules(ctx: *mut JSContext); +} pub type JSJobFunc = ::std::option::Option< unsafe extern "C" fn( ctx: *mut JSContext, diff --git a/sys/src/bindings/aarch64-unknown-linux-musl.rs b/sys/src/bindings/aarch64-unknown-linux-musl.rs index c4ead954..cd86dd45 100644 --- a/sys/src/bindings/aarch64-unknown-linux-musl.rs +++ b/sys/src/bindings/aarch64-unknown-linux-musl.rs @@ -1881,6 +1881,9 @@ extern "C" { extern "C" { pub fn JS_GetModuleName(ctx: *mut JSContext, m: *mut JSModuleDef) -> JSAtom; } +extern "C" { + pub fn JS_FreeUnevaluatedModules(ctx: *mut JSContext); +} pub type JSJobFunc = ::std::option::Option< unsafe extern "C" fn( ctx: *mut JSContext, diff --git a/sys/src/bindings/i686-pc-windows-gnu.rs b/sys/src/bindings/i686-pc-windows-gnu.rs index e52e41a7..acef7d6e 100644 --- a/sys/src/bindings/i686-pc-windows-gnu.rs +++ b/sys/src/bindings/i686-pc-windows-gnu.rs @@ -1789,6 +1789,9 @@ extern "C" { extern "C" { pub fn JS_GetModuleName(ctx: *mut JSContext, m: *mut JSModuleDef) -> JSAtom; } +extern "C" { + pub fn JS_FreeUnevaluatedModules(ctx: *mut JSContext); +} pub type JSJobFunc = ::std::option::Option< unsafe extern "C" fn( ctx: *mut JSContext, diff --git a/sys/src/bindings/i686-pc-windows-msvc.rs b/sys/src/bindings/i686-pc-windows-msvc.rs index e39fb2d3..3f5b2370 100644 --- a/sys/src/bindings/i686-pc-windows-msvc.rs +++ b/sys/src/bindings/i686-pc-windows-msvc.rs @@ -1789,6 +1789,9 @@ extern "C" { extern "C" { pub fn JS_GetModuleName(ctx: *mut JSContext, m: *mut JSModuleDef) -> JSAtom; } +extern "C" { + pub fn JS_FreeUnevaluatedModules(ctx: *mut JSContext); +} pub type JSJobFunc = ::std::option::Option< unsafe extern "C" fn( ctx: *mut JSContext, diff --git a/sys/src/bindings/i686-unknown-linux-gnu.rs b/sys/src/bindings/i686-unknown-linux-gnu.rs index 033ce0a3..bfdf57ec 100644 --- a/sys/src/bindings/i686-unknown-linux-gnu.rs +++ b/sys/src/bindings/i686-unknown-linux-gnu.rs @@ -1789,6 +1789,9 @@ extern "C" { extern "C" { pub fn JS_GetModuleName(ctx: *mut JSContext, m: *mut JSModuleDef) -> JSAtom; } +extern "C" { + pub fn JS_FreeUnevaluatedModules(ctx: *mut JSContext); +} pub type JSJobFunc = ::std::option::Option< unsafe extern "C" fn( ctx: *mut JSContext, diff --git a/sys/src/bindings/x86_64-apple-darwin.rs b/sys/src/bindings/x86_64-apple-darwin.rs index d2b5c741..239045d7 100644 --- a/sys/src/bindings/x86_64-apple-darwin.rs +++ b/sys/src/bindings/x86_64-apple-darwin.rs @@ -1882,6 +1882,9 @@ extern "C" { extern "C" { pub fn JS_GetModuleName(ctx: *mut JSContext, m: *mut JSModuleDef) -> JSAtom; } +extern "C" { + pub fn JS_FreeUnevaluatedModules(ctx: *mut JSContext); +} pub type JSJobFunc = ::std::option::Option< unsafe extern "C" fn( ctx: *mut JSContext, diff --git a/sys/src/bindings/x86_64-pc-windows-gnu.rs b/sys/src/bindings/x86_64-pc-windows-gnu.rs index 39136560..a5c05de1 100644 --- a/sys/src/bindings/x86_64-pc-windows-gnu.rs +++ b/sys/src/bindings/x86_64-pc-windows-gnu.rs @@ -1881,6 +1881,9 @@ extern "C" { extern "C" { pub fn JS_GetModuleName(ctx: *mut JSContext, m: *mut JSModuleDef) -> JSAtom; } +extern "C" { + pub fn JS_FreeUnevaluatedModules(ctx: *mut JSContext); +} pub type JSJobFunc = ::std::option::Option< unsafe extern "C" fn( ctx: *mut JSContext, diff --git a/sys/src/bindings/x86_64-pc-windows-msvc.rs b/sys/src/bindings/x86_64-pc-windows-msvc.rs index 0cfc1713..9394bcca 100644 --- a/sys/src/bindings/x86_64-pc-windows-msvc.rs +++ b/sys/src/bindings/x86_64-pc-windows-msvc.rs @@ -1881,6 +1881,9 @@ extern "C" { extern "C" { pub fn JS_GetModuleName(ctx: *mut JSContext, m: *mut JSModuleDef) -> JSAtom; } +extern "C" { + pub fn JS_FreeUnevaluatedModules(ctx: *mut JSContext); +} pub type JSJobFunc = ::std::option::Option< unsafe extern "C" fn( ctx: *mut JSContext, diff --git a/sys/src/bindings/x86_64-unknown-linux-musl.rs b/sys/src/bindings/x86_64-unknown-linux-musl.rs index c4ead954..cd86dd45 100644 --- a/sys/src/bindings/x86_64-unknown-linux-musl.rs +++ b/sys/src/bindings/x86_64-unknown-linux-musl.rs @@ -1881,6 +1881,9 @@ extern "C" { extern "C" { pub fn JS_GetModuleName(ctx: *mut JSContext, m: *mut JSModuleDef) -> JSAtom; } +extern "C" { + pub fn JS_FreeUnevaluatedModules(ctx: *mut JSContext); +} pub type JSJobFunc = ::std::option::Option< unsafe extern "C" fn( ctx: *mut JSContext,