BUG: Fix cifti_tool's CIFTI extension search, which never advanced - #56
Conversation
0b9e45a to
27b162f
Compare
|
Pushed a second commit.
Also worth noting for review: the |
20a3f5d to
27b162f
Compare
4ad5ca6 to
7dfd804
Compare
7dfd804 to
1c17d16
Compare
1c17d16 to
354843e
Compare
|
Rebased onto Fixture and testNew fixture
The existing Red proofWith 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 cleanupRemoved the AI Not coveredThe second half of the fix — closing the output stream on the "no CIFTI extension" early return — has no test. Reaching that branch needs a |
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.
354843e to
d74f89b
Compare
2405d09
into
InsightSoftwareConsortium:master
disp_cifti_extension() searched for the CIFTI extension with
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=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