Skip to content

BUG: Fix cdfbin's argument range check, which could never fire - #39

Merged
hjmjohnson merged 2 commits into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/fix-cdfbin-range-check
Sep 22, 2026
Merged

hjmjohnson merged 2 commits into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/fix-cdfbin-range-check

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 15, 2026 •

Copy link
Copy Markdown

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.


Interface impact: none. On the union of all these changes, configured with USE_FSL_CODE=ON and USE_CIFTI_CODE=ON: all 448 exported symbols across libniftiio, libnifti2, libznz, libfslio, libnifticdf and libcifti are identical to master under nm -D --defined-only, and all ten installed headers are identical under gcc -E -P. Under gcc -dM -E one macro definition differs, intentionally and only in text: #61 makes FSL_RADIOLOGICAL read (-1) so it is safe inside an expression. Its value is still -1, checked by compiling against each installed fslio.h and printing it.

Verification. This branch: builds with gcc 16.1.1, ctest unchanged from master (2 of 345 fail on master itself 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, ctest 345/345 under each, and the whole suite under valgrind memcheck with --trace-children=yes gives 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 in fslio.c for #22, 2 in nifti2_io.c and 2 in nifti_tester001.c for #24) and 8 -Wcalloc-transposed-args in nifti_findhdrname and nifti_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 master and 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

This was referenced Aug 15, 2026
@hjmjohnson

Copy link
Copy Markdown
Member

Rebased onto master (b4876bf) and added a red-green regression test. 363/363 tests pass.

Message cleanup

Removed the AI Co-Authored-By: trailer and the Claude-Session: chat URL. Author, committer, dates, and tree are unchanged.

Test and red proof

nifticdf/nifticdf_range_test.c, registered as ctest nifticdf_range_test. cdfbin is declared in the installed nifticdf.h, so the documented contract can be asserted directly: which = 9 must give status = -1, bound = 4, and which = 0 must give status = -1, bound = 1. A which = 1 call is asserted to still return status = 0, so a guard that rejected everything would not pass either.

With the fix hunk reverted (|| back to &&):

43: ** cdfbin(which=9): status=999 bound=-999, expected status=-1 bound=4
43: ** cdfbin(which=0): status=999 bound=-999, expected status=-1 bound=1
43: ** 2 failure(s)
1/1 Test #43: nifticdf_range_test ..............***Failed    0.21 sec

Restored:

43: cdfbin(which=9): status=-1 bound=4
43: cdfbin(which=0): status=-1 bound=1
43: cdfbin(which=1): status=0 p=0.5
43: nifticdf range test passed
Build

Release, Ninja, clang/macOS, with -DNIFTI_BUILD_APPLICATIONS=ON -DUSE_NIFTI2_CODE=ON -DUSE_CIFTI_CODE=ON -DUSE_FSL_CODE=ON. 363/363 pass.

gdevenyi and others added 2 commits September 22, 2026 10:51
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.
@hjmjohnson
hjmjohnson force-pushed the pr/fix-cdfbin-range-check branch from ef30e24 to 663d349 Compare September 22, 2026 15:53
@hjmjohnson
hjmjohnson merged commit 493a317 into InsightSoftwareConsortium:master Sep 22, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants