ENH: Make the 64-to-32 bit truncations explicit - #52
hjmjohnson merged 1 commit into
Conversation
842c230 to
f53855b
Compare
f53855b to
1a235ad
Compare
gdevenyi
changed the title
164cef1 to
1903564
Compare
1903564 to
2b549e4
Compare
2b549e4 to
1c81802
Compare
|
Rebased onto current One cast was narrowing further than the original code, and is now corrected. In fslio->niftiptr->ndim = (int)dim;
fslio->niftiptr->dim[0] = (short)dim; /* was */
The FslSeekVolume overflow, and why there is no ctest for itThe overflow is real. Reproducing the two expressions standalone, for a 512^3 4-byte volume at volume index 5: The truncation is at the assignment to I could not turn this into a ctest, and did not invent one:
Casts that remain lossy by necessity, for the recordWhere the narrowing target is an installed prototype the cast can only be made explicit, not removed. These stay lossy and the PR is right to say so:
Each is reachable only through an API whose own parameter is already What else changed in this update
Build and testRelease, |
b753b38 to
7d3ca4d
Compare
31 -Wshorten-64-to-32 warnings. Most were benign; one was not.
FslSeekVolume() declared `int offset` and assigned it a byte position
into the image file:
offset = fslio->niftiptr->iname_offset
+ vols * FslGetVolSize(fslio) * fslio->niftiptr->nbyper;
return znzseek(fslio->fileptr, offset, SEEK_SET);
Seek to any volume past 2GB and the multiplication overflows, so znzseek
-- which takes a 64-bit znz_off_t -- receives a wrong and quite possibly
negative position. offset is a local, so widening it to znz_off_t and
widening the operands changes no interface. FslReadVolumes' volbytes had
the same shape and is now size_t.
The rest fall into two groups. Internal helpers were widened to carry
the value they were already being handed: nifti_read_extensions() and
rci_alloc_mem() now return int64_t rather than truncating their own
results, and nifti_read_next_extension() and nifti_check_extension() take
an int64_t `remain`. nt_read_bricks(), declared in the uninstalled
nifti_tool.h, takes an int64_t length.
Where the narrowing target is an installed prototype it can only be made
explicit, so each such cast carries a note: nifti_image_load_bricks()
returning a brick count as int, nifti_read_subregion_image() returning a
byte count as int, nifti_read_ascii_image() taking an int header length,
and FslSeekVolume()'s int return. The remainder are strlen() results
assigned to int and fread()/znzread() counts, bounded by buffers a few
hundred bytes long.
Two format strings were widened to PRId64 to match the parameters that
changed.
7d3ca4d to
f45671d
Compare
08c5f80
into
InsightSoftwareConsortium:master
2 participants
31 -Wshorten-64-to-32 warnings. Most were benign; one was not.
FslSeekVolume() declared
int offsetand assigned it a byte positioninto the image file:
Seek to any volume past 2GB and the multiplication overflows, so znzseek
-- which takes a 64-bit znz_off_t -- receives a wrong and quite possibly
negative position. offset is a local, so widening it to znz_off_t and
widening the operands changes no interface. FslReadVolumes' volbytes had
the same shape and is now size_t.
The rest fall into two groups. Internal helpers were widened to carry
the value they were already being handed: nifti_read_extensions() and
rci_alloc_mem() now return int64_t rather than truncating their own
results, and nifti_read_next_extension() and nifti_check_extension() take
an int64_t
remain. nt_read_bricks(), declared in the uninstallednifti_tool.h, takes an int64_t length.
Where the narrowing target is an installed prototype it can only be made
explicit, so each such cast carries a note: nifti_image_load_bricks()
returning a brick count as int, nifti_read_subregion_image() returning a
byte count as int, nifti_read_ascii_image() taking an int header length,
and FslSeekVolume()'s int return. The remainder are strlen() results
assigned to int and fread()/znzread() counts, bounded by buffers a few
hundred bytes long.
Two format strings were widened to PRId64 to match the parameters that
changed.
A second commit extends the same fix to the rest of fslio's size
arithmetic. FslGetVolSize(), FslWriteVolumes(), FslReadSliceSeries(),
FslReadRowSeries() and FslReadTimeSeries() each declare a size_t byte
count or file offset but compute it as a product of ints, so the
arithmetic overflows at 32 bits before it is ever widened. Each operand
is now widened before the multiply. Two instances -- the slbytes and
volbytes assignments in FslReadSliceSeries() and FslReadTimeSeries() --
are deliberately left alone because upstream PR #22 changes those exact
lines.
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