Fix bmp RLE "bug" and add invalid image test to CodexTest

src/codec/SkBmpRLECodec.cpp

Index: src/codec/SkBmpRLECodec.cpp
diff --git a/src/codec/SkBmpRLECodec.cpp b/src/codec/SkBmpRLECodec.cpp
index 828871cd547be98ef2b6bc537d2ec5c3b2b9e688..dec4d55c5c1aed891723b63e82a1015746745d0b 100644
--- a/src/codec/SkBmpRLECodec.cpp
+++ b/src/codec/SkBmpRLECodec.cpp
@@ -183,6 +183,32 @@ bool SkBmpRLECodec::initializeStreamBuffer() {
 }
 
 /*
+ * It is possible for an encoded image stream to contain more encoded data than
+ * it reports that it has.  Before signalling kIncompleteInput, we should check
+ * if we can reset the stream buffer with additional data.
+ */
+size_t SkBmpRLECodec::resetStreamBuffer() {
+    size_t remainingBytes = fRLEBytes - fCurrRLEByte;

[scroggo, 2015/08/07 13:35:35]

This can be const?

[msarett, 2015/08/11 22:35:02]

Yes, I have made it const.

+    uint8_t* buffer = fStreamBuffer.get();
+
+    // Store the remaining bytes to the start of our new buffer

[scroggo, 2015/08/07 13:35:35]

"new" buffer? It is the same buffer, correct?

[msarett, 2015/08/11 22:35:02]

Yes.  I have changed the comment to make this make sense.

+    for (uint32_t i = 0; i < remainingBytes; i++) {

[scroggo, 2015/08/07 13:35:35]

Why did you use uint32_t here? remainingBytes is a size_t, so this should
perhaps be a size_t as well?

[msarett, 2015/08/11 22:35:02]

It should be a size_t.  The loop is gone (using memmove now).

+        buffer[i] = buffer[fCurrRLEByte + i];

[scroggo, 2015/08/07 13:35:35]

Are these indices guaranteed to be safe/valid?

Also, could this be replaced with memcpy or memmove?

[msarett, 2015/08/11 22:35:03]

I think the indices are safe.  I have run through all of the edge cases I can
think of.  If the original buffer is sized very small (ex: 0, 1, 2...), this
function won't be of much use, but it should correctly return a small number and
then decode() will return kIncompleteInput.

Also, I'm replacing with memmove.

+    }
+
+    // Adjust the buffer ptr to the start of the data to be overwritten
+    buffer += remainingBytes;
+
+    // Try to read additional bytes from the stream
+    size_t additionalBytes = this->stream()->read(buffer, fRLEBytes - remainingBytes);

[scroggo, 2015/08/07 13:35:35]

Two thoughts: looking at your calculation above:

remainingBytes = fRLEBytes - fCurrRLEByte

it looks like the amount you want to read is just fCurrRLEByte?

It is also not obvious to me; why is this the right amount to read?

[msarett, 2015/08/11 22:35:02]

We will either read to the end of the stream or enough to fill the buffer.
The size of the buffer is fRLEBytes, which is *supposed* to be equal to the
number of encoded bytes.

Assuming there were leftover "remainingBytes" that we copied to the start of the
buffer, there is fCurrRLEBytes of open space left in the buffer.  You are right
that we can replace "fRLEBytes - remainingBytes" with "fCurrRLEByte", but it
actually makes more sense in my head when I think about it as originally
written.

We have already filled the buffer with "remainingBytes", so there are
"BUFFER_SIZE  - remainingBytes" bytes of space left to be filled.

+
+    // Update counters and return the number of bytes we currently have available
+    fCurrRLEByte = 0;
+    fRLEBytes = remainingBytes + additionalBytes;

[scroggo, 2015/08/07 13:35:35]

So this is now the total bytes that we have read from the stream? But it is no
longer the size of the buffer, which it used to be, right?

[msarett, 2015/08/11 22:35:02]

Yes.  fRLEBytes is no longer the size of the buffer, in the case that
stream->read() is unable to fill the buffer.  I *think* that this makes sense,
assuming that, if we were unable to fill the buffer, any subsequent call to read
will return 0.

Adding a comment to clarify this.

+    return fRLEBytes - fCurrRLEByte;

[scroggo, 2015/08/07 13:35:35]

fCurrRLEByte is 0, so this is just fRLEBytes, right?

And this is the number of bytes in the buffer? It seems like this number has
changed, but the size of the buffer has not?

[msarett, 2015/08/11 22:35:02]

Yes this is correct.

+}
+
+/*
  * Set an RLE pixel using the color table
  */
 void SkBmpRLECodec::setPixel(void* dst, size_t dstRowBytes,
@@ -287,8 +313,10 @@ 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");
-            return kIncompleteInput;
+            SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+            if (this->resetStreamBuffer() < 2) {
+                return kIncompleteInput;
+            }
         }
 
         // Read the next two bytes.  These bytes have different meanings
@@ -310,8 +338,10 @@ 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");
-                        return kIncompleteInput;
+                        SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+                        if (this->resetStreamBuffer() < 2) {
+                            return kIncompleteInput;
+                        }
                     }
                     // Modify x and y
                     const uint8_t dx = fStreamBuffer.get()[fCurrRLEByte++];
@@ -319,8 +349,8 @@ SkCodec::Result SkBmpRLECodec::decode(const SkImageInfo& dstInfo,
                     x += dx;
                     y += dy;
                     if (x > width || y > height) {
-                        SkCodecPrintf("Warning: invalid RLE input 1.\n");
-                        return kIncompleteInput;
+                        SkCodecPrintf("Warning: invalid RLE input.\n");
+                        return kInvalidInput;
                     }
                     break;
                 }
@@ -333,12 +363,18 @@ 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 ||
-                            (int) fRLEBytes - fCurrRLEByte < SkAlign2(rowBytes)) {
-                        SkCodecPrintf("Warning: invalid RLE input 2.\n");
-                        return kIncompleteInput;
+                    if ((int) fRLEBytes - fCurrRLEByte < SkAlign2(rowBytes)) {
+                        SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+                        if (this->resetStreamBuffer() < SkAlign2(rowBytes)) {
+                            return kIncompleteInput;
+                        }
                     }
                     // Set numPixels number of pixels
                     while (numPixels > 0) {
@@ -394,8 +430,10 @@ 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");
-                    return kIncompleteInput;
+                    SkCodecPrintf("Warning: might be incomplete RLE input.\n");
+                    if (this->resetStreamBuffer() < 2) {
+                        return kIncompleteInput;
+                    }
                 }
 
                 // Fill the pixels up to endX with the specified color