Use fixed size buffer for RLE bmps

An RLE bmp reports how many bytes it should contain. This number may be
incorrect, or it may be a very large number. Previously, we buffered
all bytes in a single allocation. Instead, use a fixed size buffer and
only read what fits into the buffer. We already have code to refill the
buffer if there is more data, so rely on that to keep reading.

Choose an arbitrary size for the buffer. It is larger than the maximum
possible number of bytes we need to read at once.

Add a test with a test image that reports a very large number for
the number of bytes it should contain. With the old method, we would
allocate 4 gigs of memory to decode this image, which is unnecessary
and may result in OOM.

BUG=b/33251605

Change-Id: I6d66eace626002725f62237617140cab99ce42f3
Reviewed-on: https://skia-review.googlesource.com/7028
Commit-Queue: Leon Scroggins <scroggo@google.com>
Reviewed-by: Matt Sarett <msarett@google.com>

5 files changed, 56 insertions, 64 deletions

diff --git a/resources/invalid_images/b33251605.bmp b/resources/invalid_images/b33251605.bmp
new file mode 100644
index 0000000000..0060ff48dd
--- /dev/null
+++ b/resources/invalid_images/b33251605.bmp
diff --git a/src/codec/SkBmpCodec.cpp b/src/codec/SkBmpCodec.cpp
index 83ba21b0d6..66ad0caa2b 100644
--- a/src/codec/SkBmpCodec.cpp
+++ b/src/codec/SkBmpCodec.cpp
@@ -549,7 +549,6 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) {
                 SkCodecPrintf("Error: RLE requires valid input size.\n");
                 return false;
             }
-            const size_t RLEBytes = totalBytes - offset;
 
             // Bmp-in-Ico must be standard mode
             // When inIco is true, this line cannot be reached, since we
@@ -565,7 +564,7 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) {
                 const SkEncodedInfo info = SkEncodedInfo::Make(SkEncodedInfo::kBGRA_Color,
                         SkEncodedInfo::kBinary_Alpha, 8);
                 *codecOut = new SkBmpRLECodec(width, height, info, stream, bitsPerPixel, numColors,
-                        bytesPerColor, offset - bytesRead, rowOrder, RLEBytes);
+                        bytesPerColor, offset - bytesRead, rowOrder);
             }
             return true;
         }
diff --git a/src/codec/SkBmpRLECodec.cpp b/src/codec/SkBmpRLECodec.cpp
index c2574ddef8..1968616fa6 100644
--- a/src/codec/SkBmpRLECodec.cpp
+++ b/src/codec/SkBmpRLECodec.cpp
@@ -17,16 +17,13 @@
 SkBmpRLECodec::SkBmpRLECodec(int width, int height, const SkEncodedInfo& info, SkStream* stream,
                              uint16_t bitsPerPixel, uint32_t numColors,
                              uint32_t bytesPerColor, uint32_t offset,
-                             SkCodec::SkScanlineOrder rowOrder,
-                             size_t RLEBytes)
+                             SkCodec::SkScanlineOrder rowOrder)
     : INHERITED(width, height, info, stream, bitsPerPixel, rowOrder)
     , fColorTable(nullptr)
     , fNumColors(numColors)
     , fBytesPerColor(bytesPerColor)
     , fOffset(offset)
-    , fStreamBuffer(new uint8_t[RLEBytes])
-    , fRLEBytes(RLEBytes)
-    , fOrigRLEBytes(RLEBytes)
+    , fBytesBuffered(0)
     , fCurrRLEByte(0)
     , fSampleX(1)
 {}
