Revert "Indexed PNG decoding: Ensure color table is large enough that the bit depth...

This reverts commit 493c1ce1cd406ef28683203146274154783452ce.

NOTRY=true
NOTREECHECKS=true
TBR=egdaniel@google.com,dml@google.com
BUG=skia:

Review URL: https://codereview.chromium.org/1014553003

diff --git a/gyp/tests.gypi b/gyp/tests.gypi
index f03488d..d4ca3af 100644 (file)
--- a/gyp/tests.gypi
+++ b/gyp/tests.gypi
@@ -126,7 +126,6 @@
     '../tests/ImageGeneratorTest.cpp',
     '../tests/ImageIsOpaqueTest.cpp',
     '../tests/ImageNewShaderTest.cpp',
-    '../tests/IndexedPngOverflowTest.cpp',
     '../tests/InfRectTest.cpp',
     '../tests/InterpolatorTest.cpp',
     '../tests/InvalidIndexedPngTest.cpp',

diff --git a/src/images/SkImageDecoder_libpng.cpp b/src/images/SkImageDecoder_libpng.cpp
index acaeda5..c074268 100644 (file)
--- a/src/images/SkImageDecoder_libpng.cpp
+++ b/src/images/SkImageDecoder_libpng.cpp
@@ -97,7 +97,7 @@ private:
     SkPNGImageIndex* fImageIndex;
 
     bool onDecodeInit(SkStream* stream, png_structp *png_ptrp, png_infop *info_ptrp);
-    bool decodePalette(png_structp png_ptr, png_infop info_ptr, int bitDepth,
+    bool decodePalette(png_structp png_ptr, png_infop info_ptr,
                        bool * SK_RESTRICT hasAlphap, bool *reallyHasAlphap,
                        SkColorTable **colorTablep);
     bool getBitmapColorType(png_structp, png_infop, SkColorType*, bool* hasAlpha,
@@ -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, bitDepth, &hasAlpha, &reallyHasAlpha, &colorTable);
+        decodePalette(png_ptr, info_ptr, &hasAlpha, &reallyHasAlpha, &colorTable);
     }
 
     SkAutoUnref aur(colorTable);
@@ -657,8 +657,7 @@ bool SkPNGImageDecoder::getBitmapColorType(png_structp png_ptr, png_infop info_p
 typedef uint32_t (*PackColorProc)(U8CPU a, U8CPU r, U8CPU g, U8CPU b);
 
 bool SkPNGImageDecoder::decodePalette(png_structp png_ptr, png_infop info_ptr,
-                                      int bitDepth, bool *hasAlphap,
-                                      bool *reallyHasAlphap,
+                                      bool *hasAlphap, bool *reallyHasAlphap,
                                       SkColorTable **colorTablep) {
     int numPalette;
     png_colorp palette;
@@ -667,6 +666,13 @@ 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,19 +711,9 @@ bool SkPNGImageDecoder::decodePalette(png_structp png_ptr, png_infop info_ptr,
         palette++;
     }
 
-    /*  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;
+    // see BUGGY IMAGE WORKAROUND comment above
+    if (numPalette < 256) {
+        *colorPtr = colorPtr[-1];
     }
 
     *colorTablep = SkNEW_ARGS(SkColorTable, (colorStorage, colorCount));
@@ -807,7 +803,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, bitDepth);
+        decodePalette(png_ptr, info_ptr, &hasAlpha, &reallyHasAlpha, &colorTable);
     }
 
     SkAutoUnref aur(colorTable);

diff --git a/tests/IndexedPngOverflowTest.cpp b/tests/IndexedPngOverflowTest.cpp
deleted file mode 100644 (file)
index 914053a..0000000
--- a/tests/IndexedPngOverflowTest.cpp
+++ /dev/null
@@ -1,44 +0,0 @@
-/*
- * Copyright 2015 Google Inc.
- *
- * Use of this source code is governed by a BSD-style license that can be
- * found in the LICENSE file.
- */
-
-#include "SkBitmap.h"
-#include "SkCanvas.h"
-#include "SkForceLinking.h"
-#include "SkImageDecoder.h"
-#include "SkImageInfo.h"
-#include "SkSurface.h"
-#include "Test.h"
-
-// A 20x1 image with 8 bits per pixel and a palette size of 2. Pixel values are 255, 254... Run
-// this test with ASAN to make sure we don't try to access before/after any palette-sized buffers.
-unsigned char gPng[] = {
-    0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d,
-    0x49, 0x48, 0x44, 0x52, 0x00, 0x00, 0x00, 0x14, 0x00, 0x00, 0x00, 0x01,
-    0x08, 0x03, 0x00, 0x00, 0x00, 0xe9, 0x4c, 0x7e, 0x17, 0x00, 0x00, 0x00,
-    0x09, 0x70, 0x48, 0x59, 0x73, 0x00, 0x00, 0x00, 0x1c, 0x00, 0x00, 0x00,
-    0x1c, 0x00, 0x0f, 0x01, 0xb9, 0x8f, 0x00, 0x00, 0x00, 0x06, 0x50, 0x4c,
-    0x54, 0x45, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xa5, 0x67, 0xb9, 0xcf,
-    0x00, 0x00, 0x00, 0x20, 0x49, 0x44, 0x41, 0x54, 0x78, 0xda, 0xed, 0xfd,
-    0x07, 0x01, 0x00, 0x20, 0x08, 0x00, 0x41, 0xbc, 0x5b, 0xe8, 0xdf, 0x97,
-    0x99, 0xe3, 0x92, 0xa0, 0xf2, 0xdf, 0x3d, 0x7b, 0x0d, 0xda, 0x04, 0x1c,
-    0x03, 0xad, 0x00, 0x38, 0x5c, 0x2e, 0xad, 0x12, 0x00, 0x00, 0x00, 0x00,
-    0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82
-};
-
-DEF_TEST(IndexedPngOverflow, reporter) {
-    SkBitmap image;
-    SkForceLinking(false);
-    bool success = SkImageDecoder::DecodeMemory(
-        gPng, sizeof(gPng), &image, SkColorType::kUnknown_SkColorType,
-        SkImageDecoder::kDecodePixels_Mode);
-    REPORTER_ASSERT(reporter, success);
-
-    SkAutoTUnref<SkSurface> surface(SkSurface::NewRaster(SkImageInfo::MakeN32Premul(20, 1)));
-    SkCanvas* canvas = surface->getCanvas();
-    SkRect destRect = SkRect::MakeXYWH(0, 0, 20, 1);
-    canvas->drawBitmapRect(image, destRect, NULL);
-}