Skip to content

Image data allocation size comes from the header with no cross-check against the file #71

Description

@gdevenyi

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions