#56 BitsPerPixel may be left as uninitialized value when reading gif

v1.0_(example)
closed
nobody
None
1
2014-05-15
2014-04-17
No

In the DGifSetupDecompress function, Private->BitsPerPixel is initialized from CodeSize, which is read from the gif file using the READ macro. However, if the read fails for any reason, then CodeSize, and hence BitsPerPixel, will be left uninitialized, because the result of the READ macro is never checked.

This error was caught by running MemorySanitizer (https://code.google.com/p/memory-sanitizer/) on a unit test that attempts reading a gif file that has been truncated at various points.

The most obvious solution would be to change the following code in DGifSetupDecompress:

READ(GifFile, &CodeSize, 1);    /* Read Code size from file. */
BitsPerPixel = CodeSize;

to be instead:

if (READ(GifFile, &CodeSize, 1) < 1) {    /* Read Code size from file. */
    return GIF_ERROR;    /* Failed to read Code size. */
}
BitsPerPixel = CodeSize;

Of course, this means that DGifSetupDecompress is no longer guaranteed to always return GIF_OK, so it might be good to also change the following code in DGifGetImageDesc:

/* Reset decompress algorithm parameters. */
(void)DGifSetupDecompress(GifFile);

return GIF_OK;

to be instead:

/* Reset decompress algorithm parameters. */
return DGifSetupDecompress(GifFile);

Discussion

  • Eric S. Raymond

    Eric S. Raymond - 2014-05-14

    Thanks, I'll merge this change for the nrct release.

    Is there some way I could have that unit test to add to my test suite?

     
    • Matthew Steele

      Matthew Steele - 2014-05-14

      The original test was this one from mod_pagespeed: https://code.google.com/p/modpagespeed/source/browse/trunk/src/pagespeed/kernel/image/png_optimizer_test.cc#826

      However, the use of giflib there is hidden behind a couple layers of abstractions, so I've attached a more minimal C++ program that achieves the same goal.

      The basic idea of the test is to read in a gif file as a bytestring and then loop -- first try to decode the file with the last byte missing, then the last two, then the last three, and so on. There is a particular point in the file that tickles this bug when the file is truncated there; running the test under MemorySanitizer will terminate the program at this point with a use-of-uninitialized-value error.

       
  • Eric S. Raymond

    Eric S. Raymond - 2014-05-14
    • status: open --> accepted
     
  • Eric S. Raymond

    Eric S. Raymond - 2014-05-15
    • status: accepted --> closed
     

Log in to post a comment.