Corrupted nifti image loading if same filename is used with different extensions in the same folder

[lassoan]

Description

NIFTI images are loaded with corrupted image data if loading something.nii.gz and there is another file with the same filename with .nii extension.

See original bug report of a user at Slicer/Slicer#5378

Steps to Reproduce

• Place A.nii and A.nii.gz files in the same folder (any two different nifti files work, but here is an example data set: https://1drv.ms/u/s!Arm_AFxB9yqHxO1XF9OyB_PYU3khTQ?e=TasE3z)
• Load A.nii.gz -> loaded image is corrupted

Versions

ITK-5.1
(reproducible in various versions of Slicer, MITK, ITK-Snap, etc.)

Environment

Windows, Linux

Additional Information

It is just so frustrating that there are so many troubles with nifti images and people often keep using it (it only makes sense for neuroimaging but probably they should give up as well and move to a new, simpler and better format).

[dzenanz]

This is a problem with nifti library itself. Function nifti_findimgname ignores the actual given filename, and searches through a list of supported extensions:

ITK/Modules/ThirdParty/NIFTI/src/nifti/niftilib/nifti1_io.c

Lines 2846 to 2914 in 5e28b1d

	char * nifti_findimgname(const char* fname , int nifti_type)
	{
	/* store all extensions as strings, in case we need to go uppercase */
	char *basename, *imgname, elist[2][5] = { ".nii", ".img" };
	char extzip[4] = ".gz";
	char extnia[5] = ".nia";
	const char *ext;
	int first; /* first extension to use */
	/* check input file(s) for sanity */
	if( !nifti_validfilename(fname) ) return NULL;
	basename = nifti_makebasename(fname);
	imgname = (char *)calloc(sizeof(char),strlen(basename)+8);
	if( !imgname ){
	fprintf(stderr,"** nifti_findimgname: failed to alloc imgname\n");
	free(basename);
	return NULL;
	}
	/* if we are looking for uppercase, apply the fact now */
	ext = nifti_find_file_extension(fname);
	if( ext && is_uppercase(ext) ) {
	make_uppercase(elist[0]);
	make_uppercase(elist[1]);
	make_uppercase(extzip);
	make_uppercase(extnia);
	}
	/* only valid extension for ASCII type is .nia, handle first */
	if( nifti_type == NIFTI_FTYPE_ASCII ){
	strcpy(imgname,basename);
	strcat(imgname,extnia);
	if (nifti_fileexists(imgname)) { free(basename); return imgname; }
	} else {
	/**- test for .nii and .img (don't assume input type from image type) */
	/**- if nifti_type = 1, check for .nii first, else .img first */
	/* if we get 3 or more extensions, can make a loop here... */
	if (nifti_type == NIFTI_FTYPE_NIFTI1_1) first = 0; /* should match .nii */
	else first = 1; /* should match .img */
	strcpy(imgname,basename);
	strcat(imgname,elist[first]);
	if (nifti_fileexists(imgname)) { free(basename); return imgname; }
	#ifdef HAVE_ZLIB /* then also check for .gz */
	strcat(imgname,extzip);
	if (nifti_fileexists(imgname)) { free(basename); return imgname; }
	#endif
	/* failed to find image file with expected extension, try the other */
	strcpy(imgname,basename);
	strcat(imgname,elist[1-first]); /* can do this with only 2 choices */
	if (nifti_fileexists(imgname)) { free(basename); return imgname; }
	#ifdef HAVE_ZLIB /* then also check for .gz */
	strcat(imgname,extzip);
	if (nifti_fileexists(imgname)) { free(basename); return imgname; }
	#endif
	}
	/**- if nothing has been found, return NULL */
	free(basename);
	free(imgname);
	return NULL;
	}

[dzenanz]

It looks like it was not rectified upstream either. It would be good if @vanossj propagated the issue to nifti_clib.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

[seanm]

@hjmjohnson @lassoan these days, many OSes, like macOS in my case, sandboxes processes from the filesystem and allows them only to access files the user has granted permission to access. The way nifti_clib second guesses and tries different filename extensions and uppercase/lowercase changes is antithetical to that.

I'm thinking it might be a good idea to add a new API to nifti_clib to supplement this existing one (which ITK currently calls):

nifti_image *nifti_image_read( const char *hname , int read_data )

into:

nifti_image *nifti_image_read_new( const char *header_file_path , const char *image_file_path, int read_data )

In this new function we'd just not do any of that second guessing (trying different extensions, changing uppercase/lowercase) and just use the exact file paths given. For .hdr+.img you'd need to pass both, for .nii you'd only need to pass one.

Thoughts?

[lassoan]

I would find it more intuitive to add an optional data_file_path parameter to the existing nifti_image_read method.

If this new parameter can be passed to the reader, how would you determine the data file path?

The proper solution would be to store the relative path to the data file in the header file, as it is done in other file formats, from nrrd to obj. Could the NIFTI file format be changed to store the data file path? If not, then it is yet another reason to discourage everyone from using this file format.

[seanm]

I would find it more intuitive to add an optional data_file_path parameter to the existing nifti_image_read method.

That would be a backwards-incompatible change to a public API. Upstream is very conservative, and though I'm less so, I don't think that's a good idea either. Hence me suggesting a new API. It would allow the existing API to do what it's always done, and for anyone that doesn't like that to opt-in to new behaviour with new API.

If this new parameter can be passed to the reader, how would you determine the data file path?

In the case of sandboxed apps on macOS, the user must select both the .hdr and .img in the system file panel for the sandbox to allow access to said files. The file panel API then hands back the paths to both files.

In other cases, it could be the caller's responsibility to find the other half of .img/.hdr, then pass that to my proposed API.

Could the NIFTI file format be changed to store the data file path? If not, then it is yet another reason to discourage everyone from using this file format.

That's way out of scope. :)