BUG: Do fslio's size and offset arithmetic at 64-bit width - #128
Open
hjmjohnson wants to merge 2 commits into
Open
hjmjohnson wants to merge 2 commits into
hjmjohnson wants to merge 2 commits into
Conversation
A byte count or file offset is declared size_t, but the expression
assigned to it multiplies ints, so the arithmetic happens at 32 bits
and only the result is widened:
size_t FslGetVolSize(...)
{ return (nx * ny * nz); } /* int product */
offset = ((ydim * zVox + yVox) * xdim + xVox) * wordsize;
For a volume over 2GB the product overflows before it is ever assigned
to the size_t, so the destination type buys nothing. The signed
overflow is undefined, so whether the wide value survives is up to the
optimizer: clang folds it to the correct 64-bit result at -O2 and
yields 0 at -O0. Affects FslGetVolSize(), FslWriteVolumes(),
FslReadSliceSeries(), FslReadRowSeries() and FslReadTimeSeries().
Each operand is now widened before the multiply rather than after.
The slbytes and volbytes assignments in FslReadSliceSeries() and
FslReadTimeSeries() are left alone because another open pull request
changes those exact lines.
(cherry picked from commit aee824e)
The test drives FslGetVolSize() with dimensions whose product exceeds INT_MAX and asserts the exact 64-bit value, plus a small volume so an over-broad change cannot pass it. It fails in a -O0 build without the widening fix and passes with it; at -O2 clang folds the signed overflow to the wide result, so only the unoptimized build observes it. (cherry picked from commit 87b5264)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Re-submission of #73, reverted from
masteron 2026-09-24. Content isunchanged from the original.
Position 6 of 11 in the deep stack. Base:
stack/pr-fix-sign-conversion.Based on the pull request above it in the stack, so the diff shown here is
this change alone. Merge the stack bottom-up.
Stack order
stack/test-regression-coveragemasterstack/pr-fix-alloc-null-checksstack/test-regression-coveragestack/fix-axml-skip-depthstack/pr-fix-alloc-null-checksstack/pr-fix-analyzer-leaksstack/fix-axml-skip-depthstack/pr-fix-sign-conversionstack/pr-fix-analyzer-leaksstack/fix-fslio-64bit-arithmetic<- this PRstack/pr-fix-sign-conversionstack/fix-cifti-null-streamstack/fix-fslio-64bit-arithmeticstack/pr-fix-calloc-transposed-argsstack/fix-cifti-null-streamstack/pr-fix-shorten-64-to-32stack/pr-fix-calloc-transposed-argsstack/fix-image-read-complex-checkstack/pr-fix-shorten-64-to-32stack/pr-fix-xml-read-errorsstack/fix-image-read-complex-checkThe order is the order these changes sat on
masterbefore the revert, soit builds and tests at every step.
Commits introduced by this PR
Ordering for all the re-submitted work is tracked in #84.