@@ -134,22 +131,8 @@ SkCodec::Result SkBmpRLECodec::onGetPixels(const SkImageInfo& dstInfo,
 }
 
 bool SkBmpRLECodec::initializeStreamBuffer() {
-    // Setup a buffer to contain the full input stream
-    // TODO (msarett): I'm not sure it is smart or optimal to trust fRLEBytes (read from header)
-    //                 as the size of our buffer.  First of all, the decode fails if fRLEBytes is
-    //                 corrupt (negative, zero, or small) when we might be able to decode
-    //                 successfully with a fixed size buffer.  Additionally, we would save memory
-    //                 using a fixed size buffer if the RLE encoding is large.  On the other hand,
-    //                 we may also waste memory with a fixed size buffer.  And determining a
-    //                 minimum size for our buffer would depend on the image width (so it's not
-    //                 really "fixed" size), and we may end up allocating a buffer that is
-    //                 generally larger than the average encoded size anyway.
-    size_t totalBytes = this->stream()->read(fStreamBuffer.get(), fRLEBytes);
-    if (totalBytes < fRLEBytes) {
-        fRLEBytes = totalBytes;
-        SkCodecPrintf("Warning: incomplete RLE file.\n");
-    }
-    if (fRLEBytes == 0) {
+    fBytesBuffered = this->stream()->read(fStreamBuffer, kBufferSize);
+    if (fBytesBuffered == 0) {
         SkCodecPrintf("Error: could not read RLE image data.\n");
         return false;
     }
@@ -158,15 +141,12 @@ 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();
+    const size_t remainingBytes = fBytesBuffered - fCurrRLEByte;
+    uint8_t* buffer = fStreamBuffer;
 
     // We will be reusing the same buffer, starting over from the beginning.
     // Move any remaining bytes to the start of the buffer.
@@ -185,11 +165,8 @@ size_t SkBmpRLECodec::checkForMoreData() {
     // 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;
+    fBytesBuffered = remainingBytes + additionalBytes;
+    return fBytesBuffered;
 }
 
 /*
@@ -294,7 +271,6 @@ SkCodec::Result SkBmpRLECodec::onPrepareToDecode(const SkImageInfo& dstInfo,
     copy_color_table(dstInfo, fColorTable.get(), inputColorPtr, inputColorCount);
 
     // Initialize a buffer for encoded RLE data
-    fRLEBytes = fOrigRLEBytes;
     if (!this->initializeStreamBuffer()) {
         SkCodecPrintf("Error: cannot initialize stream buffer.\n");
         return SkCodec::kInvalidInput;
@@ -392,8 +368,7 @@ int SkBmpRLECodec::decodeRLE(const SkImageInfo& dstInfo, void* dst, size_t dstRo
         }
 
         // Every entry takes at least two bytes
-        if ((int) fRLEBytes - fCurrRLEByte < 2) {
-            SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+        if ((int) fBytesBuffered - fCurrRLEByte < 2) {
             if (this->checkForMoreData() < 2) {
                 return y;
             }
@@ -403,8 +378,8 @@ int SkBmpRLECodec::decodeRLE(const SkImageInfo& dstInfo, void* dst, size_t dstRo
         // depending on their values.  In the first interpretation, the first
         // byte is an escape flag and the second byte indicates what special
         // task to perform.
-        const uint8_t flag = fStreamBuffer.get()[fCurrRLEByte++];
-        const uint8_t task = fStreamBuffer.get()[fCurrRLEByte++];
+        const uint8_t flag = fStreamBuffer[fCurrRLEByte++];
+        const uint8_t task = fStreamBuffer[fCurrRLEByte++];
 
         // Perform decoding
         if (RLE_ESCAPE == flag) {
@@ -417,15 +392,14 @@ int SkBmpRLECodec::decodeRLE(const SkImageInfo& dstInfo, void* dst, size_t dstRo
                     return height;
                 case RLE_DELTA: {
                     // Two bytes are needed to specify delta
-                    if ((int) fRLEBytes - fCurrRLEByte < 2) {
-                        SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+                    if ((int) fBytesBuffered - fCurrRLEByte < 2) {
                         if (this->checkForMoreData() < 2) {
                             return y;
                         }
                     }
                     // Modify x and y
-                    const uint8_t dx = fStreamBuffer.get()[fCurrRLEByte++];
-                    const uint8_t dy = fStreamBuffer.get()[fCurrRLEByte++];
+                    const uint8_t dx = fStreamBuffer[fCurrRLEByte++];
+                    const uint8_t dy = fStreamBuffer[fCurrRLEByte++];
                     x += dx;
                     y += dy;
                     if (x > width) {
@@ -451,11 +425,20 @@ int SkBmpRLECodec::decodeRLE(const SkImageInfo& dstInfo, void* dst, size_t dstRo
                         SkCodecPrintf("Warning: invalid RLE input.\n");
                         return y;
                     }
+
                     // Also abort if there are not enough bytes
                     // remaining in the stream to set numPixels.
-                    if ((int) fRLEBytes - fCurrRLEByte < SkAlign2(rowBytes)) {
-                        SkCodecPrintf("Warning: might be incomplete RLE input.\n");
-                        if (this->checkForMoreData() < SkAlign2(rowBytes)) {
+
+                    // At most, alignedRowBytes can be 255 (max uint8_t) *
+                    // 3 (max bytes per pixel) + 1 (aligned) = 766. If
+                    // fStreamBuffer was smaller than this,
+                    // checkForMoreData would never succeed for some bmps.
+                    static_assert(255 * 3 + 1 < kBufferSize,
+                                  "kBufferSize needs to be larger!");
+                    const size_t alignedRowBytes = SkAlign2(rowBytes);
+                    if ((int) fBytesBuffered - fCurrRLEByte < alignedRowBytes) {
+                        SkASSERT(alignedRowBytes < kBufferSize);
+                        if (this->checkForMoreData() < alignedRowBytes) {
                             return y;
                         }
                     }
@@ -463,8 +446,8 @@ int SkBmpRLECodec::decodeRLE(const SkImageInfo& dstInfo, void* dst, size_t dstRo
                     while (numPixels > 0) {
                         switch(this->bitsPerPixel()) {
                             case 4: {
-                                SkASSERT(fCurrRLEByte < fRLEBytes);
-                                uint8_t val = fStreamBuffer.get()[fCurrRLEByte++];
+                                SkASSERT(fCurrRLEByte < fBytesBuffered);
+                                uint8_t val = fStreamBuffer[fCurrRLEByte++];
                                 setPixel(dst, dstRowBytes, dstInfo, x++,
                                         y, val >> 4);
                                 numPixels--;
@@ -476,16 +459,16 @@ int SkBmpRLECodec::decodeRLE(const SkImageInfo& dstInfo, void* dst, size_t dstRo
                                 break;
                             }
                             case 8:
-                                SkASSERT(fCurrRLEByte < fRLEBytes);
+                                SkASSERT(fCurrRLEByte < fBytesBuffered);
                                 setPixel(dst, dstRowBytes, dstInfo, x++,
-                                        y, fStreamBuffer.get()[fCurrRLEByte++]);
+                                        y, fStreamBuffer[fCurrRLEByte++]);
                                 numPixels--;
                                 break;
                             case 24: {
-                                SkASSERT(fCurrRLEByte + 2 < fRLEBytes);
-                                uint8_t blue = fStreamBuffer.get()[fCurrRLEByte++];
-                                uint8_t green = fStreamBuffer.get()[fCurrRLEByte++];
-                                uint8_t red = fStreamBuffer.get()[fCurrRLEByte++];
+                                SkASSERT(fCurrRLEByte + 2 < fBytesBuffered);
+                                uint8_t blue = fStreamBuffer[fCurrRLEByte++];
+                                uint8_t green = fStreamBuffer[fCurrRLEByte++];
+                                uint8_t red = fStreamBuffer[fCurrRLEByte++];
                                 setRGBPixel(dst, dstRowBytes, dstInfo,
                                             x++, y, red, green, blue);
                                 numPixels--;
@@ -513,8 +496,7 @@ int SkBmpRLECodec::decodeRLE(const SkImageInfo& dstInfo, void* dst, size_t dstRo
                 // In RLE24, the second byte read is part of the pixel color.
                 // There are two more required bytes to finish encoding the
                 // color.
-                if ((int) fRLEBytes - fCurrRLEByte < 2) {
-                    SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+                if ((int) fBytesBuffered - fCurrRLEByte < 2) {
                     if (this->checkForMoreData() < 2) {
                         return y;
                     }
@@ -522,8 +504,8 @@ int SkBmpRLECodec::decodeRLE(const SkImageInfo& dstInfo, void* dst, size_t dstRo
 
                 // Fill the pixels up to endX with the specified color
                 uint8_t blue = task;
-                uint8_t green = fStreamBuffer.get()[fCurrRLEByte++];
-                uint8_t red = fStreamBuffer.get()[fCurrRLEByte++];
+                uint8_t green = fStreamBuffer[fCurrRLEByte++];
+                uint8_t red = fStreamBuffer[fCurrRLEByte++];
                 while (x < endX) {
                     setRGBPixel(dst, dstRowBytes, dstInfo, x++, y, red, green, blue);
                 }
diff --git a/src/codec/SkBmpRLECodec.h b/src/codec/SkBmpRLECodec.h
index 7cb3e9b292..8ea3a86dba 100644
--- a/src/codec/SkBmpRLECodec.h
+++ b/src/codec/SkBmpRLECodec.h
@@ -32,13 +32,10 @@ public:
      * @param offset the offset of the image pixel data from the end of the
      *               headers
      * @param rowOrder indicates whether rows are ordered top-down or bottom-up
-     * @param RLEBytes indicates the amount of data left in the stream
-     *                 after decoding the headers
      */
     SkBmpRLECodec(int width, int height, const SkEncodedInfo& info, SkStream* stream,
             uint16_t bitsPerPixel, uint32_t numColors, uint32_t bytesPerColor,
-            uint32_t offset, SkCodec::SkScanlineOrder rowOrder,
-            size_t RLEBytes);
+            uint32_t offset, SkCodec::SkScanlineOrder rowOrder);
 
     int setSampleX(int);
 
@@ -100,9 +97,11 @@ private:
     const uint32_t             fNumColors;
     const uint32_t             fBytesPerColor;
     const uint32_t             fOffset;
-    std::unique_ptr<uint8_t[]> fStreamBuffer;
-    size_t                     fRLEBytes;
-    const size_t               fOrigRLEBytes;
+
+    static constexpr size_t    kBufferSize = 4096;
+    uint8_t                    fStreamBuffer[kBufferSize];
+    size_t                     fBytesBuffered;
+
     uint32_t                   fCurrRLEByte;
     int                        fSampleX;
     std::unique_ptr<SkSampler> fSampler;
diff --git a/tests/CodecTest.cpp b/tests/CodecTest.cpp
index 0f6d54c8d7..8294c7a565 100644
--- a/tests/CodecTest.cpp
+++ b/tests/CodecTest.cpp
@@ -1448,6 +1448,18 @@ DEF_TEST(Codec_InvalidBmp, r) {
     REPORTER_ASSERT(r, !codec);
 }
 
+DEF_TEST(Codec_InvalidRLEBmp, r) {
+    auto* stream = GetResourceAsStream("invalid_images/b33251605.bmp");
+    if (!stream) {
+        return;
+    }
+
+    std::unique_ptr<SkCodec> codec(SkCodec::NewFromStream(stream));
+    REPORTER_ASSERT(r, codec);
+
+    test_info(r, codec.get(), codec->getInfo(), SkCodec::kIncompleteInput, nullptr);
+}
+
 DEF_TEST(Codec_InvalidAnimated, r) {
     // ASAN will complain if there is an issue.
     auto path = "invalid_images/skbug6046.gif";