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

src/codec/SkBmpRLECodec.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkBmpRLECodec.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
@@ skipping 165 matching lines @@
         SkCodecPrintf("Warning: incomplete RLE file.\n");
     }
     if (fRLEBytes == 0) {
         SkCodecPrintf("Error: could not read RLE image data.\n");
         return false;
     }
     return true;
 }
 
 /*
+ * 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,
                              const SkImageInfo& dstInfo, uint32_t x, uint32_t y,
                              uint8_t index) {
     // Set the row
     int height = dstInfo.height();
     int row;
     if (SkBmpCodec::kBottomUp_RowOrder == this->rowOrder()) {
         row = height - y - 1;
@@ skipping 84 matching lines @@
         // succeeded.
         if (y >= height) {
             // It would be better to check for the EOF marker before returning
             // success, but we may be performing a scanline decode, which
             // may require us to stop before decoding the full height.
             return kSuccess;
         }
 
         // 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
         // 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++];
 
         // Perform decoding
         if (RLE_ESCAPE == flag) {
             switch (task) {
                 case RLE_EOL:
                     x = 0;
                     y++;
                     break;
                 case RLE_EOF:
                     return kSuccess;
                 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++];
                     const uint8_t dy = fStreamBuffer.get()[fCurrRLEByte++];
                     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;
                 }
                 default: {
                     // If task does not match any of the above signals, it
                     // indicates that we have a sequence of non-RLE pixels.
                     // Furthermore, the value of task is equal to the number
                     // of pixels to interpret.
                     uint8_t numPixels = task;
                     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) {
                         switch(this->bitsPerPixel()) {
                             case 4: {
                                 SkASSERT(fCurrRLEByte < fRLEBytes);
                                 uint8_t val = fStreamBuffer.get()[fCurrRLEByte++];
                                 setPixel(dst, dstRowBytes, dstInfo, x++,
                                         y, val >> 4);
                                 numPixels--;
@@ skipping 35 matching lines @@
             // If the first byte read is not a flag, it indicates the number of
             // pixels to set in RLE mode.
             const uint8_t numPixels = flag;
             const int endX = SkTMin<int>(x + numPixels, width);
 
             if (24 == this->bitsPerPixel()) {
                 // 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: 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
                 uint8_t blue = task;
                 uint8_t green = fStreamBuffer.get()[fCurrRLEByte++];
                 uint8_t red = fStreamBuffer.get()[fCurrRLEByte++];
                 while (x < endX) {
                     setRGBPixel(dst, dstRowBytes, dstInfo, x++, y, red,
                             green, blue);
                 }
@@ skipping 12 matching lines @@
                 // Set the indicated number of pixels
                 for (int which = 0; x < endX; x++) {
                     setPixel(dst, dstRowBytes, dstInfo, x, y,
                             indices[which]);
                     which = !which;
                 }
             }
         }
     }
 }