ENH: Make the implicit sign conversions explicit - #53
hjmjohnson merged 3 commits into
Conversation
f2188ec to
a9d007d
Compare
a9d007d to
b42c589
Compare
d5c9224 to
b4876bf
Compare
b42c589 to
545dde9
Compare
|
Rebased onto current The claim that holds: the swap count
nifti_swap_Nbytes( (int)(ntot / nim->swapsize), nim->swapsize, dataptr );
The two claims that do not hold, now rewordedThe ANALYZE orientation byte. The message said
Both messages now describe these as tidying rather than as fixes. Worth getting right: a reviewer who checks the ANALYZE claim and finds it false will start doubting the 189 lines they were not going to check. What else changed in this update
Build and testRelease, |
4f7fa60 to
dcf1ce4
Compare
hjmjohnson
left a comment
hjmjohnson
left a comment
There was a problem hiding this comment.
Reviewed both axes at max effort: conformance to the project's own standards, and
whether the diff does what the PR says it does. The nifti_swap_Nbytes change earns
this PR on its own — removing (int)(ntot/swapsize) fixes a real truncation above 2 GiB.
Line comments below. One I would hold the merge for; the rest are tidying.
Two of the three "this is not cosmetic" claims do not hold
The ANALYZE orientation byte was not arriving sign-extended. The description says
"taking a signed char and widening it into an unsigned one, so any value above 127
arrived sign-extended." Reading through *(char *)&... yields signed char, and the
conversion to unsigned char is defined modulo 256 — so 200 stays 200. The change is
value-preserving, which is fine, but it is tidying rather than a fix. A later comment on
this PR already concedes this; the body of commit dcf1ce4 still makes the original
claim and should be corrected before merge, since the commit message outlives the PR.
The remaining wrap recovers the same value. int - size_t wraps in size_t and then
converts back to int, giving the same result on two's complement. Strictly the old
conversion was implementation-defined rather than undefined, so again: tidying.
The swap count is genuine. nifti_swap_Nbytes(size_t n, int size, void *ar) — the
(int) truncated a count above 2 GiB. That one is a real bug fix.
Worth correcting the claims rather than dropping the hunks: the changes are still
improvements, they are just not the defect fixes the text asserts.
What I checked and found clean
Both znzlib.c casts are value-preserving. remain -= (size_t)nread; is dominated by
if( nread < 0 ) return 0; two lines above, and nread <= n2read <= remain by
construction, so the cast cannot change the value and remain cannot underflow. Same for
nwritten. This is the only open PR touching znzlib.c, so I looked at it closely.
No whitespace churn: git diff -w reports the same 189 added lines as the plain diff.
The (size_t)nim->dim[c] and nx/ny/nz/nw widenings are correct, and the
invariants the commit messages cite do hold.
The nine deliberately-deferred warnings are declared and match what is left.
Commit bodies exceed the project's convention
All three run 28, 24 and 24 lines against a 12-line convention. The subjects are fine. The
reasoning is good material — it reads better in this PR body, where it is not carried in
git log forever.
A follow-up, out of scope here
(size_t)nx * (size_t)ny * (size_t)nz * (size_t)nbyper recurs in nifti1_io.c,
nifti2_io.c and fslio.c. A small nifti_voxel_count() helper would remove the
hand-widening at every site and give the invariant one home. Not for this PR.
…plicit 152 of the -Wsign-conversion findings are an int or int64_t count reaching a size_t parameter of malloc(), calloc(), realloc(), memcpy(), memset(), strncpy(), znzread(), znzwrite() or fread(). Where a count is multiplied by a sizeof, the cast goes on the count rather than around the product, so the multiply happens at 64-bit width: (int)count * sizeof(T) can overflow before it is widened. The counts cannot be negative. nifti_update_dims_from_array() clamps every dim[i] to at least 1, nifti_datatype_sizes() leaves nbyper at 0 for an unknown datatype, and nifti_image_load() requires nbyper > 0 and nvox > 0.
The findings that are not allocation, copy or I/O lengths: file offsets, extension bookkeeping, and the ANALYZE orientation byte. znztell() returns a signed znz_off_t stored in size_t locals, so the znzseek() arguments are cast to znz_off_t and the arithmetic happens at the width the function takes. fslio's nvox is widened per operand so the product of seven int dimensions is computed at 64 bits. XML_Parse() takes an int length while blen is unsigned; the conversion is now at the call. The ANALYZE orientation byte is read through unsigned char * rather than a signed lvalue. That is value-preserving either way -- signed to unsigned char is defined modulo 256 -- so it states the intent only.
One change is a fix. nifti_read_buffer() passed (int)(ntot / nim->swapsize) to nifti_swap_Nbytes(), which takes a size_t count that was widened in 2010 because it "might not fit as int". The cast reinstated that truncation, leaving most of an image above 2 GiB unswapped. The cast is gone. nifti_image_read() took the file size through (size_t), which turns nifti_get_filesize()'s -1 error return into SIZE_MAX and slips past the guard below it. A signed temporary is used instead, matching what nifti2_io.c already does at the same place. The rest casts an int dimension to size_t at the point of use.
dcf1ce4 to
3adf034
Compare
|
Force-pushed as The diff got smaller: 227 added / 242 deleted, down from 252 / 382, with zero What changed, by finding
The nifti1/nifti2 pair settled two findings
The same pairing caught an error in my own review. Verification, including downstream ITKLocally, Because ITK vendors this code, the series was also injected into an ITK checkout
|
2696f66
into
InsightSoftwareConsortium:master
2 participants
Clears
-Wsign-conversionacross the library. There is a lot of it, so it is split into three commits by what the conversion is for: allocation, copy and I/O lengths, then file offsets and counters, then the remainder.Most of it is casting an
intdimension tosize_tat the point of use so that a product is computed at 64 bits instead of overflowing at 32 and then being widened. Three of the changes are not cosmetic and are called out in the individual commit messages below: a stale(int)cast that reintroduced the very truncation a 2010 fix removed, a(unsigned int)cast on a value returned asint, and a subtraction done insize_twhose result is stored in anint.ENH: Make the integer conversions in allocations, copies and reads explicit
152 of the -Wsign-conversion findings are an int or int64_t count
reaching a size_t parameter of malloc(), calloc(), realloc(), memcpy(),
memset(), strncpy(), znzread(), znzwrite() or fread().
Where a count is multiplied by a sizeof, the cast goes on the count
rather than around the whole product, so the multiply happens at 64-bit
width. That matters beyond the warning: (int)count * sizeof(T) can
overflow before it is widened, whereas (size_t)count * sizeof(T) cannot.
These are safe, and it is worth saying why rather than asserting it.
nifti_update_dims_from_array() rejects dim[0] outside [1,7] and clamps
every dim[i] < 1 up to 1, so nx through nw are at least 1 by the time any
of this runs; nifti_datatype_sizes() leaves nbyper at 0 for an unknown
datatype and every caller checks for that; and nifti_image_load() refuses
to proceed unless nbyper > 0 and nvox > 0. The conversions cannot be
negative, so making them explicit documents an invariant the code already
enforces rather than hiding one it does not.
Lines that upstream PRs #11, #21, #22, #23 and #24 also change are left
untouched throughout.
ENH: Make the remaining integer conversions explicit
The -Wsign-conversion findings that are not allocation, copy or I/O
lengths: file offsets and positions, extension bookkeeping, and a few
conversions that were wrong rather than merely implicit.
File offsets. znztell() returns a signed znz_off_t stored in size_t
locals; the znzseek() arguments are cast to znz_off_t so the arithmetic
is done at the width the function takes.
fslio's nvox. The product of seven int dimensions assigned to a size_t
field, widened per operand so it is computed at 64 bits.
The ANALYZE orientation byte. nifti_convert_nhdr2nim() read it with
unsigned char c = *((char *)(&nhdr.qform_code));, taking a signedchar and widening it into an unsigned one, so any value above 127
arrived sign-extended. Reading through unsigned char * gives the byte
that is actually in the header.
expat's XML_Parse takes an int length while blen is unsigned; the
conversion is now at the call rather than hidden in the variable.
Lines that upstream PRs also change are left alone, including the
d3matrix and d4matrix allocations in fslio.c, which PR #22 rewrites.
ENH: Make the last implicit sign conversions explicit
This clears -Wsign-conversion for every file the open upstream PRs do
not touch. Most of it is casting an int dimension to size_t at the
point of use so a product is computed at 64 bits rather than
overflowing at 32 and then being widened.
Two of the changes are not cosmetic:
nifti_read_buffer() called
nifti_swap_Nbytes( (int)(ntot / nim->swapsize), ... )
although nifti_swap_Nbytes() takes a size_t count, and the comment
three lines above it says the count was widened in 2010 precisely
because it 'might not fit as int'. The cast put the truncation back:
swapping a data segment larger than 2 GiB passed a wrapped count and
left most of the image unswapped. The cast is gone.
nifti_get_filesize() returns int and documents -1 for error, but ended
with
return (unsigned int)buf.st_size;, converting through unsignedbefore narrowing. It now casts to the type it returns.
nifti_image_read() computed
remaining = iname_offset - sizeof(nhdr)in size_t and narrowed the result to the int it is stored in, so a
header offset below sizeof(nhdr) produced a huge unsigned value. The
subtraction is now done in int, which is the type of both the variable
and the value.
Left for their owners: the fslio allocations and volbytes in PR #22, the
znzwrite calls and nifti_tester001 in PR #24.
Three groups of sites are handled in sibling PRs of this set rather than here, so that each PR applies to
masteron its own: the modify_field() stores carry their casts inside #44's memcpy rewrite, FslSeekVolume()'s operands are widened in #52, and the rest of fslio's size arithmetic is #52's second commit. Where this PR and a sibling touch the same line -- the XML fread call, and the sbuf calloc that #50 reorders -- whichever merges second will need a one-line rebase.Interface impact: none. On the union of all these changes, configured with
USE_FSL_CODE=ONandUSE_CIFTI_CODE=ON: all 448 exported symbols acrosslibniftiio,libnifti2,libznz,libfslio,libnifticdfandlibciftiare identical tomasterundernm -D --defined-only, and all ten installed headers are identical undergcc -E -P. Undergcc -dM -Eone macro definition differs, intentionally and only in text: #61 makesFSL_RADIOLOGICALread(-1)so it is safe inside an expression. Its value is still-1, checked by compiling against each installedfslio.hand printing it.Verification. This branch: builds with gcc 16.1.1,
ctestunchanged frommaster(2 of 345 fail onmasteritself in this environment; #31 and #29 each fix one). The union of all the PRs: 0 errors under both gcc 16.1.1 and clang 22.1.8,ctest345/345 under each, and the whole suite under valgrind memcheck with--trace-children=yesgives 484 traced processes with no invalid access, no uninitialised value and no leak in any nifti binary.Coordination. Every line of every branch was compared, whitespace-normalised, against the diffs of the open PRs (#11, #21, #22, #23, #24). Where one of those already changes a line, the line was left alone, and the few deliberate overlaps are named in the text above. What survives is 17 compiler warnings, all of them on those lines: 9
-Wsign-conversion(5 infslio.cfor #22, 2 innifti2_io.cand 2 innifti_tester001.cfor #24) and 8-Wcalloc-transposed-argsinnifti_findhdrnameandnifti_findimgname, which #11 rewrites. No formatting changes appear anywhere, to stay clear of #10 and #12.One of a set of independent, single-purpose PRs. Each bases on
masterand can be merged on its own, in any order.The full set of PRs (35)
The union of all of them is on the fork as
all-changes, if you want to build and test the lot at once.CI and build
Configuration and documentation
Defects
Warning and check classes