-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Avoid potential integer overflow in Mmap.mmap #41186
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
Conversation
There is a potential integer overflow in Mmap.mmap which can lead to an out-of-bounds access. The size of the memory-mapped array `len` is calculated as `prod(dims)`. If this multiplication overflows, the allocated size will be too small and accesses towards the end of the array will fail with e.g. a segfault or other errors. I noticed this when using `dims` taken from a binary file header in UInt32 format.
stdlib/Mmap/src/Mmap.jl
Outdated
| isbitstype(T) || throw(ArgumentError("unable to mmap $T; must satisfy isbitstype(T) == true")) | ||
|
|
||
| len = prod(dims) * sizeof(T) | ||
| len = prod(dims .|> big) * sizeof(T) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Use overflow-aware multiplication to determine the size of the mmapped array and throw an error in case of overflow. Add a test case.
stdlib/Mmap/src/Mmap.jl
Outdated
| len = sizeof(T) | ||
| for l in dims | ||
| len, overflow = Base.Checked.mul_with_overflow(promote(len, l)...) | ||
| overflow && throw(ArgumentError("requested size prod$((sizeof(T), dims...)) too large, would overflow typeof(size(T)) == $(typeof(len))")) |
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.
This check could be moved outside of the loop, since we only care whether any overflow occured at all. Very minor though, since length(dims) can have a maximum of 64 elements before it is guaranteed to overflow for UInt64, so probably wouldn't change much runtime wise and should be fine as is.
I think you meant to write prod($((sizeof(T), dims...))) (notice the extra pair of parentheses to signify the method call).
Other than that, LGTM!
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.
I had the check outside the loop at first, but this is not guaranteed to work. If the overflow does not occur on the last iteration of the loop, the last one may succeed and overflow == false at the end of the loop. Therefore I think that the check has to be done after every call to mul_with_overflow or overflow has to be accumulated using logical or.
I will push a fixed error message. It will then look like this:
ArgumentError: requested size prod((1, 9223372036854771710, 2)) too large, would overflow typeof(size(T)) == Int64
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.
I was thinking of unconditionally carrying the overflow with |= through the loop:
len = sizeof(T)
overflow = false
for l in dims
len, of = Base.Checked.mul_with_overflow(...)
overflow |= of
end
overflow && throw(...)
but as I said, I don't think it matters too much.
Yes, perfect - prod can't be splatted into after all, it takes collections.
`prod()` takes a collection, not multiple arguments.
There is a potential integer overflow in Mmap.mmap which can lead to an out-of-bounds access. The size of the memory-mapped array
lenis calculated asprod(dims). If this multiplication overflows, the allocated size will be too small and accesses towards the end of the array will fail with e.g. a segfault or other errors. I noticed this when usingdimstaken from a binary file header in UInt32 format. Using BigInt sizes will avoid this and will also allow the maximum size check below to work as intended.