Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

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

v1.0_(example)
closed
nobody
None
1
2014-05-15
2014-04-17
Matthew Steele
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

  • 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.

       
      Attachments
    • status: open --> accepted
     
    • status: accepted --> closed