Indexed PNG decoding: Ensure color table is large enough that the bit depth of the image will not a…

src/images/SkImageDecoder_libpng.cpp

Index: src/images/SkImageDecoder_libpng.cpp
diff --git a/src/images/SkImageDecoder_libpng.cpp b/src/images/SkImageDecoder_libpng.cpp
index f9ef6b7942306016c714668555d96b7c3c31fbb3..53d02dc5f4ce22021a3f37d6308089b3e8498c1b 100644
--- a/src/images/SkImageDecoder_libpng.cpp
+++ b/src/images/SkImageDecoder_libpng.cpp
@@ -99,7 +99,7 @@ private:
     bool onDecodeInit(SkStream* stream, png_structp *png_ptrp, png_infop *info_ptrp);
     bool decodePalette(png_structp png_ptr, png_infop info_ptr,
                        bool * SK_RESTRICT hasAlphap, bool *reallyHasAlphap,
-                       SkColorTable **colorTablep);
+                       SkColorTable **colorTablep, int bitDepth);

[scroggo, 2015/03/05 23:23:30]

nit: I would prefer to have this new input parameter before all these output
parameters.

[David Lattimore, 2015/03/06 00:15:26]

Done.

     bool getBitmapColorType(png_structp, png_infop, SkColorType*, bool* hasAlpha,
                             SkPMColor* theTranspColor);
 
@@ -350,7 +350,7 @@ SkImageDecoder::Result SkPNGImageDecoder::onDecode(SkStream* sk_stream, SkBitmap
     SkColorTable* colorTable = NULL;
 
     if (pngColorType == PNG_COLOR_TYPE_PALETTE) {
-        decodePalette(png_ptr, info_ptr, &hasAlpha, &reallyHasAlpha, &colorTable);
+        decodePalette(png_ptr, info_ptr, &hasAlpha, &reallyHasAlpha, &colorTable, bitDepth);
     }
 
     SkAutoUnref aur(colorTable);
@@ -652,7 +652,7 @@ typedef uint32_t (*PackColorProc)(U8CPU a, U8CPU r, U8CPU g, U8CPU b);
 
 bool SkPNGImageDecoder::decodePalette(png_structp png_ptr, png_infop info_ptr,
                                       bool *hasAlphap, bool *reallyHasAlphap,
-                                      SkColorTable **colorTablep) {
+                                      SkColorTable **colorTablep, int bitDepth) {
     int numPalette;
     png_colorp palette;
     png_bytep trans;
@@ -660,13 +660,6 @@ bool SkPNGImageDecoder::decodePalette(png_structp png_ptr, png_infop info_ptr,
 
     png_get_PLTE(png_ptr, info_ptr, &palette, &numPalette);
 
-    /*  BUGGY IMAGE WORKAROUND
-
-        We hit some images (e.g. fruit_.png) who contain bytes that are == colortable_count
-        which is a problem since we use the byte as an index. To work around this we grow
-        the colortable by 1 (if its < 256) and duplicate the last color into that slot.
-    */
-    int colorCount = numPalette + (numPalette < 256);
     SkPMColor colorStorage[256];    // worst-case storage
     SkPMColor* colorPtr = colorStorage;
 
@@ -705,9 +698,19 @@ bool SkPNGImageDecoder::decodePalette(png_structp png_ptr, png_infop info_ptr,
         palette++;
     }
 
-    // see BUGGY IMAGE WORKAROUND comment above
-    if (numPalette < 256) {
-        *colorPtr = colorPtr[-1];
+    /*  BUGGY IMAGE WORKAROUND
+
+        Invalid images could contain pixel values that are greater than the number of palette
+        entries. Since we use pixel values as indices into the palette this could result in reading
+        beyond the end of the palette which could leak the contents of uninitialized memory. To
+        ensure this doesn't happen, we grow the colortable to the maximum size that can be
+        addressed by the bitdepth of the image and fill it with the last palette color or black if
+        the palette is empty (really broken image).
+    */
+    int colorCount = SkTMax(numPalette, 1 << SkTMin(bitDepth, 8));
+    SkPMColor lastColor = index > 0 ? colorPtr[-1] : SkPackARGB32(0xFF, 0, 0, 0);
+    for (; index < colorCount; index++) {
+        *colorPtr++ = lastColor;
     }
 
     *colorTablep = SkNEW_ARGS(SkColorTable, (colorStorage, colorCount));
@@ -797,7 +800,7 @@ bool SkPNGImageDecoder::onDecodeSubset(SkBitmap* bm, const SkIRect& region) {
     SkColorTable* colorTable = NULL;
 
     if (pngColorType == PNG_COLOR_TYPE_PALETTE) {
-        decodePalette(png_ptr, info_ptr, &hasAlpha, &reallyHasAlpha, &colorTable);
+        decodePalette(png_ptr, info_ptr, &hasAlpha, &reallyHasAlpha, &colorTable, bitDepth);
     }
 
     SkAutoUnref aur(colorTable);