Skip to content

BUG: Free the header on nifti_tool's duplicate-file failure paths - #60

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/fix-tool-error-path-leaks
Sep 21, 2026
Merged

hjmjohnson merged 1 commit into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/fix-tool-error-path-leaks

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 15, 2026 •

Copy link
Copy Markdown

act_mod_hdrs(), act_mod_hdr2s() and act_swap_hdrs() -- five functions
across the two tools -- read a header, then, when -prefix is given,
duplicate the dataset before writing the modified header back. Each of
the three ways that duplication can fail returns without freeing the
header, and the last of them also loses the strdup'd duplicate name:

nhdr = nt_read_header(fname, &nver, &swap, 0, ...);
...
if( opts->prefix ) {
   nim = nt_image_read(opts, fname, 1, 1);
   if( !nim ) { fprintf(...); return 1; }              /* nhdr */
   if( nifti_set_filenames(nim, opts->prefix, 1, 1) ) {
      nifti_image_free(nim); return 1;                 /* nhdr */
   }
   dupname = nifti_strdup(nim->fname);
   if( nifti_image_write_status(nim) ) {
      nifti_image_free(nim); return 1;                 /* nhdr, dupname */
   }
}
...
free(dupname);
free(nhdr);

The normal path frees both. Reproduced by pointing -prefix at a
directory that cannot be written:

nifti_tool -mod_hdr -prefix <read-only dir>/anat1 -infiles anat0.nii \
           -mod_field qoffset_x -17.325

before: definitely lost: 348 bytes in 1 blocks
after: ERROR SUMMARY: 0 errors from 0 contexts

Separately, act_diff_nims() in both tools releases the first image with
free(nim0) when reading the second one fails. That is a shallow free: it
loses nim0's fname, iname and any data or extensions. It now calls
nifti_image_free() like the success path six lines below.

Found by running the test suite under valgrind. On the normal paths the
suite is clean -- 484 traced processes, no invalid access, no
uninitialised value and no leak in any nifti binary -- so these are
error-path defects that the tests do not otherwise reach.


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
act_mod_hdrs(), act_mod_hdr2s() and act_swap_hdrs() -- five functions
across the two tools -- read a header, then, when -prefix is given,
duplicate the dataset before writing the modified header back.  Each of
the three ways that duplication can fail returns without freeing the
header, and the last of them also loses the strdup'd duplicate name:

    nhdr = nt_read_header(fname, &nver, &swap, 0, ...);
    ...
    if( opts->prefix ) {
       nim = nt_image_read(opts, fname, 1, 1);
       if( !nim ) { fprintf(...); return 1; }              /* nhdr */
       if( nifti_set_filenames(nim, opts->prefix, 1, 1) ) {
          nifti_image_free(nim); return 1;                 /* nhdr */
       }
       dupname = nifti_strdup(nim->fname);
       if( nifti_image_write_status(nim) ) {
          nifti_image_free(nim); return 1;                 /* nhdr, dupname */
       }
    }
    ...
    free(dupname);
    free(nhdr);

The normal path frees both.  Reproduced by pointing -prefix at a
directory that cannot be written:

    nifti_tool -mod_hdr -prefix <read-only dir>/anat1 -infiles anat0.nii \
               -mod_field qoffset_x -17.325

  before:  definitely lost: 348 bytes in 1 blocks
  after:   ERROR SUMMARY: 0 errors from 0 contexts

Separately, act_diff_nims() in both tools releases the first image with
free(nim0) when reading the second one fails.  That is a shallow free: it
loses nim0's fname, iname and any data or extensions.  It now calls
nifti_image_free() like the success path six lines below.

Found by running the test suite under valgrind.  On the normal paths the
suite is clean -- 484 traced processes, no invalid access, no
uninitialised value and no leak in any nifti binary -- so these are
error-path defects that the tests do not otherwise reach.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KSPnbwpDjVcAYqDdVqLkMU
@hjmjohnson
hjmjohnson force-pushed the pr/fix-tool-error-path-leaks branch from 90c7a50 to 23b9d0d Compare September 21, 2026 23:29
@hjmjohnson
hjmjohnson merged commit 744e751 into InsightSoftwareConsortium:master Sep 21, 2026
17 of 21 checks passed
@hjmjohnson

Copy link
Copy Markdown
Member

The commit messages in this range were rewritten to remove trailers that do not belong in permanent history: Co-Authored-By: naming an AI tool, and Claude-Session: URLs that resolve for nobody. Only messages changed — the tree at the tip of master is byte-identical, and author, committer, and dates are preserved.

This PR's commit on the rewritten master:

  • 40009a92db BUG: Free the header on nifti_tool's duplicate-file failure paths

The SHA recorded above by GitHub is from the pre-rewrite history and no longer resolves.

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