Use fixed size buffer for RLE bmps DO NOT MERGE

Cherry-pick of b3b24538e02ead0c3f5bc528818982475890efd6

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 5616a650a6..5274ec4325 100644
--- a/src/codec/SkBmpCodec.cpp
+++ b/src/codec/SkBmpCodec.cpp
@@ -459,7 +459,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;
 
     // Calculate the number of bytes read so far
     const uint32_t bytesRead = kBmpHeaderBytes + infoBytes + maskBytes;
@@ -519,7 +518,7 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) {
                 // Icos skip the header that contains totalBytes.
                 SkASSERT(!inIco);
                 *codecOut = new SkBmpRLECodec(imageInfo, stream, bitsPerPixel, numColors,
-                        bytesPerColor, offset - bytesRead, rowOrder, RLEBytes);
+                        bytesPerColor, offset - bytesRead, rowOrder);
                 return true;
             default:
                 SkASSERT(false);
diff --git a/src/codec/SkBmpRLECodec.cpp b/src/codec/SkBmpRLECodec.cpp
index 793bbfd260..2b59485b29 100644
--- a/src/codec/SkBmpRLECodec.cpp
+++ b/src/codec/SkBmpRLECodec.cpp
@@ -17,16 +17,13 @@
 SkBmpRLECodec::SkBmpRLECodec(const SkImageInfo& 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(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)
 {}
@@ -137,22 +134,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;
     }
@@ -161,15 +144,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.
@@ -188,11 +168,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;
 }
 
 /*
@@ -284,7 +261,6 @@ SkCodec::Result SkBmpRLECodec::prepareToDecode(const SkImageInfo& dstInfo,
     copy_color_table(dstInfo, this->fColorTable, inputColorPtr, inputColorCount);
 
     // Initialize a buffer for encoded RLE data
-    fRLEBytes = fOrigRLEBytes;
     if (!this->initializeStreamBuffer()) {
         SkCodecPrintf("Error: cannot initialize stream buffer.\n");
         return SkCodec::kInvalidInput;
@@ -346,8 +322,7 @@ int SkBmpRLECodec::decodeRows(const SkImageInfo& info, void* dst, size_t dstRowB
         }
 
         // 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;
             }
@@ -357,8 +332,8 @@ int SkBmpRLECodec::decodeRows(const SkImageInfo& info, void* dst, size_t dstRowB
         // 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) {
@@ -371,15 +346,14 @@ int SkBmpRLECodec::decodeRows(const SkImageInfo& info, void* dst, size_t dstRowB
                     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) {
@@ -405,11 +379,20 @@ int SkBmpRLECodec::decodeRows(const SkImageInfo& info, void* dst, size_t dstRowB
                         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;
                         }
                     }
@@ -417,8 +400,8 @@ int SkBmpRLECodec::decodeRows(const SkImageInfo& info, void* dst, size_t dstRowB
                     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--;
@@ -430,16 +413,16 @@ int SkBmpRLECodec::decodeRows(const SkImageInfo& info, void* dst, size_t dstRowB
                                 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--;
@@ -466,8 +449,7 @@ int SkBmpRLECodec::decodeRows(const SkImageInfo& info, void* dst, size_t dstRowB
                 // 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;
                     }
@@ -475,8 +457,8 @@ int SkBmpRLECodec::decodeRows(const SkImageInfo& info, void* dst, size_t dstRowB
 
                 // 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 2ddf8d8b90..a44ade88a3 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(const SkImageInfo& srcInfo, 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);
 
@@ -99,9 +96,11 @@ private:
     const uint32_t                      fNumColors;
     const uint32_t                      fBytesPerColor;
     const uint32_t                      fOffset;
-    SkAutoTDeleteArray<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;
     SkAutoTDelete<SkSampler>            fSampler;
diff --git a/tests/CodexTest.cpp b/tests/CodexTest.cpp
index 7e6d950430..ead679544f 100644
--- a/tests/CodexTest.cpp
+++ b/tests/CodexTest.cpp
@@ -1003,3 +1003,15 @@ DEF_TEST(Codec_jpeg_rewind, r) {
     SkCodec::Result result = codec->getPixels(codec->getInfo(), pixelStorage.get(), rowBytes);
     REPORTER_ASSERT(r, SkCodec::kSuccess == result);
 }
+
+DEF_TEST(Codec_InvalidRLEBmp, r) {
+    auto* stream = GetResourceAsStream("invalid_images/b33251605.bmp");
+    if (!stream) {
+        return;
+    }
+
+    SkAutoTDelete<SkCodec> codec(SkCodec::NewFromStream(stream));
+    REPORTER_ASSERT(r, codec);
+
+    test_info(r, codec.get(), codec->getInfo(), SkCodec::kIncompleteInput, nullptr);
+}