Use fixed size buffer for RLE bmps DO NOT MERGE am: 318e3505ac am: 89f3cdc1be am: 7da5c5809c
am: beb907e507
Change-Id: I2c2f6167fd214c7ae2e3c019787c185b9afe3015
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 5616a65..5274ec4 100644
--- a/src/codec/SkBmpCodec.cpp
+++ b/src/codec/SkBmpCodec.cpp
@@ -459,7 +459,6 @@
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 @@
// 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 793bbfd..2b59485 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 @@
}
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 @@
}
/*
- * 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 @@
// 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 @@
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 @@
}
// 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 @@
// 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 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
// 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 2ddf8d8..a44ade8 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(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 @@
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 7e6d950..ead6795 100644
--- a/tests/CodexTest.cpp
+++ b/tests/CodexTest.cpp
@@ -1003,3 +1003,15 @@
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);
+}
