BUG: Stop returning -1 from functions that return size_t - #43
hjmjohnson merged 2 commits into
Conversation
58d3c2d to
48b757a
Compare
48b757a to
bdbab41
Compare
bdbab41 to
aa39d73
Compare
|
Rebased onto Before reviewing the diff, please read the risk note below. This is a silent semantic change to three published library functions, which under this project's ABI/API policy is the most expensive category of change there is. The in-tree evidence says it is right, but the out-of-tree cost is real and cannot be measured from here. The risk, stated plainly
The bottom two rows are the hazard. A downstream consumer that wrote its check against the documented-by-observation In-tree blast radius, counted
No nifti2 code is touched: its equivalents return Test and red proof
With the That is the defect in the PR description reproduced exactly: the warning prints, the failure is ignored, and the caller gets an image whose last 1000 bytes are uninitialized heap. With the fix the same file is rejected and the test passes. Other changesRemoved the Build: What I would ask of the maintainerIf you want this to land safely rather than quietly, the alternative worth weighing is an additive one: leave the three |
znzread(), znzwrite() and nifti_read_buffer() all return size_t and
all returned -1 to report an error. In a size_t that value is
SIZE_MAX, larger than any length a caller can have asked for, so a
caller that tests the result with '<' reads the error as a complete
transfer:
ii = nifti_read_buffer(fp, nim->data, ntot, nim);
if( ii < ntot ){ ... } /* SIZE_MAX < ntot is false */
The visible effect is that a truncated image loads as if it were
whole: nifti_image_read() prints its short-read warning, ignores the
failure and returns an image whose tail is uninitialized heap.
FslReadVolumes() has the same problem one level up, dividing the
returned byte count by the volume size to report SIZE_MAX/volbytes
volumes read.
These now return 0, which every one of these callers already treats
as failure and which is the only value a size_t function has for
"nothing was transferred". The published return types in znzlib.h
and nifti1_io.h are unchanged.
The nifti2 equivalents return int64_t, where -1 is representable,
and are left alone.
Writes a 31x31x31 float image, copies it 1000 bytes short, and asserts nifti_image_read() returns NULL for the copy and a populated image for the original, so a blanket refusal cannot pass. Without the fix the truncated file comes back accepted with nvox=29791 and an uninitialized tail.
aa39d73 to
d4bd5a4
Compare
|
Rebased onto current Maintainer decision recorded: external consumers are not assumed to depend on the current behavior, so this is cleared to merge. The change, stated preciselyThree
NIFTI-2's The effect on a caller depends on how it tests the result:
Blast radius, measured rather than arguedDownstream native callers: none. Searched the four major vendors for direct calls to the three functions:
In-tree: 37 call sites classified, 0 broken.
The change repairs a latent defect rather than risking one. At ss = nifti_write_buffer(fp, NBL->bricks[bnum], NBL->bsize);
if( ss < NBL->bsize ){ /* SIZE_MAX < bsize is false: never fired */Under the old return that test could not fire on error. It fires correctly now. The read side has the same shape.
Why the old sentinel was not a usable contract
Detecting it required either casting to a signed type first or comparing against Test and red proof
Reverting only the short-read return in |
9b31abf
into
InsightSoftwareConsortium:master
2 participants
znzread(), znzwrite() and nifti_read_buffer() all return size_t and all
returned -1 to report an error. In a size_t that value is SIZE_MAX,
which is larger than any length a caller can have asked for, so every
caller that tests the result with '<' reads the error as a complete
transfer:
The visible effect is that a truncated image loads as if it were whole.
Reading a NIfTI file 1000 bytes short of its declared data size, on
master:
nifti_image_read() prints the warning, ignores the failure and hands
back an image whose last 1000 bytes are uninitialised heap. With this
change the same file gives:
FslReadVolumes() had the same problem one level up: it divides the
returned byte count by the volume size, so a failed read was reported to
its caller as SIZE_MAX/volbytes volumes successfully read.
The fix is to return 0, which is the value every one of these callers
already treats as failure, and the only value a size_t function has for
'nothing was transferred'. The return types are published in
znzlib.h and nifti1_io.h and are unchanged.
The equivalent nifti2 functions return int64_t, where -1 is
representable, and are left alone.
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