Fixing oss-fuzz issue 22432: leaks during png writing

* It wrote palette index exceeding num_palette * Initialize palette to NULL; free in the setjmp. * New function pixGetMaxColorIndex(); used in pixcmapIsValid(). * If pix is defined in pixcmapIsValid(), check the image for pixel values that exceed the palette size. This is a relatively expensive operation; don't do this for pixSetColormap() and pixCopyColormap(), but do check full colormap validity when reading gif, png, etc.
DanBloomberg · May 25, 2020 · cf6ba57 · cf6ba57
1 parent e88dea0
commit cf6ba57
Show file tree

Hide file tree

Showing 6 changed files with 118 additions and 11 deletions.
diff --git a/src/allheaders.h b/src/allheaders.h
@@ -476,7 +476,7 @@ LEPT_DLL extern PIXCMAP * pixcmapCreateRandom ( l_int32 depth, l_int32 hasblack,
 LEPT_DLL extern PIXCMAP * pixcmapCreateLinear ( l_int32 d, l_int32 nlevels );
 LEPT_DLL extern PIXCMAP * pixcmapCopy ( const PIXCMAP *cmaps );
 LEPT_DLL extern void pixcmapDestroy ( PIXCMAP **pcmap );
-LEPT_DLL extern l_ok pixcmapIsValid ( const PIXCMAP *cmap, const PIX *pix, l_int32 *pvalid );
+LEPT_DLL extern l_ok pixcmapIsValid ( const PIXCMAP *cmap, PIX *pix, l_int32 *pvalid );
 LEPT_DLL extern l_ok pixcmapAddColor ( PIXCMAP *cmap, l_int32 rval, l_int32 gval, l_int32 bval );
 LEPT_DLL extern l_ok pixcmapAddRGBA ( PIXCMAP *cmap, l_int32 rval, l_int32 gval, l_int32 bval, l_int32 aval );
 LEPT_DLL extern l_ok pixcmapAddNewColor ( PIXCMAP *cmap, l_int32 rval, l_int32 gval, l_int32 bval, l_int32 *pindex );
@@ -1649,6 +1649,7 @@ LEPT_DLL extern l_int32 pixColumnStats ( PIX *pixs, BOX *box, NUMA **pnamean, NU
 LEPT_DLL extern l_ok pixGetRangeValues ( PIX *pixs, l_int32 factor, l_int32 color, l_int32 *pminval, l_int32 *pmaxval );
 LEPT_DLL extern l_ok pixGetExtremeValue ( PIX *pixs, l_int32 factor, l_int32 type, l_int32 *prval, l_int32 *pgval, l_int32 *pbval, l_int32 *pgrayval );
 LEPT_DLL extern l_ok pixGetMaxValueInRect ( PIX *pixs, BOX *box, l_uint32 *pmaxval, l_int32 *pxmax, l_int32 *pymax );
+LEPT_DLL extern l_ok pixGetMaxColorIndex ( PIX *pixs, l_int32 *pmaxindex );
 LEPT_DLL extern l_ok pixGetBinnedComponentRange ( PIX *pixs, l_int32 nbins, l_int32 factor, l_int32 color, l_int32 *pminval, l_int32 *pmaxval, l_uint32 **pcarray, l_int32 fontsize );
 LEPT_DLL extern l_ok pixGetRankColorArray ( PIX *pixs, l_int32 nbins, l_int32 type, l_int32 factor, l_uint32 **pcarray, PIXA *pixadb, l_int32 fontsize );
 LEPT_DLL extern l_ok pixGetBinnedColor ( PIX *pixs, PIX *pixg, l_int32 factor, l_int32 nbins, NUMA *nalut, l_uint32 **pcarray, PIXA *pixadb );

diff --git a/src/colormap.c b/src/colormap.c
@@ -304,15 +304,20 @@ PIXCMAP  *cmap;
  *
  * <pre>
  * Notes:
- *      (1) If %pix is input, verify that pix values cannot overflow the cmap.
+ *      (1) If %pix is input, this will veify that pixel values cannot
+ *          overflow the colormap.  This is a relatively expensive operation
+ *          that may need to check all the pixel values.
+ *      (2) If %pix is input, there must be at least one color in the
+ *          colormap if it is to be valid with any pix, even if the
+ *          pixels are all 0.
  * </pre>
  */
 l_ok
 pixcmapIsValid(const PIXCMAP  *cmap,
-               const PIX      *pix,
+               PIX            *pix,
                l_int32        *pvalid)
 {
-l_int32  d, nalloc;
+l_int32  d, nalloc, maxindex;
 
     PROCNAME("pixcmapIsValid");
 
@@ -330,7 +335,7 @@ l_int32  d, nalloc;
     }
     nalloc = cmap->nalloc;
     if (nalloc != (1 << d)) {
-        L_ERROR("invalid cmap nalloc: %d; d = %d\n", procName, nalloc, d);
+        L_ERROR("invalid cmap nalloc = %d; d = %d\n", procName, nalloc, d);
         return 1;
     }
     if (cmap->n < 0 || cmap->n > nalloc) {
@@ -342,10 +347,27 @@ l_int32  d, nalloc;
          * must not exceed the cmap depth.  Do not require depth equality,
          * because some functions such as median cut quantizers do not. */
     if (pix && (pixGetDepth(pix) > d)) {
-        L_ERROR("pix depth = %d > cmap depth = %d\n", procName,
+        L_ERROR("(pix depth = %d) > (cmap depth = %d)\n", procName,
                 pixGetDepth(pix), d);
         return 1;
     }
+    if (pix && cmap->n < 1) {
+        L_ERROR("cmap array is empty; invalid with any pix\n", procName);
+        return 1;
+    }
+
+        /* Where the colormap or the pix may have been corrupted, and
+         * in particular when reading or writing image files, it should
+         * be verified that the image pixel values do not exceed the
+         * max indexing into the colormap array. */
+    if (pix) {
+        pixGetMaxColorIndex(pix, &maxindex);
+        if (maxindex >= cmap->n) {
+            L_ERROR("(max index = %d) >= (num colors = %d)\n", procName,
+                    maxindex, cmap->n);
+            return 1;
+        }
+    }
 
     *pvalid = 1;
     return 0;

diff --git a/src/gifio.c b/src/gifio.c
@@ -225,7 +225,7 @@ l_int32         bytesRead;
 static PIX *
 gifToPix(GifFileType  *gif)
 {
-l_int32          wpl, i, j, w, h, d, cindex, ncolors;
+l_int32          wpl, i, j, w, h, d, cindex, ncolors, valid;
 l_int32          rval, gval, bval;
 l_uint32        *data, *line;
 PIX             *pixd;
@@ -300,6 +300,13 @@ int              giferr;
     }
     pixSetInputFormat(pixd, IFF_GIF);
     pixSetColormap(pixd, cmap);
+    pixcmapIsValid(cmap, pixd, &valid);
+    if (!valid) {
+        DGifCloseFile(gif, &giferr);
+        pixDestroy(&pixd);
+        pixcmapDestroy(&cmap);
+        return (PIX *)ERROR_PTR("colormap is invalid", procName, NULL);
+    }
 
     wpl = pixGetWpl(pixd);
     data = pixGetData(pixd);

diff --git a/src/pix1.c b/src/pix1.c
@@ -822,7 +822,7 @@ PIXCMAP        *cmapd;
     pixDestroyColormap(pixd);
     if ((cmaps = pixs->colormap) == NULL)  /* not an error */
         return 0;
-    pixcmapIsValid(cmaps, pixs, &valid);
+    pixcmapIsValid(cmaps, NULL, &valid);
     if (!valid)
         return ERROR_INT("cmap not valid", procName, 1);
 
@@ -1637,7 +1637,11 @@ pixGetColormap(PIX  *pix)
  *      (3) If the colormap is not valid, this returns 1.  The caller
  *          should check if there is a possibility that the pix and
  *          colormap depths differ.
- *      (4) Because colormaps are not ref counted, the new colormap
+ *      (4) This does not do the work of checking pixs for a pixel value
+ *          that is out of bounds for the colormap -- that only needs to
+ *          be done when reading and writing with an I/O library like
+ *          png and gif.
+ *      (5) Because colormaps are not ref counted, the new colormap
  *          must not belong to any other pix.
  * </pre>
  */
@@ -1657,7 +1661,7 @@ l_int32  valid;
     pixDestroyColormap(pix);
     pix->colormap = colormap;
 
-    pixcmapIsValid(colormap, pix, &valid);
+    pixcmapIsValid(colormap, NULL, &valid);
     if (!valid)
         return ERROR_INT("colormap is not valid", procName, 1);
     return 0;
@@ -1698,6 +1702,11 @@ PIXCMAP  *cmap;
  * Notes:
  *      (1) This gives a new handle for the data.  The data is still
  *          owned by the pix, so do not call LEPT_FREE() on it.
+ *      (2) This cannot guarantee that the pix data returned will not
+ *          be changed, so %pix cannot be declared const.  And because
+ *          most imaging operations call this for access to the data,
+ *          this prevents them from declaring %pix to be const, even if
+ *          they only use the data for inspection.
  * </pre>
  */
 l_uint32 *

diff --git a/src/pix4.c b/src/pix4.c
@@ -62,6 +62,7 @@
  *           l_int32     pixGetRangeValues()
  *           l_int32     pixGetExtremeValue()
  *           l_int32     pixGetMaxValueInRect()
+ *           l_int32     pixGetMaxColorIndex()
  *           l_int32     pixGetBinnedComponentRange()
  *           l_int32     pixGetRankColorArray()
  *           l_int32     pixGetBinnedColor()
@@ -2411,6 +2412,66 @@ l_uint32  *data, *line;
 }
 
 
+/*!
+ * \brief   pixGetMaxColorIndex()
+ *
+ * \param[in]    pixs          1, 2, 4 or 8 bpp colormapped
+ * \param[out]   pmaxindex     max colormap index value
+ * \return  0 if OK, 1 on error
+ */
+l_ok
+pixGetMaxColorIndex(PIX      *pixs,
+                    l_int32  *pmaxindex)
+{
+l_int32    i, j, w, h, d, wpl, val, max, maxval, empty;
+l_uint32  *data, *line;
+
+    PROCNAME("pixGetMaxColorIndex");
+
+    if (!pmaxindex)
+        return ERROR_INT("&maxindex not defined", procName, 1);
+    *pmaxindex = 0;
+    if (!pixs)
+        return ERROR_INT("pixs not defined", procName, 1);
+    pixGetDimensions(pixs, &w, &h, &d);
+    if (d != 1 && d != 2 && d != 4 && d != 8)
+        return ERROR_INT("invalid pixs depth; not in (1,2,4,8}", procName, 1);
+
+    wpl = pixGetWpl(pixs);
+    data = pixGetData(pixs);
+    max = 0;
+    maxval = (1 << d) - 1;
+    if (d == 1) {
+        pixZero(pixs, &empty);
+        if (!empty) max = 1;
+        *pmaxindex = max;
+        return 0;
+    }
+    for (i = 0; i < h; i++) {
+        line = data + i * wpl;
+        if (d == 2) {
+            for (j = 0; j < w; j++) {
+                val = GET_DATA_DIBIT(line, j);
+                if (val > max) max = val;
+            }
+        } else if (d == 4) {
+            for (j = 0; j < w; j++) {
+                val = GET_DATA_QBIT(line, j);
+                if (val > max) max = val;
+            }
+        } else if (d == 8) {
+            for (j = 0; j < w; j++) {
+                val = GET_DATA_BYTE(line, j);
+                if (val > max) max = val;
+            }
+        }
+        if (max == maxval) break;
+    }
+    *pmaxindex = max;
+    return 0;
+}
+
+
 /*!
  * \brief   pixGetBinnedComponentRange()
  *

diff --git a/src/pngio.c b/src/pngio.c
@@ -1034,7 +1034,7 @@ png_bytep   *row_pointers;
 png_bytep    rowbuffer;
 png_structp  png_ptr;
 png_infop    info_ptr;
-png_colorp   palette;
+png_colorp   palette = NULL;
 PIX         *pix1;
 PIXCMAP     *cmap;
 char        *text;
@@ -1050,6 +1050,8 @@ char        *text;
     h = pixGetHeight(pix);
     d = pixGetDepth(pix);
     spp = pixGetSpp(pix);
+
+        /* A cmap validity check should prevent low-level colormap errors. */
     if ((cmap = pixGetColormap(pix))) {
         cmflag = 1;
         pixcmapIsValid(cmap, pix, &valid);
@@ -1091,8 +1093,13 @@ char        *text;
     }
 
         /* Set up png setjmp error handling */
+    pix1 = NULL;
+    row_pointers = NULL;
     if (setjmp(png_jmpbuf(png_ptr))) {
         png_destroy_write_struct(&png_ptr, &info_ptr);
+        LEPT_FREE(palette);
+        LEPT_FREE(row_pointers);
+        pixDestroy(&pix1);
         return ERROR_INT("internal png error", procName, 1);
     }