From b74634b0c77cd61a900b6666ae464b3431003eed Mon Sep 17 00:00:00 2001 From: Sam Clegg Date: Thu, 16 Dec 2021 10:16:04 -0800 Subject: [PATCH 1/3] Fix bug where ASan shadow region was overlapping static data This bug was caused by miscalculating the adjustments make to TOTAL_MEMORY and GLOBAL_BASE and using ASan. The bug was introduced in #14631 and was causing ASan to write poison values over static data in some cases. This bug was really only effecting builds without ALLOW_MEMORY_GROWTH since when ALLOW_MEMORY_GROWTH is enabled that shadow region is very large and unless users were use a lot of memory it was unlikely ASan would write to the end of it (and clobber static data). As well as fixing the calculations, this change also adds a runtime assert to ensure these regions never overlap (at least not at startup). This should catch regressions in these calculations. In order to test this case I removed ALLOW_MEMORY_GROWTH form several asan and lsan tests. It should not longer be necessary to set `ALLOW_MEMORY_GROWTH` or adjust `TOTAL_MEMORY` when using sanitizers these days. The `asan` suite in the core tests still uses `ALLOW_MEMORY_GROWTH`. As a followup we might want to look into removing that too. Fixes: #15762 --- emcc.py | 58 +++++++++++++------ .../compiler-rt/lib/asan/asan_emscripten.cpp | 11 ++++ tests/test_other.py | 34 ++++++----- 3 files changed, 70 insertions(+), 33 deletions(-) diff --git a/emcc.py b/emcc.py index 4147460a8e1e0..e84600fb8624a 100755 --- a/emcc.py +++ b/emcc.py @@ -187,6 +187,11 @@ def base64_encode(b): return b64.decode('ascii') +def align_to_wasm_page_boudary(address): + page_size = webassembly.WASM_PAGE_SIZE + return ((address + (page_size - 1)) // page_size) * page_size + + @unique class OFormat(Enum): # Output a relocatable object file. We use this @@ -2179,6 +2184,17 @@ def check_memory_setting(setting): 'emscripten_builtin_free', ] + if ('leak' in sanitize or 'address' in sanitize) and not settings.ALLOW_MEMORY_GROWTH: + # Increase the minimum memory requirements to account for extra memory + # that the sanitizers might need (in addition to the shadow memory + # requirements handled below). + # These values are designed be over-estimate the actual requirements and + # based on experimentation with different tests/programs under asan and + # lsan. + settings.INITIAL_MEMORY += 50 * 1024 * 1024 + if settings.USE_PTHREADS: + settings.INITIAL_MEMORY += 50 * 1024 * 1024 + if settings.USE_OFFSET_CONVERTER and settings.WASM2JS: exit_with_error('wasm2js is not compatible with USE_OFFSET_CONVERTER (see #14630)') @@ -2235,27 +2251,31 @@ def check_memory_setting(setting): if settings.GLOBAL_BASE != -1: exit_with_error("ASan does not support custom GLOBAL_BASE") - max_mem = settings.INITIAL_MEMORY + user_mem = settings.INITIAL_MEMORY if settings.ALLOW_MEMORY_GROWTH: - max_mem = settings.MAXIMUM_MEMORY - - shadow_size = max_mem // 8 - settings.GLOBAL_BASE = shadow_size - - sanitizer_mem = (shadow_size + webassembly.WASM_PAGE_SIZE) & ~webassembly.WASM_PAGE_SIZE - # sanitizers do at least 9 page allocs of a single page during startup. - sanitizer_mem += webassembly.WASM_PAGE_SIZE * 9 - # we also allocate at least 11 "regions". Each region is kRegionSize (2 << 20) but - # MmapAlignedOrDieOnFatalError adds another 2 << 20 for alignment. - sanitizer_mem += (1 << 21) * 11 - # When running in the threaded mode asan needs to allocate an array of kMaxNumberOfThreads - # (1 << 22) pointers. See compiler-rt/lib/asan/asan_thread.cpp. - if settings.USE_PTHREADS: - sanitizer_mem += (1 << 22) * 4 + user_mem = settings.MAXIMUM_MEMORY - # Increase the size of the initial memory according to how much memory - # we think the sanitizers will use. - settings.INITIAL_MEMORY += sanitizer_mem + # The total memory is 1/8th shadow and 7/8th user/usable memory. + # Given the know value of user memory size we can work backwards + # to find the total memory and the shadow size based on the fact + # that the user memory is 7/8ths of the total memory. + # (i.e. user_mem == total_mem * 7 / 8 + total_mem = user_mem * 8 / 7 + + # But we might need to re-align to wasm page size + total_mem = int(align_to_wasm_page_boudary(total_mem)) + + # The shadow size is 1/8th the resulting rounded up size + shadow_size = total_mem // 8 + + # We start our global data after the shadow memory. + # We don't need to worry about alignment here. wasm-ld will take care of that. + settings.GLOBAL_BASE = shadow_size + 1 + + if not settings.ALLOW_MEMORY_GROWTH: + settings.INITIAL_MEMORY = total_mem + else: + settings.INITIAL_MEMORY += align_to_wasm_page_boudary(shadow_size) if settings.SAFE_HEAP: # SAFE_HEAP instruments ASan's shadow memory accesses. diff --git a/system/lib/compiler-rt/lib/asan/asan_emscripten.cpp b/system/lib/compiler-rt/lib/asan/asan_emscripten.cpp index a43d5a285e100..17e18287337dc 100644 --- a/system/lib/compiler-rt/lib/asan/asan_emscripten.cpp +++ b/system/lib/compiler-rt/lib/asan/asan_emscripten.cpp @@ -8,6 +8,8 @@ #if SANITIZER_EMSCRIPTEN #include +#include +#include #include #include #define __ATTRP_C11_THREAD ((void*)(uptr)-1) @@ -18,6 +20,15 @@ void InitializeShadowMemory() { // Poison the shadow memory of the shadow area at the start of the address // space. This helps catching null pointer dereference. FastPoisonShadow(kLowShadowBeg, kLowShadowEnd - kLowShadowBeg, 0xff); + + // Assert that the shadow region is large enough. We don't want to start + // running into the static data region which starts right after the shadow + // region. + uptr max_address = __builtin_wasm_memory_size(0) * WASM_PAGE_SIZE; + uptr max_shadow_address = MEM_TO_SHADOW(max_address); + // TODO(sbc): In the growable memory case we should really be checking this + // every time we grow. + assert(max_shadow_address <= kLowShadowEnd && "shadow region is too small"); } void AsanCheckDynamicRTPrereqs() {} diff --git a/tests/test_other.py b/tests/test_other.py index eed8d9fe68741..f5a16d98b3507 100644 --- a/tests/test_other.py +++ b/tests/test_other.py @@ -9312,12 +9312,13 @@ def test_malloc_none(self): self.assertContained('undefined symbol: malloc', stderr) @parameterized({ - 'c': ['c'], - 'cpp': ['cpp'], + 'c': ['c', []], + 'cpp': ['cpp', []], + 'growth': ['cpp', ['-sALLOW_MEMORY_GROWTH']], }) - def test_lsan_leaks(self, ext): + def test_lsan_leaks(self, ext, args): self.do_smart_test(test_file('other/test_lsan_leaks.' + ext), - emcc_args=['-fsanitize=leak', '-s', 'ALLOW_MEMORY_GROWTH'], + emcc_args=['-fsanitize=leak'] + args, assert_returncode=NON_ZERO, literals=[ 'Direct leak of 2048 byte(s) in 1 object(s) allocated from', 'Direct leak of 1337 byte(s) in 1 object(s) allocated from', @@ -9342,7 +9343,7 @@ def test_lsan_leaks(self, ext): }) def test_lsan_stack_trace(self, ext, regexes): self.do_smart_test(test_file('other/test_lsan_leaks.' + ext), - emcc_args=['-fsanitize=leak', '-s', 'ALLOW_MEMORY_GROWTH', '-gsource-map'], + emcc_args=['-fsanitize=leak', '-gsource-map'], assert_returncode=NON_ZERO, literals=[ 'Direct leak of 2048 byte(s) in 1 object(s) allocated from', 'Direct leak of 1337 byte(s) in 1 object(s) allocated from', @@ -9355,12 +9356,12 @@ def test_lsan_stack_trace(self, ext, regexes): }) def test_lsan_no_leak(self, ext): self.do_smart_test(test_file('other/test_lsan_no_leak.' + ext), - emcc_args=['-fsanitize=leak', '-s', 'ALLOW_MEMORY_GROWTH', '-s', 'ASSERTIONS=0'], + emcc_args=['-fsanitize=leak', '-s', 'ASSERTIONS=0'], regexes=[r'^\s*$']) def test_lsan_no_stack_trace(self): self.do_smart_test(test_file('other/test_lsan_leaks.c'), - emcc_args=['-fsanitize=leak', '-s', 'ALLOW_MEMORY_GROWTH', '-DDISABLE_CONTEXT'], + emcc_args=['-fsanitize=leak', '-DDISABLE_CONTEXT'], assert_returncode=NON_ZERO, literals=[ 'Direct leak of 3427 byte(s) in 3 object(s) allocated from:', 'SUMMARY: LeakSanitizer: 3427 byte(s) leaked in 3 allocation(s).', @@ -9368,26 +9369,33 @@ def test_lsan_no_stack_trace(self): def test_asan_null_deref(self): self.do_smart_test(test_file('other/test_asan_null_deref.c'), - emcc_args=['-fsanitize=address', '-sALLOW_MEMORY_GROWTH=1'], + emcc_args=['-fsanitize=address'], + assert_returncode=NON_ZERO, literals=[ + 'AddressSanitizer: null-pointer-dereference on address', + ]) + + def test_asan_memory_growth(self): + self.do_smart_test(test_file('other/test_asan_null_deref.c'), + emcc_args=['-fsanitize=address', '-sALLOW_MEMORY_GROWTH'], assert_returncode=NON_ZERO, literals=[ 'AddressSanitizer: null-pointer-dereference on address', ]) def test_asan_no_stack_trace(self): self.do_smart_test(test_file('other/test_lsan_leaks.c'), - emcc_args=['-fsanitize=address', '-sALLOW_MEMORY_GROWTH=1', '-DDISABLE_CONTEXT', '-s', 'EXIT_RUNTIME'], + emcc_args=['-fsanitize=address', '-DDISABLE_CONTEXT', '-s', 'EXIT_RUNTIME'], assert_returncode=NON_ZERO, literals=[ 'Direct leak of 3427 byte(s) in 3 object(s) allocated from:', 'SUMMARY: AddressSanitizer: 3427 byte(s) leaked in 3 allocation(s).', ]) def test_asan_pthread_stubs(self): - self.do_smart_test(test_file('other/test_asan_pthread_stubs.c'), emcc_args=['-fsanitize=address', '-sALLOW_MEMORY_GROWTH']) + self.do_smart_test(test_file('other/test_asan_pthread_stubs.c'), emcc_args=['-fsanitize=address']) def test_asan_strncpy(self): # Regression test for asan false positives in strncpy: # https://github.com/emscripten-core/emscripten/issues/14618 - self.do_smart_test(test_file('other/test_asan_strncpy.c'), emcc_args=['-fsanitize=address', '-sALLOW_MEMORY_GROWTH']) + self.do_smart_test(test_file('other/test_asan_strncpy.c'), emcc_args=['-fsanitize=address']) @node_pthreads def test_proxy_to_pthread_stack(self): @@ -9491,7 +9499,7 @@ def test_mmap_and_munmap_anonymous(self): self.do_other_test('test_mmap_and_munmap_anonymous.cpp', emcc_args=['-s', 'NO_FILESYSTEM']) def test_mmap_and_munmap_anonymous_asan(self): - self.do_other_test('test_mmap_and_munmap_anonymous.cpp', emcc_args=['-s', 'NO_FILESYSTEM', '-fsanitize=address', '-s', 'ALLOW_MEMORY_GROWTH']) + self.do_other_test('test_mmap_and_munmap_anonymous.cpp', emcc_args=['-s', 'NO_FILESYSTEM', '-fsanitize=address']) def test_mmap_memorygrowth(self): self.do_other_test('test_mmap_memorygrowth.cpp', ['-s', 'ALLOW_MEMORY_GROWTH']) @@ -11070,7 +11078,6 @@ def test_pthread_lsan_no_leak(self): self.set_setting('USE_PTHREADS') self.set_setting('PROXY_TO_PTHREAD') self.set_setting('EXIT_RUNTIME') - self.set_setting('INITIAL_MEMORY', '256MB') self.emcc_args += ['-gsource-map'] self.do_run_in_out_file_test(test_file('pthread/test_pthread_lsan_no_leak.cpp'), emcc_args=['-fsanitize=leak']) self.do_run_in_out_file_test(test_file('pthread/test_pthread_lsan_no_leak.cpp'), emcc_args=['-fsanitize=address']) @@ -11080,7 +11087,6 @@ def test_pthread_lsan_leak(self): self.set_setting('USE_PTHREADS') self.set_setting('PROXY_TO_PTHREAD') self.set_setting('EXIT_RUNTIME') - self.set_setting('INITIAL_MEMORY', '256MB') self.add_pre_run("Module['LSAN_OPTIONS'] = 'exitcode=0'") self.emcc_args += ['-gsource-map'] expected = [ From 69477c0c4287c971192a2a2ddb13e4d7bac35e88 Mon Sep 17 00:00:00 2001 From: Sam Clegg Date: Thu, 16 Dec 2021 14:44:58 -0800 Subject: [PATCH 2/3] minus one --- emcc.py | 15 +++++++++++---- .../lib/compiler-rt/lib/asan/asan_emscripten.cpp | 3 ++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/emcc.py b/emcc.py index e84600fb8624a..19fd55ff06722 100755 --- a/emcc.py +++ b/emcc.py @@ -2188,8 +2188,8 @@ def check_memory_setting(setting): # Increase the minimum memory requirements to account for extra memory # that the sanitizers might need (in addition to the shadow memory # requirements handled below). - # These values are designed be over-estimate the actual requirements and - # based on experimentation with different tests/programs under asan and + # These values are designed be an over-estimate of the actual requirements and + # are based on experimentation with different tests/programs under asan and # lsan. settings.INITIAL_MEMORY += 50 * 1024 * 1024 if settings.USE_PTHREADS: @@ -2251,11 +2251,18 @@ def check_memory_setting(setting): if settings.GLOBAL_BASE != -1: exit_with_error("ASan does not support custom GLOBAL_BASE") + # Increase the TOTAL_MEMORY and shift GLOBAL_BASE to account for + # the ASan shadow region which starts at address zero. + # The shadow region is 1/8th the size of the total memory and is + # itself part of the total memory. + # We use the following variables in this calculation: + # - user_mem : memory usable/visible by the user program. + # - shadow_size : memory used by asan for shadow memory. + # - total_mem : the sum of the above. this is the size of the wasm memory (and must be aligned to WASM_PAGE_SIZE) user_mem = settings.INITIAL_MEMORY if settings.ALLOW_MEMORY_GROWTH: user_mem = settings.MAXIMUM_MEMORY - # The total memory is 1/8th shadow and 7/8th user/usable memory. # Given the know value of user memory size we can work backwards # to find the total memory and the shadow size based on the fact # that the user memory is 7/8ths of the total memory. @@ -2270,7 +2277,7 @@ def check_memory_setting(setting): # We start our global data after the shadow memory. # We don't need to worry about alignment here. wasm-ld will take care of that. - settings.GLOBAL_BASE = shadow_size + 1 + settings.GLOBAL_BASE = shadow_size if not settings.ALLOW_MEMORY_GROWTH: settings.INITIAL_MEMORY = total_mem diff --git a/system/lib/compiler-rt/lib/asan/asan_emscripten.cpp b/system/lib/compiler-rt/lib/asan/asan_emscripten.cpp index 17e18287337dc..90b3c5bbae8ee 100644 --- a/system/lib/compiler-rt/lib/asan/asan_emscripten.cpp +++ b/system/lib/compiler-rt/lib/asan/asan_emscripten.cpp @@ -24,7 +24,8 @@ void InitializeShadowMemory() { // Assert that the shadow region is large enough. We don't want to start // running into the static data region which starts right after the shadow // region. - uptr max_address = __builtin_wasm_memory_size(0) * WASM_PAGE_SIZE; + uptr max_address = + (__builtin_wasm_memory_size(0) * uint64_t(WASM_PAGE_SIZE)) - 1; uptr max_shadow_address = MEM_TO_SHADOW(max_address); // TODO(sbc): In the growable memory case we should really be checking this // every time we grow. From 979bdbccdbf6eaa26e1f8d3486e530c4153f5eb9 Mon Sep 17 00:00:00 2001 From: Sam Clegg Date: Thu, 16 Dec 2021 15:07:37 -0800 Subject: [PATCH 3/3] typo --- emcc.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/emcc.py b/emcc.py index 19fd55ff06722..4ffe4b105d632 100755 --- a/emcc.py +++ b/emcc.py @@ -187,7 +187,7 @@ def base64_encode(b): return b64.decode('ascii') -def align_to_wasm_page_boudary(address): +def align_to_wasm_page_boundary(address): page_size = webassembly.WASM_PAGE_SIZE return ((address + (page_size - 1)) // page_size) * page_size @@ -2270,7 +2270,7 @@ def check_memory_setting(setting): total_mem = user_mem * 8 / 7 # But we might need to re-align to wasm page size - total_mem = int(align_to_wasm_page_boudary(total_mem)) + total_mem = int(align_to_wasm_page_boundary(total_mem)) # The shadow size is 1/8th the resulting rounded up size shadow_size = total_mem // 8 @@ -2282,7 +2282,7 @@ def check_memory_setting(setting): if not settings.ALLOW_MEMORY_GROWTH: settings.INITIAL_MEMORY = total_mem else: - settings.INITIAL_MEMORY += align_to_wasm_page_boudary(shadow_size) + settings.INITIAL_MEMORY += align_to_wasm_page_boundary(shadow_size) if settings.SAFE_HEAP: # SAFE_HEAP instruments ASan's shadow memory accesses.