Skip to content

Fix potential overflow in memory size calculation #4549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kylo5aby
Copy link
Contributor

@kylo5aby kylo5aby commented Aug 8, 2025

After resizing memory size, num_bytes_per_page might be greater than DEFAULT_NUM_BYTES_PER_PAGE,
hence check for integer overflow before num_bytes_per_page += heap_size.

Here is the detail error log:

wasm-micro-runtime/core/iwasm/interpreter/wasm_runtime.c:339:32: runtime error: unsigned integer overflow: 4294836224 + 16777216 cannot be represented in type 'unsigned int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/zhenwei/project/wasm-micro-runtime/core/iwasm/interpreter/wasm_runtime.c:339:32

Copy link
Collaborator

@TianlongLiang TianlongLiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to make the same modification for aot_runtime.c too. And if this overflow can happen only when the SHRUNK_MEMORY is enabled, maybe consider to add the macro around the condition.

@kylo5aby
Copy link
Contributor Author

kylo5aby commented Aug 8, 2025

Need to make the same modification for aot_runtime.c too. And if this overflow can happen only when the SHRUNK_MEMORY is enabled, maybe consider to add the macro around the condition.

resolved. refers to:https://github.com/bytecodealliance/wasm-micro-runtime/blob/6b51c61f5e50573f672916a7df953a2704b823f4/core/iwasm/interpreter/wasm_loader.c#L6559C1-L6560C61. this check happened when MULTI_MODULE is disabled or SHRUNK_MEMORY enabled

@lum1n0us
Copy link
Collaborator

UndefinedBehaviorSanitizer: undefined-behavior: unsigned integer overflow: 4294836224 + 16777216 cannot be represented in type 'unsigned int'

IIUC, this reveals a more common question: which is better, pre-additional-check or post-additional-check to fix a potential integer overflow? A pre-additional-check involves using a check to prevent integer overflow from the very beginning. A post-additional-check involves using a check after addition to see if there is an overflow. Clearly, UBSan strongly prefers the pre-additional-check, and I believe GCC compilation warnings do the same.

The issue is that post-additional-check is widely used in source code. Should we conduct a comprehensive check to replace post-additional-check with pre-additional-check? Should this approach be extended to all four integer arithmetic operations?

Please let me know if I am overthinking this.

@yamt @loganek @TianlongLiang

@kylo5aby
Copy link
Contributor Author

for C/C++ spec, unsigned integer overflow is well defined, and overflow of signed integers invokes undefined behavior. so the above code will perform well in GCC compiler, So overflow of signed integers pre-additional-check should be better, and for overflow of unsigned integers, UBSan suggests it's not the intended value.

@lum1n0us
Copy link
Collaborator

FYI: from https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html

-fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where the result of an unsigned integer computation cannot be represented in its type. Unlike signed integer overflow, this is not undefined behavior, but it is often unintentional. This sanitizer does not check for lossy implicit conversions performed before such a computation

@lum1n0us lum1n0us added the bug-fix Determine if this PR addresses a bug. It will be used by scripts to classify PRs. label Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix Determine if this PR addresses a bug. It will be used by scripts to classify PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants