What happens
nifti_image_load() takes the size of the image entirely from the header:
/* nifti2_io.c:6797, and nifti1_io.c:4967 */
ntot = nifti_get_volsize(nim);
...
nim->data = calloc(1,ntot) ; /* create image memory */
Nothing compares ntot with the size of the file the data is supposed to
come from, so a small file can ask for an arbitrarily large allocation.
Reproducing
A 352 byte .nii whose header says dim = 3 2000 2000 2000, datatype = 16 (float32):
$ ls -l huge.nii
-rw-r--r-- 1 gdevenyi gdevenyi 352 huge.nii
$ ./drv_load huge.nii # nifti_image_read(fname, 1)
++ WARNING: nifti_read_buffer(huge.nii):
data bytes needed = 32000000000
data bytes input = 0
number missing = 32000000000 (set to 0)
32 GB is requested for a 352 byte file. Under Linux overcommit the calloc
succeeds, the pages are never touched, resident memory stays at 12 MB, and
the read then fails cleanly and returns NULL, so on a normal desktop this
costs address space and not much else. Where it does bite:
- a system with overcommit disabled, or a cgroup or
ulimit -v, takes the
allocation failure instead — handled, but the whole read fails on a file
that could have been rejected from its header;
- 32-bit builds;
- callers that use
nifti_read_header() and nifti_get_volsize() to size
their own buffers.
AddressSanitizer reports the same thing as allocation-size-too-big when
the dimensions are large enough, which is how it first showed up.
Why an issue and not a pull request
The obvious check -- refuse when nifti_get_volsize(nim) exceeds what is
left in the file -- is wrong for at least three cases this library
supports:
.hdr / .img pairs, where the data is in a different file;
.nii.gz, where the uncompressed data is legitimately larger than the
file;
- a caller that reads the header, allocates its own buffer and fills it.
So the check belongs only on the uncompressed single-file path, and it
needs a decision about whether a short file is an error or a warning with
zero fill, which is what nifti_read_buffer() already does today. That is
a behaviour change, so it seems better raised than assumed.
Small related wart
The failure message truncates the size it reports:
/* nifti2_io.c:6806, nifti1_io.c:4976 */
fprintf(stderr,"** NIFTI: failed to alloc %d bytes for image data\n", (int)ntot);
With ulimit -v set, the example above prints
** NIFTI: failed to alloc 1935228928 bytes for image data
for a 32000000000 byte request. %" PRId64 " and no cast would say what
was actually asked for. Happy to send that one as its own pull request if
you want it separated from the rest.
Found while fuzzing nifti_image_read() with clang's libFuzzer under
AddressSanitizer, alongside the defects in #63 to #69.
What happens
nifti_image_load()takes the size of the image entirely from the header:Nothing compares
ntotwith the size of the file the data is supposed tocome from, so a small file can ask for an arbitrarily large allocation.
Reproducing
A 352 byte
.niiwhose header saysdim = 3 2000 2000 2000,datatype = 16(float32):32 GB is requested for a 352 byte file. Under Linux overcommit the
callocsucceeds, the pages are never touched, resident memory stays at 12 MB, and
the read then fails cleanly and returns NULL, so on a normal desktop this
costs address space and not much else. Where it does bite:
ulimit -v, takes theallocation failure instead — handled, but the whole read fails on a file
that could have been rejected from its header;
nifti_read_header()andnifti_get_volsize()to sizetheir own buffers.
AddressSanitizer reports the same thing as
allocation-size-too-bigwhenthe dimensions are large enough, which is how it first showed up.
Why an issue and not a pull request
The obvious check -- refuse when
nifti_get_volsize(nim)exceeds what isleft in the file -- is wrong for at least three cases this library
supports:
.hdr/.imgpairs, where the data is in a different file;.nii.gz, where the uncompressed data is legitimately larger than thefile;
So the check belongs only on the uncompressed single-file path, and it
needs a decision about whether a short file is an error or a warning with
zero fill, which is what
nifti_read_buffer()already does today. That isa behaviour change, so it seems better raised than assumed.
Small related wart
The failure message truncates the size it reports:
With
ulimit -vset, the example above printsfor a 32000000000 byte request.
%" PRId64 "and no cast would say whatwas actually asked for. Happy to send that one as its own pull request if
you want it separated from the rest.
Found while fuzzing
nifti_image_read()with clang's libFuzzer underAddressSanitizer, alongside the defects in #63 to #69.