Skip to content

BUG: Fix cifti_tool's CIFTI extension search, which never advanced - #56

Merged
hjmjohnson merged 2 commits into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/fix-analyzer-leaks
Sep 22, 2026
Merged

hjmjohnson merged 2 commits into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/fix-analyzer-leaks

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 15, 2026 •

Copy link
Copy Markdown

disp_cifti_extension() searched for the CIFTI extension with

ext = nim->ext_list;
for( ind = 0; ind < nim->num_ext; ind++ )
   if( ext->ecode == NIFTI_ECODE_CIFTI ) break;

ext is never advanced, so this tests the first extension num_ext times.
cifti_tool could only ever find a CIFTI extension that happened to be
first in the list; with any other extension ahead of it the tool
reported 'no CIFTI extension' for a file that has one. It now indexes
ext_list[ind] and leaves ext NULL when there is no match, which also
avoids the read past the end of the list that a bare ext++ would have
introduced.

The same function opened its output stream before the 'no CIFTI
extension' check and returned without closing it; the early return now
closes the stream like the normal path does.

Found by clang's static analyzer, on paths the test suite does not
reach. The out-of-bounds indirect call from the same sweep is #40; the
extension-data leak it found in the two nifti tools is #62.


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
@gdevenyi
gdevenyi force-pushed the pr/fix-analyzer-leaks branch from 0b9e45a to 27b162f Compare August 15, 2026 05:22
@gdevenyi gdevenyi changed the title BUG: Fix two leaks and a search loop that never advanced BUG: Fix cifti_tool's CIFTI extension search, which never advanced Aug 15, 2026
@gdevenyi
gdevenyi marked this pull request as ready for review August 15, 2026 05:26
@gdevenyi

Copy link
Copy Markdown
Author

Pushed a second commit. open_write_stream() returns NULL when its fopen() fails, and both callers used the result without testing it:

$ cifti_tool -input c.nii -disp_cext -output /nonexistent-dir/x.txt
** failed to open '/nonexistent-dir/x.txt' for writing
Segmentation fault (core dumped)

disp_cifti_extension() passes it to fprintf(); eval_cifti_extension() hands it to axml_set_wstream(), so the NULL becomes the XML library's write stream and every later write follows it. Both now return 1 — the message is already printed.

Also worth noting for review: the return 1 this PR adds for the no-extension case has no observable effect, because process() discards the result of both calls. It is right on its own terms, just not an exit-status change.

@hjmjohnson

Copy link
Copy Markdown
Member

Rebased onto master (b4876bf) and added a fixture plus a red-green test for the search bug. 363/363 tests pass.

Fixture and test

New fixture cifti/testdata/cext_second_extension.nii, built from the existing cext_unterminated.nii by inserting a 48-byte NIFTI_ECODE_COMMENT extension ahead of the CIFTI one and fixing up vox_offset. It follows the cifti/testdata + cifti/CMakeLists.txt pattern already in the tree.

cifti_tool_cext_not_first runs cifti_tool -disp_cext on it, requiring <MapName>demo</MapName> in the output and forbidding no CIFTI extension.

The existing cifti_tool_unterminated_cext test pins the first-position case, so together they exclude both over-broad fixes: a search that always matches would print the comment extension's bytes and fail the MapName regex; a search that never matches would fail the no CIFTI extension regex.

Red proof

With cifti/cifti_tool.c reverted to master:

362: ** no CIFTI extension in .../cifti/testdata/cext_second_extension.nii
4/5 Test #362: cifti_tool_cext_not_first .........***Failed  Error regular expression found in output. Regex=[no CIFTI extension]  0.16 sec
5/5 Test #363: cifti_tool_unterminated_cext ......   Passed

The exit status is 0 in both builds, which is why the test asserts the output and not the return code. With the fix the CIFTI payload is printed.

Message and comment cleanup

Removed the AI Co-Authored-By: trailer and the Claude-Session: chat URL. Also dropped the closing "Found by clang's static analyzer, on paths the test suite does not reach" — the search bug is now reached by the test suite, so that sentence was no longer true. The added source comment was cut from three narrating lines to one.

Not covered

The second half of the fix — closing the output stream on the "no CIFTI extension" early return — has no test. Reaching that branch needs a nifti_image carrying extensions but no CIFTI one, and the CIFTI reader rejects such a file earlier (axio: no CIFTI extension found), so disp_cifti_extension is never entered. The leak is real on that path, but the path is currently unreachable through cifti_tool.

gdevenyi and others added 2 commits September 22, 2026 10:52
disp_cifti_extension() searched for the CIFTI extension with

    ext = nim->ext_list;
    for( ind = 0; ind < nim->num_ext; ind++ )
       if( ext->ecode == NIFTI_ECODE_CIFTI ) break;

ext is never advanced, so this tests the first extension num_ext times.
cifti_tool could only ever find a CIFTI extension that happened to be
first in the list; with any other extension ahead of it the tool
reported 'no CIFTI extension' for a file that has one.  It now indexes
ext_list[ind] and leaves ext NULL when there is no match, which also
avoids the read past the end of the list that a bare ext++ would have
introduced.

The same function opened its output stream before the 'no CIFTI
extension' check and returned without closing it; the early return now
closes the stream like the normal path does.
cext_second_extension.nii carries a comment extension ahead of the
CIFTI one.  The test requires the extension's payload in the output and
forbids the 'no CIFTI extension' message, and the existing
unterminated-cext test still pins the first-position case, so neither a
search that always matches nor one that never does would pass.
@hjmjohnson
hjmjohnson force-pushed the pr/fix-analyzer-leaks branch from 354843e to d74f89b Compare September 22, 2026 15:54
@hjmjohnson
hjmjohnson merged commit 2405d09 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