From 3d17962109d6662d4b9ac9077b8ecfc30b8d7c37 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Fri, 3 Feb 2023 19:43:08 -0800 Subject: [PATCH 1/5] Use RuntimeError in abort() before module instantiation We decided use a trap for `abort` function in case of Wasm EH in order to prevent infinite-looping (#16910). https://github.com/emscripten-core/emscripten/blob/44b2c2a1ecad39c534e14179acb49419dfee528b/src/preamble.js#L472-L474 The short reason is, in Wasm EH `RuntimeError` is treated as a foreign exception and caught by `catch_all`, so you try to abort the program and throw a `RuntimeError`, but it is caught by some `catch_all` used in the cleanup (=destructors, etc) code, causing the program to continue to run. `__trap` is defined in compiler-rt and exported when Wasm EH is enabled. This has worked so far, but in case we fail to instantiate a Wasm module and call `abort` because of it, we don't have access to the imported `Module['asm']['__trap']`, because `Module['asm']` has not been set: https://github.com/emscripten-core/emscripten/blob/44b2c2a1ecad39c534e14179acb49419dfee528b/src/preamble.js#L848 So the `abort` call will end like this: ```console TypeError: Cannot read properties of undefined (reading '__trap') at ___trap (/usr/local/google/home/aheejin/test/gl84/exported_api.js:5152:34) at abort (/usr/local/google/home/aheejin/test/gl84/exported_api.js:892:5) at /usr/local/google/home/aheejin/test/gl84/exported_api.js:1172:5 ``` which may not be the worst thing in the world given that we are crashing anyway, but not the situation we intended. This PR lets us throw `RuntimeError` in case we don't have the wasm module instantiated and have access to `Module['asm']['__trap']`. This is OK even with Wasm EH because we haven't been running the module, there's no risk of infinite-looping we tried to prevent in #16910. I'm not sure how to add a test for this, because the test would require a module that fails to instantiate. This was found while I was debugging something else. --- src/preamble.js | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/src/preamble.js b/src/preamble.js index fcb076e908b3f..0147300036944 100644 --- a/src/preamble.js +++ b/src/preamble.js @@ -469,20 +469,37 @@ function abort(what) { // defintion for WebAssembly.RuntimeError claims it takes no arguments even // though it can. // TODO(https://github.com/google/closure-compiler/pull/3913): Remove if/when upstream closure gets fixed. -#if WASM_EXCEPTIONS == 1 - // See above, in the meantime, we resort to wasm code for trapping. - ___trap(); -#else - /** @suppress {checkTypes} */ - var e = new WebAssembly.RuntimeError(what); + + function throwError(what) { + /** @suppress {checkTypes} */ + var e = new WebAssembly.RuntimeError(what); #if MODULARIZE - readyPromiseReject(e); + readyPromiseReject(e); #endif - // Throw the error whether or not MODULARIZE is set because abort is used - // in code paths apart from instantiation where an exception is expected - // to be thrown when abort is called. - throw e; + // Throw the error whether or not MODULARIZE is set because abort is used + // in code paths apart from instantiation where an exception is expected + // to be thrown when abort is called. + throw e; + } + +#if WASM_EXCEPTIONS == 1 + // See above, in the meantime, we resort to wasm code for trapping. + // + // In case abort() is called before the module is initialized, Module['asm'] + // and its exported '__trap' function is not available, in which case we throw + // a RuntimeError. + // + // We trap instead of throwing RuntimeError to prevent infinite-looping in + // Wasm EH code (because RuntimeError is considered as a foreign exception and + // caught by 'catch_all'), but in case throwing RuntimeError is fine because + // the module has not even been instantiated, even less running. + if (typeof Module['asm'] !== 'undefined') + ___trap(); + else + throwError(what); +#else + throwError(what); #endif } From 8dcdcb27cc854f386201f85755e4a25257b753b8 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 7 Feb 2023 14:32:07 -0800 Subject: [PATCH 2/5] Update src/preamble.js Co-authored-by: Alon Zakai --- src/preamble.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/preamble.js b/src/preamble.js index 0147300036944..d06bfd3c4872c 100644 --- a/src/preamble.js +++ b/src/preamble.js @@ -494,13 +494,11 @@ function abort(what) { // Wasm EH code (because RuntimeError is considered as a foreign exception and // caught by 'catch_all'), but in case throwing RuntimeError is fine because // the module has not even been instantiated, even less running. - if (typeof Module['asm'] !== 'undefined') + if (typeof Module['asm'] !== 'undefined') { ___trap(); - else - throwError(what); -#else - throwError(what); + } #endif + throwError(what); } #include "memoryprofiler.js" From a48c85e19c2a5b5cdb621db6556e3dff9b0f700a Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 7 Feb 2023 14:33:15 -0800 Subject: [PATCH 3/5] Remove `throwError` --- src/preamble.js | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/preamble.js b/src/preamble.js index d06bfd3c4872c..14bf55faf1c7b 100644 --- a/src/preamble.js +++ b/src/preamble.js @@ -470,19 +470,6 @@ function abort(what) { // though it can. // TODO(https://github.com/google/closure-compiler/pull/3913): Remove if/when upstream closure gets fixed. - function throwError(what) { - /** @suppress {checkTypes} */ - var e = new WebAssembly.RuntimeError(what); - -#if MODULARIZE - readyPromiseReject(e); -#endif - // Throw the error whether or not MODULARIZE is set because abort is used - // in code paths apart from instantiation where an exception is expected - // to be thrown when abort is called. - throw e; - } - #if WASM_EXCEPTIONS == 1 // See above, in the meantime, we resort to wasm code for trapping. // @@ -498,7 +485,16 @@ function abort(what) { ___trap(); } #endif - throwError(what); + /** @suppress {checkTypes} */ + var e = new WebAssembly.RuntimeError(what); + +#if MODULARIZE + readyPromiseReject(e); +#endif + // Throw the error whether or not MODULARIZE is set because abort is used + // in code paths apart from instantiation where an exception is expected + // to be thrown when abort is called. + throw e; } #include "memoryprofiler.js" From 060e9e7d4795d98a54261db63a35efb589cf57f5 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 7 Feb 2023 14:34:02 -0800 Subject: [PATCH 4/5] Address comments --- src/preamble.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/preamble.js b/src/preamble.js index 14bf55faf1c7b..5ecc6bbde5780 100644 --- a/src/preamble.js +++ b/src/preamble.js @@ -469,7 +469,6 @@ function abort(what) { // defintion for WebAssembly.RuntimeError claims it takes no arguments even // though it can. // TODO(https://github.com/google/closure-compiler/pull/3913): Remove if/when upstream closure gets fixed. - #if WASM_EXCEPTIONS == 1 // See above, in the meantime, we resort to wasm code for trapping. // @@ -481,7 +480,7 @@ function abort(what) { // Wasm EH code (because RuntimeError is considered as a foreign exception and // caught by 'catch_all'), but in case throwing RuntimeError is fine because // the module has not even been instantiated, even less running. - if (typeof Module['asm'] !== 'undefined') { + if (runtimeInitialized) { ___trap(); } #endif From 89bf29460c6b4a09ecb02a50cdd6271f0c500ebf Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 7 Feb 2023 14:34:29 -0800 Subject: [PATCH 5/5] Indent fix --- src/preamble.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/preamble.js b/src/preamble.js index 5ecc6bbde5780..afcae6766d5d2 100644 --- a/src/preamble.js +++ b/src/preamble.js @@ -493,7 +493,7 @@ function abort(what) { // Throw the error whether or not MODULARIZE is set because abort is used // in code paths apart from instantiation where an exception is expected // to be thrown when abort is called. - throw e; + throw e; } #include "memoryprofiler.js"