Fix bmp RLE "bug"
authormsarett <msarett@google.com>
Wed, 12 Aug 2015 15:08:56 +0000 (08:08 -0700)
committerCommit bot <commit-bot@chromium.org>
Wed, 12 Aug 2015 15:08:56 +0000 (08:08 -0700)
Chromium's test suite contains an RLE image that reports a certain
file size in the header, but then contains additional encoded data.
Our bmp decoder would only decode half of the image, before stopping.

With this fix, we check for additional data before returning
kIncompleteInput.

If this lands, I will upload the test image to the bots.

Also adding an invalid image test to CodexTest.

BUG=skia:

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

resources/invalid_images/mask-bmp-ico.ico [new file with mode: 0644]
src/codec/SkBmpCodec.cpp
src/codec/SkBmpRLECodec.cpp
src/codec/SkBmpRLECodec.h
tests/CodexTest.cpp

diff --git a/resources/invalid_images/mask-bmp-ico.ico b/resources/invalid_images/mask-bmp-ico.ico
new file mode 100644 (file)
index 0000000..159699b
Binary files /dev/null and b/resources/invalid_images/mask-bmp-ico.ico differ
index 5eb5354..fa4608f 100644 (file)
@@ -478,6 +478,7 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) {
             case kBitMask_BmpInputFormat:
                 // Bmp-in-Ico must be standard mode
                 if (inIco) {
+                    SkCodecPrintf("Error: Icos may not use bit mask format.\n");
                     return false;
                 }
                 // Skip to the start of the pixel array.
@@ -493,9 +494,10 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) {
                 return true;
             case kRLE_BmpInputFormat:
                 // Bmp-in-Ico must be standard mode
-                if (inIco) {
-                    return false;
-                }
+                // When inIco is true, this line cannot be reached, since we
+                // require that RLE Bmps have a valid number of totalBytes, and
+                // Icos skip the header that contains totalBytes.
+                SkASSERT(!inIco);
                 *codecOut = SkNEW_ARGS(SkBmpRLECodec, (
                         imageInfo, stream, bitsPerPixel, numColors,
                         bytesPerColor, offset - bytesRead, rowOrder, RLEBytes));
@@ -553,7 +555,7 @@ void* SkBmpCodec::getDstStartRow(void* dst, size_t dstRowBytes, int32_t y) const
  */
 uint32_t SkBmpCodec::computeNumColors(uint32_t numColors) {
     // Zero is a default for maxColors
-    // Also set fNumColors to maxColors when it is too large
+    // Also set numColors to maxColors when it is too large
     uint32_t maxColors = 1 << fBitsPerPixel;
     if (numColors == 0 || numColors >= maxColors) {
         return maxColors;
index 6be7449..c71a540 100644 (file)
@@ -183,6 +183,41 @@ bool SkBmpRLECodec::initializeStreamBuffer() {
 }
 
 /*
+ * Before signalling kIncompleteInput, we should attempt to load the
+ * stream buffer with additional data.
+ *
+ * @return the number of bytes remaining in the stream buffer after
+ *         attempting to read more bytes from the stream
+ */
+size_t SkBmpRLECodec::checkForMoreData() {
+    const size_t remainingBytes = fRLEBytes - fCurrRLEByte;
+    uint8_t* buffer = fStreamBuffer.get();
+
+    // We will be reusing the same buffer, starting over from the beginning.
+    // Move any remaining bytes to the start of the buffer.
+    // We use memmove() instead of memcpy() because there is risk that the dst
+    // and src memory will overlap in corrupt images.
+    memmove(buffer, SkTAddOffset<uint8_t>(buffer, fCurrRLEByte), remainingBytes);
+
+    // Adjust the buffer ptr to the start of the unfilled data.
+    buffer += remainingBytes;
+
+    // Try to read additional bytes from the stream.  There are fCurrRLEByte
+    // bytes of additional space remaining in the buffer, assuming that we
+    // have already copied remainingBytes to the start of the buffer.
+    size_t additionalBytes = this->stream()->read(buffer, fCurrRLEByte);
+
+    // Update counters and return the number of bytes we currently have
+    // available.  We are at the start of the buffer again.
+    fCurrRLEByte = 0;
+    // If we were unable to fill the buffer, fRLEBytes is no longer equal to
+    // the size of the buffer.  There will be unused space at the end.  This
+    // should be fine, given that there are no more bytes in the stream.
+    fRLEBytes = remainingBytes + additionalBytes;
+    return fRLEBytes;
+}
+
+/*
  * Set an RLE pixel using the color table
  */
 void SkBmpRLECodec::setPixel(void* dst, size_t dstRowBytes,
@@ -287,8 +322,10 @@ SkCodec::Result SkBmpRLECodec::decode(const SkImageInfo& dstInfo,
 
         // Every entry takes at least two bytes
         if ((int) fRLEBytes - fCurrRLEByte < 2) {
-            SkCodecPrintf("Warning: incomplete RLE input.\n");
-            return kIncompleteInput;
+            SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+            if (this->checkForMoreData() < 2) {
+                return kIncompleteInput;
+            }
         }
 
         // Read the next two bytes.  These bytes have different meanings
@@ -310,8 +347,10 @@ SkCodec::Result SkBmpRLECodec::decode(const SkImageInfo& dstInfo,
                 case RLE_DELTA: {
                     // Two bytes are needed to specify delta
                     if ((int) fRLEBytes - fCurrRLEByte < 2) {
-                        SkCodecPrintf("Warning: incomplete RLE input\n");
-                        return kIncompleteInput;
+                        SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+                        if (this->checkForMoreData() < 2) {
+                            return kIncompleteInput;
+                        }
                     }
                     // Modify x and y
                     const uint8_t dx = fStreamBuffer.get()[fCurrRLEByte++];
@@ -319,8 +358,8 @@ SkCodec::Result SkBmpRLECodec::decode(const SkImageInfo& dstInfo,
                     x += dx;
                     y += dy;
                     if (x > width || y > height) {
-                        SkCodecPrintf("Warning: invalid RLE input 1.\n");
-                        return kIncompleteInput;
+                        SkCodecPrintf("Warning: invalid RLE input.\n");
+                        return kInvalidInput;
                     }
                     break;
                 }
@@ -333,12 +372,18 @@ SkCodec::Result SkBmpRLECodec::decode(const SkImageInfo& dstInfo,
                     const size_t rowBytes = compute_row_bytes(numPixels,
                             this->bitsPerPixel());
                     // Abort if setting numPixels moves us off the edge of the
-                    // image.  Also abort if there are not enough bytes
+                    // image.
+                    if (x + numPixels > width) {
+                        SkCodecPrintf("Warning: invalid RLE input.\n");
+                        return kInvalidInput;
+                    }
+                    // Also abort if there are not enough bytes
                     // remaining in the stream to set numPixels.
-                    if (x + numPixels > width ||
-                            (int) fRLEBytes - fCurrRLEByte < SkAlign2(rowBytes)) {
-                        SkCodecPrintf("Warning: invalid RLE input 2.\n");
-                        return kIncompleteInput;
+                    if ((int) fRLEBytes - fCurrRLEByte < SkAlign2(rowBytes)) {
+                        SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+                        if (this->checkForMoreData() < SkAlign2(rowBytes)) {
+                            return kIncompleteInput;
+                        }
                     }
                     // Set numPixels number of pixels
                     while (numPixels > 0) {
@@ -394,8 +439,10 @@ SkCodec::Result SkBmpRLECodec::decode(const SkImageInfo& dstInfo,
                 // There are two more required bytes to finish encoding the
                 // color.
                 if ((int) fRLEBytes - fCurrRLEByte < 2) {
-                    SkCodecPrintf("Warning: incomplete RLE input\n");
-                    return kIncompleteInput;
+                    SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+                    if (this->checkForMoreData() < 2) {
+                        return kIncompleteInput;
+                    }
                 }
 
                 // Fill the pixels up to endX with the specified color
index f1f1ae9..ee8989b 100644 (file)
@@ -56,6 +56,15 @@ private:
     bool initializeStreamBuffer();
 
     /*
+     * Before signalling kIncompleteInput, we should attempt to load the
+     * stream buffer with additional data.
+     *
+     * @return the number of bytes remaining in the stream buffer after
+     *         attempting to read more bytes from the stream
+     */
+    size_t checkForMoreData();
+
+    /*
      * Set an RLE pixel using the color table
      */
     void setPixel(void* dst, size_t dstRowBytes,
index b7f1584..f3137a8 100644 (file)
@@ -266,7 +266,7 @@ DEF_TEST(Codec_Dimensions, r) {
     test_dimensions(r, "randPixels.jpg");
 }
 
-static void test_empty(skiatest::Reporter* r, const char path[]) {
+static void test_invalid(skiatest::Reporter* r, const char path[]) {
     SkAutoTDelete<SkStream> stream(resource(path));
     if (!stream) {
         SkDebugf("Missing resource '%s'\n", path);
@@ -278,16 +278,18 @@ static void test_empty(skiatest::Reporter* r, const char path[]) {
 
 DEF_TEST(Codec_Empty, r) {
     // Test images that should not be able to create a codec
-    test_empty(r, "empty_images/zero-dims.gif");
-    test_empty(r, "empty_images/zero-embedded.ico");
-    test_empty(r, "empty_images/zero-width.bmp");
-    test_empty(r, "empty_images/zero-height.bmp");
-    test_empty(r, "empty_images/zero-width.jpg");
-    test_empty(r, "empty_images/zero-height.jpg");
-    test_empty(r, "empty_images/zero-width.png");
-    test_empty(r, "empty_images/zero-height.png");
-    test_empty(r, "empty_images/zero-width.wbmp");
-    test_empty(r, "empty_images/zero-height.wbmp");
+    test_invalid(r, "empty_images/zero-dims.gif");
+    test_invalid(r, "empty_images/zero-embedded.ico");
+    test_invalid(r, "empty_images/zero-width.bmp");
+    test_invalid(r, "empty_images/zero-height.bmp");
+    test_invalid(r, "empty_images/zero-width.jpg");
+    test_invalid(r, "empty_images/zero-height.jpg");
+    test_invalid(r, "empty_images/zero-width.png");
+    test_invalid(r, "empty_images/zero-height.png");
+    test_invalid(r, "empty_images/zero-width.wbmp");
+    test_invalid(r, "empty_images/zero-height.wbmp");
+    // This image is an ico with an embedded mask-bmp.  This is illegal.
+    test_invalid(r, "invalid_images/mask-bmp-ico.ico");
 }
 
 static void test_invalid_parameters(skiatest::Reporter* r, const char path[]) {