BUG: Fix the ambiguous-filename path in nifti_findhdrname - #54
Conversation
12af00b to
ef05b83
Compare
gdevenyi
changed the title
ef05b83 to
df1cc2b
Compare
Under FSLSTYLE the block that reports "Multiple possible filenames" has
three defects, and they cannot be separated.
free(basename); <- freed here
char *gzname = calloc(...); <- unchecked
strcpy(gzname, hdrname); <- used at once
...
fprintf(stderr,"... %s\n", basename); <- read after free
exit(134); <- kills the caller
basename is read by the message, so it cannot be freed at the top.
gzname goes straight into strcpy(), so it has to be checked. And once
the function returns NULL instead of calling exit(), every path out has
to release what it holds, which is what ties the three together: moving
the free of basename decides what the error paths must free, and there
are no error paths to speak of until exit() is gone.
FSLSTYLE is off by default but is a supported configuration:
-DFSLSTYLE:BOOL=ON turns it on, and nifti2/Makefile defines it always.
Built and tested with -DFSLSTYLE=ON.
The nifti_image_read() half of the original change is now NIFTI-Imaging#74.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
df1cc2b to
fe115f2
Compare
|
Rebased onto current
What changed and why each part is needed
Returning instead of exiting means every path out of the function now has to release what it holds, which is why the diff also frees |
e139858
into
InsightSoftwareConsortium:master
|
@hjmjohnson I see you've had a burst of activity here, nice! But instead of merging straight after your review, could you instead add your approval to the PR then give a few days for others (me mostly, I guess) to add a second review. I've been on vacation for several weeks but finally have time this week to start looking at these MRs. Especially since they are bot created, I think a double human review would be wise. |
|
The commit messages in this range were rewritten to remove trailers that do not belong in permanent history: This PR's commit on the rewritten
The SHA recorded above by GitHub is from the pre-rewrite history and no longer resolves. |
3 participants
A library must not terminate the host application. niftilib and nifti2
each have two exit() calls, both inside optional FSL build modes, and
both sit next to other defects in the same few lines.
nifti_findhdrname(), under -DFSLSTYLE:
error, so the diagnostic path is a use-after-free;
The free now happens after the last use, the allocation is checked, both
buffers are released and the function returns NULL like every other
failure path in it.
nifti_image_read(), under -DREJECT_COMPLEX, tested nim->datatype
before the 'if( nim == NULL )' check three lines below it, so a header
that failed to convert dereferenced a null pointer instead of reporting
the error. The block now follows the NULL check, releases nim, the file
handle and hfile, and returns NULL rather than exit(13).
Overlap with #11: that PR makes the same exit(134) -> return NULL change
in nifti_findhdrname, and rewords the REJECT_COMPLEX message, which this
adopts verbatim. It does not fix the use-after-free, the unchecked
calloc, the leak or the null dereference. If #11 merges first I will
rebase; the overlap is two lines.
Stated plainly: callers that relied on the process dying now get NULL
back. That is the intent, and it is what the maintainer asked for in
#11, but a caller that never checked the return value will continue with
a NULL pointer where it previously exited. Both paths are behind
non-default build options.
fsliolib is deliberately untouched. Its FSLIOERR macro also exits, and
de-fatalising it changes the contract at roughly fifty call sites, which
belongs in its own change.
Verified with the default configuration and with each of
FSLSTYLE_NAME_CONFLICTS=ON and FSLSTYLE_REJECT_COMPLEX=ON: all build
clean and none change the test result.
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