Adjust initial memory to handle asan shadow size #14631
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Without this change any user of ALLOW_MEMORY_GROWTH with asan would get a rather cryptic error message from the linker: wasm-ld: error: initial memory too small, 278278624 bytes needed This is because asan increases the memory requirements in proportion to the max memory, and the max memory defaults to 2Gb with `ALLOW_MEMORY_GROWTH` enabled. Also, remove check for `MAXIMUM_MEMORY` == -1 which is no longer a thing since #14088. Fixes: #14621
sbc100
added a commit
that referenced
this pull request
Dec 16, 2021
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
sbc100
added a commit
that referenced
this pull request
Dec 16, 2021
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
sbc100
added a commit
that referenced
this pull request
Dec 16, 2021
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
sbc100
added a commit
that referenced
this pull request
Dec 17, 2021
This bug was caused by miscalculating the adjustments made to TOTAL_MEMORY and GLOBAL_BASE and using ASan. The bug was introduced in #14631 and causes 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
mmarczell-graphisoft
pushed a commit
to GRAPHISOFT/emscripten
that referenced
this pull request
Jan 5, 2022
…ten-core#15806) This bug was caused by miscalculating the adjustments made to TOTAL_MEMORY and GLOBAL_BASE and using ASan. The bug was introduced in emscripten-core#14631 and causes 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: emscripten-core#15762
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Without this change any user of ALLOW_MEMORY_GROWTH
with asan would get a rather cryptic error message from
the linker:
wasm-ld: error: initial memory too small, 278278624 bytes needed
This is because asan increases the memory requirements in
proportion to the max memory, and the max memory defaults
to 2Gb with
ALLOW_MEMORY_GROWTHenabled.Also, remove check for
MAXIMUM_MEMORY== -1 which is no longera thing since #14088.
Fixes: #14621