#24 memleak in DGifGetImageDesc()

closed
nobody
None
5
2014-08-27
2007-12-20
Antony Dovgal
No

This part of the code is already marked with FIXME, so this is just to confirm it:

    /*** FIXME: Why do we check both of these in order to do this?
     * Why do we have both Image and SavedImages? */
    if (GifFile->Image.ColorMap && GifFile->SavedImages == NULL)
        FreeMapObject(GifFile->Image.ColorMap);

    GifFile->Image.ColorMap = MakeMapObject(1 << BitsPerPixel, NULL);

Of course, it's wrong to free GifFile->Image.ColorMap only when GifFile->SavedImages is NULL.
This causes a memleak, 'cause GifFile->Image.ColorMap is re-created on the very next line.

Removing the check for NULL fixes the leak (and doesn't seem to cause any problems according to my tests).

Discussion

  • Logged In: YES
    user_id=944205
    Originator: NO

    Thanks for the report. The following patch has been committed to cvs::

    diff -u -u -r1.5 dgif_lib.c
    --- dgif_lib.c 10 Nov 2007 22:56:19 -0000 1.5
    +++ dgif_lib.c 21 Jan 2008 22:36:12 -0000
    @@ -350,13 +350,14 @@
    }
    BitsPerPixel = (Buf[0] & 0x07) + 1;
    GifFile->Image.Interlace = (Buf[0] & 0x40);
    - if (Buf[0] & 0x80) { / Does this image have local color map? /
    -
    - /** FIXME: Why do we check both of these in order to do this?
    - * Why do we have both Image and SavedImages?
    /
    - if (GifFile->Image.ColorMap && GifFile->SavedImages == NULL)
    - FreeMapObject(GifFile->Image.ColorMap);

    • / Setup the colormap /
    • if (GifFile->Image.ColorMap) {
    • FreeMapObject(GifFile->Image.ColorMap);
    • GifFile->Image.ColorMap = NULL;
    • }
    • / Does this image have local color map? /
    • if (Buf[0] & 0x80) {
      GifFile->Image.ColorMap = MakeMapObject(1 << BitsPerPixel, NULL);
      if (GifFile->Image.ColorMap == NULL) {
      _GifError = D_GIF_ERR_NOT_ENOUGH_MEM;
      @@ -375,9 +376,6 @@
      GifFile->Image.ColorMap->Colors[i].Green = Buf[1];
      GifFile->Image.ColorMap->Colors[i].Blue = Buf[2];
      }
    • } else if (GifFile->Image.ColorMap) {
    • FreeMapObject(GifFile->Image.ColorMap);
    • GifFile->Image.ColorMap = NULL;
      }

      if (GifFile->SavedImages) {