From d506ad00c407c55248ad0822939ee5b1f7b74f65 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Mon, 12 May 2025 17:01:27 +0100 Subject: [PATCH 1/4] Test and fix for incompatible HEAP transforms While working on #24295, I noticed I could accidentally prevent ASan and SAFE_HEAP from finding any `HEAPxx[n]` accesses from JS when `ALLOW_MEMORY_GROWTH` is enabled, and no tests would catch it, because no tests verify that acorn-optimizer passes which change `HEAPxx[n]` into something else can work in conjunction with each other. After adding the test, I found that an existing `SUPPORT_BIG_ENDIAN` feature was already broken in the same way, because it transforms `HEAPxx[n]` accesses into DataView methods, and since it was running before ASan and SAFE_HEAP, those latter transforms became no-ops. I fixed that in the same PR by moving the big-endian transform to the end. In the future we might want to consolidate those transforms in a more reliable way (e.g. only run a single code transform that does `HEAPxx[n]` -> `__loadHeap`/`__storeHeap` where those functions do all the work using `#if`s), but at least for now this PR fixes an immediate problem and catches potential future regressions. --- test/other/test_null_deref_via_js.c | 7 +++++++ test/test_other.py | 18 ++++++++++++++++++ tools/link.py | 22 ++++++++++++---------- 3 files changed, 37 insertions(+), 10 deletions(-) create mode 100644 test/other/test_null_deref_via_js.c diff --git a/test/other/test_null_deref_via_js.c b/test/other/test_null_deref_via_js.c new file mode 100644 index 0000000000000..600e38fda591b --- /dev/null +++ b/test/other/test_null_deref_via_js.c @@ -0,0 +1,7 @@ +#include + +int main() { + EM_ASM({ + HEAP32[0] = 0; + }); +} diff --git a/test/test_other.py b/test/test_other.py index 50227450ce043..c9eebfbd1d268 100644 --- a/test/test_other.py +++ b/test/test_other.py @@ -12235,6 +12235,24 @@ def test_asan_strncpy(self): # https://github.com/emscripten-core/emscripten/issues/14618 self.do_runf('other/test_asan_strncpy.c', emcc_args=['-fsanitize=address']) + @parameterized({ + 'asan': ['AddressSanitizer: null-pointer-dereference', '-fsanitize=address'], + 'safe_heap': ['Aborted(segmentation fault storing 4 bytes at address 0)', '-sSAFE_HEAP'], + }) + @parameterized({ + '': [], + 'memgrowth': ['-pthread', '-sALLOW_MEMORY_GROWTH', '-Wno-pthreads-mem-growth'], + }) + def test_null_deref_via_js(self, expected_output, *args): + # Multiple JS transforms look for pattern like `HEAPxx[...]` and transform it. + # This test ensures that one of the transforms doesn't produce a pattern that + # another pass can't find anymore, that is that features can work in conjunction. + self.do_runf( + 'other/test_null_deref_via_js.c', + emcc_args=args, + assert_returncode=NON_ZERO, + expected_output=[expected_output]) + @node_pthreads def test_proxy_to_pthread_stack(self): # Check that the proxied main gets run with STACK_SIZE setting and not diff --git a/tools/link.py b/tools/link.py index 2d573ba844a0d..0f0bb92fe1486 100644 --- a/tools/link.py +++ b/tools/link.py @@ -2322,10 +2322,6 @@ def phase_binaryen(target, options, wasm_target): # after generating the wasm, do some final operations if final_js: - if settings.SUPPORT_BIG_ENDIAN: - with ToolchainProfiler.profile_block('little_endian_heap'): - final_js = building.little_endian_heap(final_js) - # >=2GB heap support requires pointers in JS to be unsigned. rather than # require all pointers to be unsigned by default, which increases code size # a little, keep them signed, and just unsign them here if we need that. @@ -2333,20 +2329,26 @@ def phase_binaryen(target, options, wasm_target): with ToolchainProfiler.profile_block('use_unsigned_pointers_in_js'): final_js = building.use_unsigned_pointers_in_js(final_js) + if settings.USE_ASAN: + final_js = building.instrument_js_for_asan(final_js) + + if settings.SAFE_HEAP: + final_js = building.instrument_js_for_safe_heap(final_js) + # shared memory growth requires some additional JS fixups. # note that we must do this after handling of unsigned pointers. unsigning # adds some >>> 0 things, while growth will replace a HEAP8 with a call to # a method to get the heap, and that call would not be recognized by the - # unsigning pass + # unsigning pass. + # we also must do this after the asan or safe_heap instrumentation, as they + # wouldn't be able to recognize patterns produced by the growth pass. if settings.SHARED_MEMORY and settings.ALLOW_MEMORY_GROWTH: with ToolchainProfiler.profile_block('apply_wasm_memory_growth'): final_js = building.apply_wasm_memory_growth(final_js) - if settings.USE_ASAN: - final_js = building.instrument_js_for_asan(final_js) - - if settings.SAFE_HEAP: - final_js = building.instrument_js_for_safe_heap(final_js) + if settings.SUPPORT_BIG_ENDIAN: + with ToolchainProfiler.profile_block('little_endian_heap'): + final_js = building.little_endian_heap(final_js) if settings.OPT_LEVEL >= 2 and settings.DEBUG_LEVEL <= 2: # minify the JS. Do not minify whitespace if Closure is used, so that From 9856de99816a156195fcae8310dbd995a7174a33 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Wed, 21 May 2025 12:02:52 +0100 Subject: [PATCH 2/4] Reuse existing test file --- test/other/test_null_deref_via_js.c | 7 ------- test/{core => other}/test_safe_heap_user_js.c | 0 test/test_core.py | 8 -------- test/test_other.py | 4 ++-- 4 files changed, 2 insertions(+), 17 deletions(-) delete mode 100644 test/other/test_null_deref_via_js.c rename test/{core => other}/test_safe_heap_user_js.c (100%) diff --git a/test/other/test_null_deref_via_js.c b/test/other/test_null_deref_via_js.c deleted file mode 100644 index 600e38fda591b..0000000000000 --- a/test/other/test_null_deref_via_js.c +++ /dev/null @@ -1,7 +0,0 @@ -#include - -int main() { - EM_ASM({ - HEAP32[0] = 0; - }); -} diff --git a/test/core/test_safe_heap_user_js.c b/test/other/test_safe_heap_user_js.c similarity index 100% rename from test/core/test_safe_heap_user_js.c rename to test/other/test_safe_heap_user_js.c diff --git a/test/test_core.py b/test/test_core.py index a6da1c8756a60..f5677b627c86f 100644 --- a/test/test_core.py +++ b/test/test_core.py @@ -9084,14 +9084,6 @@ def test_asan_modularized_with_closure(self): self.set_setting('INITIAL_MEMORY', '300mb') self.do_run_in_out_file_test('hello_world.c') - @no_asan('SAFE_HEAP cannot be used with ASan') - @no_2gb('asan doesnt support GLOBAL_BASE') - @no_esm_integration('sanitizers do not support WASM_ESM_INTEGRATION') - def test_safe_heap_user_js(self): - self.set_setting('SAFE_HEAP') - self.do_runf('core/test_safe_heap_user_js.c', - expected_output=['Aborted(segmentation fault storing 1 bytes at address 0)'], assert_returncode=NON_ZERO) - def test_safe_stack(self): self.set_setting('STACK_OVERFLOW_CHECK', 2) self.set_setting('STACK_SIZE', 1024) diff --git a/test/test_other.py b/test/test_other.py index c9eebfbd1d268..318dd26176b72 100644 --- a/test/test_other.py +++ b/test/test_other.py @@ -12237,7 +12237,7 @@ def test_asan_strncpy(self): @parameterized({ 'asan': ['AddressSanitizer: null-pointer-dereference', '-fsanitize=address'], - 'safe_heap': ['Aborted(segmentation fault storing 4 bytes at address 0)', '-sSAFE_HEAP'], + 'safe_heap': ['Aborted(segmentation fault storing 1 bytes at address 0)', '-sSAFE_HEAP'], }) @parameterized({ '': [], @@ -12248,7 +12248,7 @@ def test_null_deref_via_js(self, expected_output, *args): # This test ensures that one of the transforms doesn't produce a pattern that # another pass can't find anymore, that is that features can work in conjunction. self.do_runf( - 'other/test_null_deref_via_js.c', + 'other/test_safe_heap_user_js.c', emcc_args=args, assert_returncode=NON_ZERO, expected_output=[expected_output]) From 8545d0e6e3f0bd3118b5dec37d49097177389b45 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Wed, 21 May 2025 12:57:46 +0100 Subject: [PATCH 3/4] Fix ASan checks for newly hit code paths --- src/lib/libpthread.js | 2 +- src/runtime_asan.js | 17 +++++++++++------ tools/acorn-optimizer.mjs | 5 ++++- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/lib/libpthread.js b/src/lib/libpthread.js index fd2a72de0a31f..6d979627efe31 100644 --- a/src/lib/libpthread.js +++ b/src/lib/libpthread.js @@ -978,7 +978,7 @@ var LibraryPThread = { $establishStackSpace__internal: true, $establishStackSpace__deps: ['$stackRestore', 'emscripten_stack_set_limits'], - $establishStackSpace: (pthread_ptr) => { + $establishStackSpace: function (pthread_ptr) { var stackHigh = {{{ makeGetValue('pthread_ptr', C_STRUCTS.pthread.stack, '*') }}}; var stackSize = {{{ makeGetValue('pthread_ptr', C_STRUCTS.pthread.stack_size, '*') }}}; var stackLow = stackHigh - stackSize; diff --git a/src/runtime_asan.js b/src/runtime_asan.js index d6e522c2d41b7..34cafeffcf320 100644 --- a/src/runtime_asan.js +++ b/src/runtime_asan.js @@ -12,18 +12,23 @@ // ASan instrumentation on them. However, until the wasm module is ready, we // must access things directly. -function _asan_js_load(arr, index) { +function _asan_js_check_index(arr, index, asanFn) { +#if EXIT_RUNTIME + if (runtimeInitialized && !runtimeExited) { +#else if (runtimeInitialized) { +#endif const elemSize = arr.BYTES_PER_ELEMENT; - ___asan_loadN(index * elemSize, elemSize); + asanFn(index * elemSize, elemSize); } +} + +function _asan_js_load(arr, index) { + _asan_js_check_index(arr, index, ___asan_loadN); return arr[index]; } function _asan_js_store(arr, index, value) { - if (runtimeInitialized) { - const elemSize = arr.BYTES_PER_ELEMENT; - ___asan_storeN(index * elemSize, elemSize); - } + _asan_js_check_index(arr, index, ___asan_storeN); return arr[index] = value; } diff --git a/tools/acorn-optimizer.mjs b/tools/acorn-optimizer.mjs index 0d02295eb9abf..b661561863519 100755 --- a/tools/acorn-optimizer.mjs +++ b/tools/acorn-optimizer.mjs @@ -1372,7 +1372,10 @@ function isHEAPAccess(node) { function asanify(ast) { recursiveWalk(ast, { FunctionDeclaration(node, c) { - if (node.id.type === 'Identifier' && node.id.name.startsWith('_asan_js_')) { + if ( + node.id.type === 'Identifier' && + (node.id.name.startsWith('_asan_js_') || node.id.name === 'establishStackSpace') + ) { // do not recurse into this js impl function, which we use during // startup before the wasm is ready } else { From 1598f8d55877f2e9a8b4baabba71af000b09ec52 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Wed, 21 May 2025 14:06:34 +0100 Subject: [PATCH 4/4] Rebaseline --- test/other/codesize/test_codesize_minimal_pthreads.gzsize | 2 +- test/other/codesize/test_codesize_minimal_pthreads.jssize | 2 +- .../codesize/test_codesize_minimal_pthreads_memgrowth.gzsize | 2 +- .../codesize/test_codesize_minimal_pthreads_memgrowth.jssize | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/other/codesize/test_codesize_minimal_pthreads.gzsize b/test/other/codesize/test_codesize_minimal_pthreads.gzsize index b7250e74c4249..cf41364ab9510 100644 --- a/test/other/codesize/test_codesize_minimal_pthreads.gzsize +++ b/test/other/codesize/test_codesize_minimal_pthreads.gzsize @@ -1 +1 @@ -3785 +3786 diff --git a/test/other/codesize/test_codesize_minimal_pthreads.jssize b/test/other/codesize/test_codesize_minimal_pthreads.jssize index e203aec0f8b4b..b688862fcc76b 100644 --- a/test/other/codesize/test_codesize_minimal_pthreads.jssize +++ b/test/other/codesize/test_codesize_minimal_pthreads.jssize @@ -1 +1 @@ -7816 +7827 diff --git a/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.gzsize b/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.gzsize index 6cd1cb4835d89..586557492ae4d 100644 --- a/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.gzsize +++ b/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.gzsize @@ -1 +1 @@ -3985 +3989 diff --git a/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.jssize b/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.jssize index 94e3a63d098bf..dd601261add0e 100644 --- a/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.jssize +++ b/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.jssize @@ -1 +1 @@ -8247 +8258