ENH: code review of recent nifti_image_conflicted_names PR

[seanm]

No description provided.

[hjmjohnson]

@seanm I updated and fixed merge conflicts for this PR. Would you please review and update so that we could incorporate the removal of the "exit()" commands that could cause an application to crash from using this library?

[seanm]

lgtm.

What do you want me to update? Not sure I understand that part of your comment...

[hjmjohnson]

@seanm At a minimum, the commit message needs to be changed from WIP.

[seanm]

So you've already mutated my commits here, right? That means what's on my local disk is not the same. If I alter my commit message and push, I'll overwrite what you did I think. I'm not sure how to overcome that because my git-fu is weak...

[gdevenyi]

The exit() removal that @hjmjohnson asked for here is in #26, extended a bit.

This PR changes the two exit() calls in nifti2_io.c. nifti1_io.c has the same two and keeps them — its REJECT_COMPLEX branch still calls exit(13), only the message text changes. #26 does both libraries.

It also fixes the other defects in the same blocks, which are easy to miss because they sit on the lines either side of the exit():

free(basename);
char * gzname = (char *)calloc(strlen(hdrname) + 8, sizeof(char));
strcpy(gzname, hdrname);                        /* gzname unchecked */
...
    fprintf(stderr, "... basename (*.nii, *.nii.gz): %s\n",
            basename);                          /* freed above */
    exit(134);

basename is freed and then read by the fprintf that reports the error, the calloc result is used by strcpy/strcat without a NULL check, and hdrname leaks on that path. In nifti_image_read(), the REJECT_COMPLEX block tests nim->datatype before the if (nim == NULL) check immediately below it, so a header that failed to convert dereferences a null pointer instead of reporting the error; #26 moves the block after the check.

Behaviour change worth stating either way: callers that relied on the process dying now get NULL back, which is the point, but a caller that never checked will continue with a NULL pointer where it previously exited.

The doPigz/doPigz2 half of this PR is handled differently. Making them static removes two exported symbols from a PIGZ-enabled build, so #26 gives them file-local prototypes instead — same effect on -Wmissing-prototypes, no ABI change. Happy to be argued out of that if you would rather they were static.

@seanm — on your last comment about not knowing how to update the commit message after @hjmjohnson rebased your branch: git fetch origin pull/11/head && git checkout FETCH_HEAD && git commit --amend then force-push to your branch gets you there.

[hjmjohnson]

Superseded by #54, which merged as part of the same cleanup and makes the identical change in both copies of nifti_findhdrname.

Rebasing this branch onto current master leaves zero commits and an empty diff — the content is already in.

Why #54 was taken instead

Both replace exit() with return NULL on the ambiguous-filename path. #54 additionally releases everything that path still holds:

free(gzname); free(basename); free(hdrname);
return NULL;

where this branch frees only gzname, leaving basename and hdrname leaked on the way out. #54 also moved the free(basename) to after the fprintf that reads it, fixing a use-after-free that GCC reports as -Wuse-after-free under FSLSTYLE, and added a null check on the gzname allocation.

The exit() removal this PR proposed was the right call and is what prompted the wider pass; thank you for it.