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>
diff --git a/resources/invalid_images/b33251605.bmp b/resources/invalid_images/b33251605.bmp
new file mode 100644
index 0000000..0060ff4
--- /dev/null
+++ b/resources/invalid_images/b33251605.bmp
Binary files differ
diff --git a/src/codec/SkBmpCodec.cpp b/src/codec/SkBmpCodec.cpp
index 83ba21b..66ad0ca 100644
--- a/src/codec/SkBmpCodec.cpp
+++ b/src/codec/SkBmpCodec.cpp
@@ -549,7 +549,6 @@
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 @@
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 c2574dd..1968616 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 @@
}
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 @@
}
/*
- * 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 @@
// 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 @@
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 @@
}
// 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 @@
// 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 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
// 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 7cb3e9b..8ea3a86 100644
--- a/src/codec/SkBmpRLECodec.h
+++ b/src/codec/SkBmpRLECodec.h
@@ -32,13 +32,10 @@
* @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 @@
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 0f6d54c..8294c7a 100644
--- a/tests/CodecTest.cpp
+++ b/tests/CodecTest.cpp
@@ -1448,6 +1448,18 @@
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";
