BUG: Fix cdfbin's argument range check, which could never fire - #39
hjmjohnson merged 2 commits into
Conversation
52a0998 to
087fc0b
Compare
087fc0b to
2b2de0d
Compare
2b2de0d to
ef30e24
Compare
|
Rebased onto Message cleanupRemoved the AI Test and red proof
With the fix hunk reverted ( Restored: BuildRelease, Ninja, clang/macOS, with |
cdfbin() validates its `which` selector like this:
if(!(*which < 1 && *which > 4)) goto S30;
A value cannot be both less than 1 and greater than 4, so the condition
is always false, the negation is always true, and the jump to S30 is
always taken -- skipping the entire range check. gcc reports it as
-Wlogical-op, "logical 'and' of mutually exclusive tests is always
false".
The ten sibling functions in this file all spell the same guard with
`||`:
cdfbet 1527: if(!(*which < 1 || *which > 4)) goto S30;
cdfchi 2255: if(!(*which < 1 || *which > 3)) goto S30;
cdfchn 2553: if(!(*which < 1 || *which > 4)) goto S30;
... 7 more
so this is a single-character typo rather than an intentional deviation.
The effect is visible from the public API. cdfbin is declared in the
installed nifticdf.h, and its contract is that an out-of-range input sets
*status to -1 and *bound to the limit that was violated. Calling it with
which = 9:
before: status=999 bound=-999 (both left as the caller set them)
after: status=-1 bound=4 (the documented error return)
Before the fix the caller has no indication anything was wrong and the
function proceeds to compute with an unhandled selector.
The range check is reachable from the installed nifticdf.h, so assert the documented contract directly: which=9 and which=0 must set *status to -1 and *bound to the limit that was violated. A which=1 call is asserted to still return status 0, so a guard that rejected everything would not pass.
ef30e24 to
663d349
Compare
493a317
into
InsightSoftwareConsortium:master
cdfbin() validates its
whichselector like this:A value cannot be both less than 1 and greater than 4, so the condition
is always false, the negation is always true, and the jump to S30 is
always taken -- skipping the entire range check. gcc reports it as
-Wlogical-op, "logical 'and' of mutually exclusive tests is always
false".
The ten sibling functions in this file all spell the same guard with
||:so this is a single-character typo rather than an intentional deviation.
The effect is visible from the public API. cdfbin is declared in the
installed nifticdf.h, and its contract is that an out-of-range input sets
*status to -1 and *bound to the limit that was violated. Calling it with
which = 9:
Before the fix the caller has no indication anything was wrong and the
function proceeds to compute with an unhandled selector.
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