-
Notifications
You must be signed in to change notification settings - Fork 745
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
Memory flattening: Check for overflow #6233
Conversation
src/support/safe_math.h
Outdated
#if !__has_builtin(__builtin_add_overflow) | ||
template<typename T> bool __builtin_add_overflow(T a, T b, T* output) { | ||
// Atm this only supports unsigned types. | ||
static_assert(std::is_unsigned_v<T>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this support only unsigned types and what do you mean by LLVM-isms? If LLVM's version supports both, is there a reason why we wouldn't use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By LLVM-isms I mean the code uses other LLVM headers and various llvm::
stuff so it isn't easy to extract the entire header. We could extract just the one function, I suppose, but it uses LLVM naming conventions (AddOverflow
).
Detecting signed overflow is harder as it can't just be a+b > a
as with unsigned (since the added value can be negative). I think it's just another comparison (looks like that's what LLVM does) but I didn't look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks!
Fixes a fuzz testcase for wasm-ctor-eval. Add the beginnings of a polyfill for stdckdint.h to help that.
Fixes a fuzz testcase for wasm-ctor-eval.
Add the beginnings of a header for overflow detection. I took a quick look at LLVM's
after writing this and it has many LLVM-isms so it doesn't seem worth trying to
adapt theirs.