Fix bmp RLE "bug"

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

src/codec/SkBmpCodec.cpp

@@ -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) {
+                // When inIco is true, this line cannot be reached, since we
-                    return false;
+                // 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;

src/codec/SkBmpRLECodec.cpp

@@ -182,6 +182,41 @@ bool SkBmpRLECodec::initializeStreamBuffer() {
     return true;
 }
 
+/*
+ * 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
  */
@@ -287,9 +322,11 @@ 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");
+            SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+            if (this->checkForMoreData() < 2) {
                 return kIncompleteInput;
             }
+        }
 
         // Read the next two bytes.  These bytes have different meanings
         // depending on their values.  In the first interpretation, the first
@@ -310,17 +347,19 @@ 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");
+                        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++];
                     const uint8_t dy = fStreamBuffer.get()[fCurrRLEByte++];
                     x += dx;
                     y += dy;
                     if (x > width || y > height) {
-                        SkCodecPrintf("Warning: invalid RLE input 1.\n");
+                        SkCodecPrintf("Warning: invalid RLE input.\n");
-                        return kIncompleteInput;
+                        return kInvalidInput;
                     }
                     break;
                 }
@@ -333,13 +372,19 @@ 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 ||
+                    if ((int) fRLEBytes - fCurrRLEByte < SkAlign2(rowBytes)) {
-                            (int) fRLEBytes - fCurrRLEByte < SkAlign2(rowBytes)) {
+                        SkCodecPrintf("Warning: might be incomplete RLE input.\n");
-                        SkCodecPrintf("Warning: invalid RLE input 2.\n");
+                        if (this->checkForMoreData() < SkAlign2(rowBytes)) {
                             return kIncompleteInput;
                         }
+                    }
                     // Set numPixels number of pixels
                     while (numPixels > 0) {
                         switch(this->bitsPerPixel()) {
@@ -394,9 +439,11 @@ 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");
+                    SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+                    if (this->checkForMoreData() < 2) {
                         return kIncompleteInput;
                     }
+                }
 
                 // Fill the pixels up to endX with the specified color
                 uint8_t blue = task;

src/codec/SkBmpRLECodec.h

@@ -55,6 +55,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
      */

tests/CodexTest.cpp

@@ -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_invalid(r, "empty_images/zero-dims.gif");
-    test_empty(r, "empty_images/zero-embedded.ico");
+    test_invalid(r, "empty_images/zero-embedded.ico");
-    test_empty(r, "empty_images/zero-width.bmp");
+    test_invalid(r, "empty_images/zero-width.bmp");
-    test_empty(r, "empty_images/zero-height.bmp");
+    test_invalid(r, "empty_images/zero-height.bmp");
-    test_empty(r, "empty_images/zero-width.jpg");
+    test_invalid(r, "empty_images/zero-width.jpg");
-    test_empty(r, "empty_images/zero-height.jpg");
+    test_invalid(r, "empty_images/zero-height.jpg");
-    test_empty(r, "empty_images/zero-width.png");
+    test_invalid(r, "empty_images/zero-width.png");
-    test_empty(r, "empty_images/zero-height.png");
+    test_invalid(r, "empty_images/zero-height.png");
-    test_empty(r, "empty_images/zero-width.wbmp");
+    test_invalid(r, "empty_images/zero-width.wbmp");
-    test_empty(r, "empty_images/zero-height.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[]) {