From f599d895852d247da36f09c1ead3e1da144e22a7 Mon Sep 17 00:00:00 2001 From: Sam Clegg Date: Tue, 16 Apr 2024 18:10:56 -0700 Subject: [PATCH] In MODULARIZE mode avoid modifying the incoming moduleArg. NFC This avoids leaking of the partially constructed module and means that only way to get access the module instance is via waiting on the promise. Previously we were attaching all our module properties to the incoming object, but not, as far as I can tell for any good reason. In case anybody was actually depending on this, inject thunks into the moduleArg that abort on access with an actionable error. --- src/closure-externs/closure-externs.js | 3 +- src/jsifier.mjs | 4 ++ src/postamble_modularize.js | 41 +++++++++++++++++++ src/shell.js | 4 +- test/other/test_unoptimized_code_size.js.size | 2 +- ...t_unoptimized_code_size_no_asserts.js.size | 2 +- .../test_unoptimized_code_size_strict.js.size | 2 +- test/test_other.py | 25 ++++++++--- tools/link.py | 14 +------ 9 files changed, 72 insertions(+), 25 deletions(-) create mode 100644 src/postamble_modularize.js diff --git a/src/closure-externs/closure-externs.js b/src/closure-externs/closure-externs.js index 512345e4c10ee..6a110ce5f1290 100644 --- a/src/closure-externs/closure-externs.js +++ b/src/closure-externs/closure-externs.js @@ -245,9 +245,8 @@ var moduleArg; * Used in MODULARIZE mode. * We need to access this after the code we pass to closure so from closure's * POV this is "extern". - * @suppress {duplicate} */ -var readyPromise; +var moduleRtn; /** * This was removed from upstream closure compiler in diff --git a/src/jsifier.mjs b/src/jsifier.mjs index 6f17578db8f23..22cf0c38cca11 100644 --- a/src/jsifier.mjs +++ b/src/jsifier.mjs @@ -744,6 +744,10 @@ var proxiedFunctionTable = [ includeFile(fileName, shouldPreprocess(fileName)); } + if (MODULARIZE) { + includeFile('postamble_modularize.js'); + } + print( '//FORWARDED_DATA:' + JSON.stringify({ diff --git a/src/postamble_modularize.js b/src/postamble_modularize.js new file mode 100644 index 0000000000000..098954d852a43 --- /dev/null +++ b/src/postamble_modularize.js @@ -0,0 +1,41 @@ +// In MODULARIZE mode we wrap the generated code in a factory function +// and return either the Module itself, or a promise of the module. +// +// We assign to the `moduleRtn` global here and configure closure to see +// this as and extern so it won't get minified. + +#if WASM_ASYNC_COMPILATION + +#if USE_READY_PROMISE +moduleRtn = readyPromise; +#else +moduleRtn = {}; +#endif + +#else // WASM_ASYNC_COMPILATION + +moduleRtn = Module; + +#endif // WASM_ASYNC_COMPILATION + +#if ASSERTIONS +// Assertion for attempting to use access modulue properties on the incoming +// moduleArg. In the past we used this object as the prototype of the module +// and assigned properties to it, but now we return a distinct object. This +// keeps the instance private until it is ready (i.e the promise has been +// resolved). +for (const prop of Object.keys(Module)) { + if (!(prop in moduleArg)) { + Object.defineProperty(moduleArg, prop, { + configurable: true, + get() { +#if WASM_ASYNC_COMPILATION + abort(`Access to module property ('${prop}') is no longer possible via the incoming module contructor argument; Instead, use the result of the module promise.`) +#else + abort(`Access to module property ('${prop}') is no longer possible via the module input argument; Instead, use the module constructor return value.`) +#endif + } + }); + } +} +#endif diff --git a/src/shell.js b/src/shell.js index 319d793d1c564..8aa6453d3eddd 100644 --- a/src/shell.js +++ b/src/shell.js @@ -10,7 +10,7 @@ // The Module object: Our interface to the outside world. We import // and export values on it. There are various ways Module can be used: // 1. Not defined. We create it here -// 2. A function parameter, function(Module) { ..generated code.. } +// 2. A function parameter, function(moduleArg) => Promise // 3. pre-run appended it, var Module = {}; ..generated code.. // 4. External script tag defines var Module. // We need to check if Module already exists (e.g. case 3 above). @@ -21,7 +21,7 @@ // before the code. Then that object will be used in the code, and you // can continue to use Module afterwards as well. #if MODULARIZE -var Module = moduleArg; +var Module = Object.assign({}, moduleArg); #elif USE_CLOSURE_COMPILER // if (!Module)` is crucial for Closure Compiler here as it will otherwise replace every `Module` occurrence with a string var /** @type {{ diff --git a/test/other/test_unoptimized_code_size.js.size b/test/other/test_unoptimized_code_size.js.size index 2fbf80c562263..ab60bcadfd6d3 100644 --- a/test/other/test_unoptimized_code_size.js.size +++ b/test/other/test_unoptimized_code_size.js.size @@ -1 +1 @@ -55580 +55579 diff --git a/test/other/test_unoptimized_code_size_no_asserts.js.size b/test/other/test_unoptimized_code_size_no_asserts.js.size index 5802b66090c51..6e6d776dba7f9 100644 --- a/test/other/test_unoptimized_code_size_no_asserts.js.size +++ b/test/other/test_unoptimized_code_size_no_asserts.js.size @@ -1 +1 @@ -31465 +31464 diff --git a/test/other/test_unoptimized_code_size_strict.js.size b/test/other/test_unoptimized_code_size_strict.js.size index 5678620799953..e4814168ec178 100644 --- a/test/other/test_unoptimized_code_size_strict.js.size +++ b/test/other/test_unoptimized_code_size_strict.js.size @@ -1 +1 @@ -54468 +54467 diff --git a/test/test_other.py b/test/test_other.py index 6d57569df3378..af1502dec06f9 100644 --- a/test/test_other.py +++ b/test/test_other.py @@ -6416,11 +6416,11 @@ def test_modularize_sync_compilation(self): console.log('before'); var result = Module(); // It should be an object. -console.log(typeof result); +console.log('typeof result: ' + typeof result); // And it should have the exports that Module has, showing it is Module in fact. -console.log(typeof result._main); +console.log('typeof _main: ' + typeof result._main); // And it should not be a Promise. -console.log(typeof result.then); +console.log('typeof result.then: ' + typeof result.then); console.log('after'); ''') self.run_process([EMCC, test_file('hello_world.c'), @@ -6430,12 +6430,25 @@ def test_modularize_sync_compilation(self): self.assertContained('''\ before hello, world! -object -function -undefined +typeof result: object +typeof _main: function +typeof result.then: undefined after ''', self.run_js('a.out.js')) + def test_modularize_argument_misuse(self): + create_file('test.c', ''' + #include + EMSCRIPTEN_KEEPALIVE int foo() { return 42; }''') + + create_file('post.js', r''' + var arg = { bar: 1 }; + var promise = Module(arg); + arg._foo();''') + + expected = "Aborted(Access to module property ('_foo') is no longer possible via the incoming module contructor argument; Instead, use the result of the module promise" + self.do_runf('test.c', expected, assert_returncode=NON_ZERO, emcc_args=['--no-entry', '-sMODULARIZE', '--extern-post-js=post.js']) + def test_export_all_3142(self): create_file('src.cpp', r''' typedef unsigned int Bit32u; diff --git a/tools/link.py b/tools/link.py index 8080ea8b87388..a029180c0b7bb 100644 --- a/tools/link.py +++ b/tools/link.py @@ -2350,31 +2350,21 @@ def modularize(options): shared.target_environment_may_be('web'): async_emit = 'async ' - # Return the incoming `moduleArg`. This is is equivalent to the `Module` var within the - # generated code but its not run through closure minification so we can reference it in - # the return statement. - return_value = 'moduleArg' - if settings.WASM_ASYNC_COMPILATION: - if settings.USE_READY_PROMISE: - return_value = 'readyPromise' - else: - return_value = '{}' - # TODO: Remove when https://bugs.webkit.org/show_bug.cgi?id=223533 is resolved. if async_emit != '' and settings.EXPORT_NAME == 'config': diagnostics.warning('emcc', 'EXPORT_NAME should not be named "config" when targeting Safari') src = ''' %(maybe_async)sfunction(moduleArg = {}) { + var moduleRtn; %(src)s - return %(return_value)s + return moduleRtn; } ''' % { 'maybe_async': async_emit, 'src': src, - 'return_value': return_value, } if settings.MINIMAL_RUNTIME and not settings.PTHREADS: