#94 giflib 5 loves to fail to load images... a LOT (vs 4)

[The Rasterman]

so hmmm. where do i begin. giflib 4.x worked like a charm for us... and i have just fouind that giflib5 has major issues. our code has basically not changed over the past 2 years or so except to add some #if GIFLIB_MAJOR etc. stuff.

https://git.enlightenment.org/core/efl.git/tree/src/modules/evas/image_loaders/gif/evas_image_load_gif.c

as you can see it's just the DGifOpen and DGifCloseFile changes we dealt with. all of the reast is identical... and gitlib4 thumbs up. gilbi5 . bzzzt. only SOME gifs load. most of my old gif collections dont load.

we have slightly odd code in that we decode twice. first time we jusst look at headers to figure out what this file is, size, transparency etc. because we defer decoding of data until later (when actually needed). for the cases where someone queries the image for size and other things but never shows it sso it needs actual rendering.

so let me zoom in on evas_image_load_file_head_gif2() and this is where the problems begin. let's look at the do/while and the image record type now reports an error when we get the dessscription:

    if (rec == IMAGE_DESC_RECORD_TYPE)
      {
         int img_code;
         GifByteType *img;

         // get image desc
         if (DGifGetImageDesc(gif) == GIF_ERROR)
              LOADERR(EVAS_LOAD_ERROR_UNKNOWN_FORMAT);

on giflib4 - we are all good. this doesn't happen. but on giflib 5... we start getting GIF_ERROR on parsing the gif file and going through img records. the isssues (differences) begin here. all sorts of other parsing is broken later on and i suspect it's related. maybe we abuse libgif a bit (well ok - we did out own extension block handling for animated gifs so they worked with libgif4 etc.), but i don't think so, we're sticking to a pretty boring usage of the api, but we are trying to handle animated gifs too so we have to deal with proper record parsing, not just simplistic "get first image and display it".

so what changed? what broke? valgrind time:

==6574== Conditional jump or move depends on uninitialised value(s)
==6574==    at 0x19B6BCE6: DGifSetupDecompress (dgif_lib.c:767)
==6574==    by 0x19B6BCE6: DGifGetImageDesc (dgif_lib.c:431)
==6574==    by 0x199670EF: evas_image_load_file_head_gif2 (evas_image_load_gif.c:458)
==6574==    by 0x5DD468C: _evas_image_file_header (evas_image_load.c:219)
==6574==    by 0x5DD4CEF: evas_common_load_rgba_image_module_from_file (evas_image_load.c:330)
==6574==    by 0x5D5529B: _evas_cache_image_entry_new (evas_cache_image.c:281)
==6574==    by 0x5D55F5E: evas_cache_image_request (evas_cache_image.c:906)
==6574==    by 0x5D4E7E7: _evas_image_file_set (efl_canvas_image.c:71)
==6574==    by 0x4010C2: main (in /home/raster/C/showtime)
==6574==  Uninitialised value was created by a heap allocation
==6574==    at 0x4C2ABD0: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6574==    by 0x19B6B788: DGifOpen (dgif_lib.c:175)
==6574==    by 0x19966CF7: evas_image_load_file_head_gif2 (evas_image_load_gif.c:423)
==6574==    by 0x5DD468C: _evas_image_file_header (evas_image_load.c:219)
==6574==    by 0x5DD4CEF: evas_common_load_rgba_image_module_from_file (evas_image_load.c:330)
==6574==    by 0x5D5529B: _evas_cache_image_entry_new (evas_cache_image.c:281)
==6574==    by 0x5D55F5E: evas_cache_image_request (evas_cache_image.c:906)
==6574==    by 0x5D4E7E7: _evas_image_file_set (efl_canvas_image.c:71)
==6574==    by 0x4010C2: main (in /home/raster/C/showtime)

specifically:

0x0000000019b6bce6 in DGifSetupDecompress (GifFile=0x19412860) at dgif_lib.c:767
767     if (BitsPerPixel > 8 || Private->RunningBits > 32) {

here:

762     return GIF_ERROR;    /* Failed to read Code size. */
763     }
764     BitsPerPixel = CodeSize;
765
766     /* this can only happen on a severely malformed GIF */
767     if (BitsPerPixel > 8 || Private->RunningBits > 32) {
768     GifFile->Error = D_GIF_ERR_READ_FAILED; /* somewhat bogus error code */
769     return GIF_ERROR;    /* Failed to read Code size. */
770     }
771

so ... something is up with giflib setting up file opens...

specifically the memory allocated by:

    Private = (GifFilePrivateType *)malloc(sizeof(GifFilePrivateType));

is then not initialized and then is USED by libgif later on ... and this is causing issues. is the problem that libgif has failed to init this later on when it is ASSUMED it will be? what should be filling this in so this actually works... the gifs are not malformed in that they have worked for years and are even widely distributed ones. this gels with something i saw... sometimes things work after we load enouhg images - maybe this Private data now cintains valid info?

can i suggest you at least calloc this Private stuff to ensure it's a known state ... or something? e.g.:

--- lib/dgif_lib.c~ 2016-01-07 19:44:44.000000000 +0900
+++ lib/dgif_lib.c  2016-04-02 15:15:50.620852098 +0900
@@ -89,7 +89,7 @@
     GifFile->SavedImages = NULL;
     GifFile->SColorMap = NULL;


-    Private = (GifFilePrivateType *)malloc(sizeof(GifFilePrivateType));
+    Private = (GifFilePrivateType *)calloc(1, sizeof(GifFilePrivateType));
     if (Private == NULL) {
         if (Error != NULL)
        *Error = D_GIF_ERR_NOT_ENOUGH_MEM;
@@ -172,7 +172,7 @@
     GifFile->SavedImages = NULL;
     GifFile->SColorMap = NULL;


-    Private = (GifFilePrivateType *)malloc(sizeof(GifFilePrivateType));
+    Private = (GifFilePrivateType *)calloc(1, sizeof(GifFilePrivateType));
     if (!Private) {
         if (Error != NULL)
        *Error = D_GIF_ERR_NOT_ENOUGH_MEM;

because... once i do this.... VOILA. giflib works again. under valgrind it works because valgrind is ensuring the memory is 0'd when allocated and your tests may get lucky or other giflib users too, but i am happily getting garbage content from malloc causing Private->RunningBits to be junk, thus getting a GIF_ERROR return very often (sometimes not). probably because we really heavily massage the heap and shuffle stuff in and out and i am sure you're getting recycled memory for theat private blob...

you might want to double-check all your malloc()s and see if they are all fully initialized or not or move them to calloc()s to be sure. unless its a lot of data... it's not really a bad thing to do. :)

[Eric S. Raymond]

Yourur usage pattern must be pretty odd. If you look at lines 101 and 172 of the head version you'll see that the PrivateType storage gets zeroed there. I suggest you try upgrading to 5.1.3 and see if your problem goes away.

[The Rasterman]

Well 5.1.2 that is Shipped on my distro has no zeroing code.

line 101 is:

    _setmode(FileHandle, O_BINARY);    /* Make sure it is in binary mode. */

and 172 is

    GifFile->SavedImages = NULL;

there is a:

    /*@i1@*/memset(GifFile, '\0', sizeof(GifFileType));

and

    memset(GifFile, '\0', sizeof(GifFileType));

But that zeros the GifFile, not Private data. :) Looking at your curent cvs/vn/whatever it is... it also doesn't zero out Private struct. Please see my patch - it ensures Private is zero'ed out, not GifFile - that is already zero'ed by the memset(). (You could nuke the memset and just use calloc for that too - it'd be more efficient if fetching fresh RAM as the malloc implementation can avoid zeroing if it knows its freh and already zero).

So... You still have the big in your current head/master/trunk. No upgrade will help. :)

