Skip to content

Commit

Permalink
In MODULARIZE mode avoid modifying the incoming moduleArg. NFC
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sbc100 committed Apr 17, 2024
1 parent 3f15cfd commit 6f1d807
Show file tree
Hide file tree
Showing 9 changed files with 72 additions and 25 deletions.
3 changes: 1 addition & 2 deletions src/closure-externs/closure-externs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions src/jsifier.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -744,6 +744,10 @@ var proxiedFunctionTable = [
includeFile(fileName, shouldPreprocess(fileName));
}

if (MODULARIZE) {
includeFile('postamble_modularize.js');
}

print(
'//FORWARDED_DATA:' +
JSON.stringify({
Expand Down
41 changes: 41 additions & 0 deletions src/postamble_modularize.js
Original file line number Diff line number Diff line change
@@ -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 access module 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
4 changes: 2 additions & 2 deletions src/shell.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<Module>
// 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).
Expand All @@ -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 {{
Expand Down
2 changes: 1 addition & 1 deletion test/other/test_unoptimized_code_size.js.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
55580
55579
2 changes: 1 addition & 1 deletion test/other/test_unoptimized_code_size_no_asserts.js.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
31465
31464
2 changes: 1 addition & 1 deletion test/other/test_unoptimized_code_size_strict.js.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
54468
54467
25 changes: 19 additions & 6 deletions test/test_other.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand All @@ -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.h>
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;
Expand Down
14 changes: 2 additions & 12 deletions tools/link.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down

0 comments on commit 6f1d807

Please sign in to comment.