added some tests to check how we handle variously damaged GIFs

added code to release the memory allocated for the returned colour map
correctly

diff --git a/gif.c b/gif.c
index 27bf1b8a8f307a7eadee642f335fe5d04ca8a53b..cc0149cd7c6c583e6aabf987fe5cdad6c716120d 100644 (file)
--- a/gif.c
+++ b/gif.c
@@ -163,6 +163,13 @@ i_readgif_low(GifFileType *GifFile, int **colour_table, int *colours) {
 
   mm_log((1,"i_readgif_low(GifFile %p, colour_table %p, colours %p)\n", GifFile, colour_table, colours));
 
+  /* it's possible that the caller has called us with *colour_table being
+     non-NULL, but we check that to see if we need to free an allocated
+     colour table on error.
+  */
+  if (colour_table)
+    *colour_table = NULL;
+
   BackGround = GifFile->SBackGroundColor;
   ColorMap = (GifFile->Image.ColorMap ? GifFile->Image.ColorMap : GifFile->SColorMap);
 
@@ -187,7 +194,10 @@ i_readgif_low(GifFileType *GifFile, int **colour_table, int *colours) {
     if (DGifGetRecordType(GifFile, &RecordType) == GIF_ERROR) {
       gif_push_error();
       i_push_error(0, "Unable to get record type");
-      if (colour_table) free(colour_table); /* FIXME: Isn't this an error? */
+      if (colour_table && *colour_table) {
+       free(*colour_table);
+       *colour_table = NULL;
+      }
       i_img_destroy(im);
       DGifCloseFile(GifFile);
       return NULL;
@@ -198,7 +208,10 @@ i_readgif_low(GifFileType *GifFile, int **colour_table, int *colours) {
       if (DGifGetImageDesc(GifFile) == GIF_ERROR) {
        gif_push_error();
        i_push_error(0, "Unable to get image descriptor");
-       if (colour_table) free(colour_table); /* FIXME: Isn't this an error? */
+       if (colour_table && *colour_table) {
+         free(*colour_table);
+         *colour_table = NULL;
+       }
        i_img_destroy(im);
        DGifCloseFile(GifFile);
        return NULL;
@@ -214,7 +227,7 @@ i_readgif_low(GifFileType *GifFile, int **colour_table, int *colours) {
          /* No colormap and we are about to read in the image - abandon for now */
          mm_log((1, "Going in with no colormap\n"));
          i_push_error(0, "Image does not have a local or a global color map");
-         if (colour_table) free(colour_table); /* FIXME: Isn't this an error? */
+         /* we can't have allocated a colour table here */
          i_img_destroy(im);
          DGifCloseFile(GifFile);
          return NULL;
@@ -230,7 +243,10 @@ i_readgif_low(GifFileType *GifFile, int **colour_table, int *colours) {
       if (GifFile->Image.Left + GifFile->Image.Width > GifFile->SWidth ||
          GifFile->Image.Top + GifFile->Image.Height > GifFile->SHeight) {
        i_push_errorf(0, "Image %d is not confined to screen dimension, aborted.\n",ImageNum);
-       if (colour_table) free(colour_table); /* FIXME: Yet again */
+       if (colour_table && *colour_table) {
+         free(*colour_table);
+         *colour_table = NULL;
+       }
        i_img_destroy(im);
        DGifCloseFile(GifFile);
        return(0);
@@ -242,8 +258,10 @@ i_readgif_low(GifFileType *GifFile, int **colour_table, int *colours) {
          if (DGifGetLine(GifFile, &GifRow[Col], Width) == GIF_ERROR) {
            gif_push_error();
            i_push_error(0, "Reading GIF line");
-           if (colour_table)
-             free(colour_table);
+           if (colour_table && *colour_table) {
+             free(*colour_table);
+             *colour_table = NULL;
+           }
            i_img_destroy(im);
            DGifCloseFile(GifFile);
            return NULL;
@@ -264,8 +282,10 @@ i_readgif_low(GifFileType *GifFile, int **colour_table, int *colours) {
          if (DGifGetLine(GifFile, &GifRow[Col], Width) == GIF_ERROR) {
            gif_push_error();
            i_push_error(0, "Reading GIF line");
-           if (colour_table)
-             free(colour_table);
+           if (colour_table && *colour_table) {
+             free(*colour_table);
+             *colour_table = NULL;
+           }
            i_img_destroy(im);
            DGifCloseFile(GifFile);
            return NULL;
@@ -287,8 +307,10 @@ i_readgif_low(GifFileType *GifFile, int **colour_table, int *colours) {
       if (DGifGetExtension(GifFile, &ExtCode, &Extension) == GIF_ERROR) {
        gif_push_error();
        i_push_error(0, "Reading extension record");
-       if (colour_table)
-         free(colour_table);
+       if (colour_table && *colour_table) {
+         free(*colour_table);
+         *colour_table = NULL;
+       }
        i_img_destroy(im);
        DGifCloseFile(GifFile);
        return NULL;
@@ -297,8 +319,10 @@ i_readgif_low(GifFileType *GifFile, int **colour_table, int *colours) {
        if (DGifGetExtensionNext(GifFile, &Extension) == GIF_ERROR) {
          gif_push_error();
          i_push_error(0, "reading next block of extension");
-         if (colour_table)
-           free(colour_table);
+         if (colour_table && *colour_table) {
+           free(*colour_table);
+           *colour_table = NULL;
+         }
          i_img_destroy(im);
          DGifCloseFile(GifFile);
          return NULL;
@@ -317,8 +341,10 @@ i_readgif_low(GifFileType *GifFile, int **colour_table, int *colours) {
   if (DGifCloseFile(GifFile) == GIF_ERROR) {
     gif_push_error();
     i_push_error(0, "Closing GIF file object");
-    if (colour_table)
-      free(colour_table);
+    if (colour_table && *colour_table) {
+      free(*colour_table);
+      *colour_table = NULL;
+    }
     i_img_destroy(im);
     return NULL;
   }

diff --git a/t/t105gif.t b/t/t105gif.t
index 5fcfaa8f85a0ae081ac344e2610e2708462344c4..509310013212f574d932d8fd82b91225ab228a6a 100644 (file)
--- a/t/t105gif.t
+++ b/t/t105gif.t
@@ -1,5 +1,5 @@
 $|=1;
-print "1..19\n";
+print "1..22\n";
 use Imager qw(:all);
 
 init_log("testout/t105gif.log",1);
@@ -293,9 +293,25 @@ EOS
     else {
       print "ok 19 # ",Imager::_error_as_msg(),"\n";
     }
-}
 
+    # try to read a truncated gif (no image descriptors)
+    read_failure('testimg/trimgdesc.gif', 20);
+    # file truncated just after the image descriptor tag
+    read_failure('testimg/trmiddesc.gif', 21);
+    # image has no colour map
+    read_failure('testimg/nocmap.gif', 22);
 
+    # image has a local colour map
+    open FH, "< testimg/loccmap.gif"
+      or die "Cannot open testimg/loccmap.gif: $!";
+    binmode FH;
+    if (i_readgif(fileno(FH))) {
+      print "ok 23\n";
+    }
+    else {
+      print "not ok 23 # failed to read image with only a local colour map";
+    }
+}
 
 sub test_readgif_cb {
   my ($size) = @_;
@@ -306,3 +322,21 @@ sub test_readgif_cb {
   close FH; 
   return $img;
 }
+
+# tests for reading bad gif files
+sub read_failure {
+  my ($filename, $testnum) = @_;
+
+  open FH, "< $filename"
+    or die "Cannot open $filename: $!";
+  binmode FH;
+  my ($result, $map) = i_readgif(fileno(FH));
+  if ($result) {
+    print "not ok $testnum # this is an invalid file, we succeeded\n";
+  }
+  else {
+    print "ok $testnum # ",Imager::_error_as_msg(),"\n";
+  }
+  close FH;
+}
+

diff --git a/testimg/loccmap.gif b/testimg/loccmap.gif
new file mode 100644 (file)
index 0000000..3a595bd
Binary files /dev/null and b/testimg/loccmap.gif differ

diff --git a/testimg/nocmap.gif b/testimg/nocmap.gif
new file mode 100644 (file)
index 0000000..8394110
Binary files /dev/null and b/testimg/nocmap.gif differ

diff --git a/testimg/trimgdesc.gif b/testimg/trimgdesc.gif
new file mode 100644 (file)
index 0000000..f352b0e
Binary files /dev/null and b/testimg/trimgdesc.gif differ

diff --git a/testimg/trmiddesc.gif b/testimg/trmiddesc.gif
new file mode 100644 (file)
index 0000000..386e3c1
Binary files /dev/null and b/testimg/trmiddesc.gif differ