[Eric S. Raymond]

Oh shit. How did that make it past my static checkers?

New release coming as soon as I can run the validation tests and ship it.

Ironically, Ithink I got in this hole with changes intended to fend off bugs found by fuzzer attacks.

[The Rasterman]

i don't know what static checkers you use but coverity scan has been amazingly good. it's not open source, but it's a free service for oss projects. worth signing up if you haven't. its a very good tool.

but really... you should use valgrind. not tatus and you need to run actual test cases/code but it found ther actual sssource of this problem right off the bat. immediately pointed out that giflib accessed uninitialized memory and the exact line etc. and from there it was an easy chase back to the source of allocation to see nothing guarantees all of the mem is set to something sane.

just fyi - over the years i've made a habe of using calloc for "objects" and data structs. not for large blobs of memory (image data/pixels, video, audio), but for these little things. it really just is a good habit to have to ue it. it ensures you are sanely starting at zero without any effort. you really shsould consider jusst doing callocs everywhere you can... :) good habit to have. :) other great habits are to set ptrs to null after freeing them... just in case ;)

[Eric S. Raymond]

I use Coverity oin this code routinely. That's part of why I'm puzzled.

[Eric S. Raymond]

Fixed 5.1.4 has shipped.

[The Rasterman]

interesting that coverity didn't catch it! :) it's pretty good. either way - that's where good habits like calloc() come in. :)