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

Fix bmp RLE "bug"

Chromium's test suite contains an RLE image that reports a certain
file size in the header, but then contains additional encoded data.
Our bmp decoder would only decode half of the image, before stopping.

With this fix, we check for additional data before returning
kIncompleteInput.

If this lands, I will upload the test image to the bots.

Also adding an invalid image test to CodexTest.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/d0375bc4607f9d3f2ec427771e90ec7d284c174d

[msarett, 2015-08-06 22:41:23]

msarett@google.com changed reviewers:
+ emmaleer@google.com, scroggo@google.com

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

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:191: size_t remainingBytes = fRLEBytes -
fCurrRLEByte;
This can be const?

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:194: // Store the remaining bytes to the start of
our new buffer
"new" buffer? It is the same buffer, correct?

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:195: for (uint32_t i = 0; i < remainingBytes; i++) {
Why did you use uint32_t here? remainingBytes is a size_t, so this should
perhaps be a size_t as well?

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:196: buffer[i] = buffer[fCurrRLEByte + i];
Are these indices guaranteed to be safe/valid?

Also, could this be replaced with memcpy or memmove?

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:203: size_t additionalBytes =
this->stream()->read(buffer, fRLEBytes - remainingBytes);
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?

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:207: fRLEBytes = remainingBytes + additionalBytes;
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?

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:208: return fRLEBytes - fCurrRLEByte;
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:03]

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:191: size_t remainingBytes = fRLEBytes -
fCurrRLEByte;
On 2015/08/07 13:35:35, scroggo wrote:
> This can be const?

Yes, I have made it const.

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:194: // Store the remaining bytes to the start of
our new buffer
On 2015/08/07 13:35:35, scroggo wrote:
> "new" buffer? It is the same buffer, correct?

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

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:195: for (uint32_t i = 0; i < remainingBytes; i++) {
On 2015/08/07 13:35:35, scroggo wrote:
> Why did you use uint32_t here? remainingBytes is a size_t, so this should
> perhaps be a size_t as well?

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

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:196: buffer[i] = buffer[fCurrRLEByte + i];
On 2015/08/07 13:35:35, scroggo wrote:
> Are these indices guaranteed to be safe/valid?
> 
> Also, could this be replaced with memcpy or memmove?

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.

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:203: size_t additionalBytes =
this->stream()->read(buffer, fRLEBytes - remainingBytes);
On 2015/08/07 13:35:35, scroggo wrote:
> 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?

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.

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:207: fRLEBytes = remainingBytes + additionalBytes;
On 2015/08/07 13:35:35, scroggo wrote:
> 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?

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.

https://codereview.chromium.org/1273853004/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:208: return fRLEBytes - fCurrRLEByte;
On 2015/08/07 13:35:35, scroggo wrote:
> 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?

Yes this is correct.

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:480: if (inIco) {
Turns out this is reachable and we have a test now!

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:500: SkASSERT(!inIco);
I discovered that this is unreachable when trying to hack a test image.

[scroggo, 2015-08-12 14:14:30]

What are the "final nits from 1258863008"? Anything that should be added to the
change description?

lgtm

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:497: // This line should be unreachable, since we
require that
nit: this line is reachable - it just shouldn't be reached if inIco is true.

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpRLECodec...
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpRLECodec...
src/codec/SkBmpRLECodec.cpp:197: // src memory will overlap in corrupt images.
nit: and* src ...

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpRLECodec.h
File src/codec/SkBmpRLECodec.h (right):

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpRLECodec...
src/codec/SkBmpRLECodec.h:60: * it reports that it has.  Before signalling
kIncompleteInput, we should check
Maybe this should say something like:

Before signaling kIncompleteInput, we call this method to attempt to load the
stream buffer with any additional data.

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpRLECodec...
src/codec/SkBmpRLECodec.h:63: * @return the number of bytes remaining in the new
stream buffer
nit: In my mind, the stream buffer itself is not new - just the data

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpRLECodec...
src/codec/SkBmpRLECodec.h:65: size_t resetStreamBuffer();
I'm not a big fan of the name. It doesn't really explain its purpose. Maybe it
should be something like checkForMoreData?or checkForExtraData?

https://codereview.chromium.org/1273853004/diff/20001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1273853004/diff/20001/tests/CodexTest.cpp#new...
tests/CodexTest.cpp:292: test_invalid(r, "invalid_mask-bmp_ico.ico");
Can you move this file into resources/invalid_images? That way we can run:

  dm --src image --images resources

and expect it to succeed.

I do not feel strongly about the name, but invalid will then be redundant.

[msarett, 2015-08-12 15:01:25]

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpCodec.cpp
File src/codec/SkBmpCodec.cpp (right):

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:497: // This line should be unreachable, since we
require that
On 2015/08/12 14:14:30, scroggo wrote:
> nit: this line is reachable - it just shouldn't be reached if inIco is true.

Haha yep.  Fixing the comment.

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpRLECodec...
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpRLECodec...
src/codec/SkBmpRLECodec.cpp:197: // src memory will overlap in corrupt images.
On 2015/08/12 14:14:30, scroggo wrote:
> nit: and* src ...

Done.

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpRLECodec.h
File src/codec/SkBmpRLECodec.h (right):

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpRLECodec...
src/codec/SkBmpRLECodec.h:60: * it reports that it has.  Before signalling
kIncompleteInput, we should check
On 2015/08/12 14:14:30, scroggo wrote:
> Maybe this should say something like:
> 
> Before signaling kIncompleteInput, we call this method to attempt to load the
> stream buffer with any additional data.

Agreed.  Done.

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpRLECodec...
src/codec/SkBmpRLECodec.h:63: * @return the number of bytes remaining in the new
stream buffer
On 2015/08/12 14:14:30, scroggo wrote:
> nit: In my mind, the stream buffer itself is not new - just the data

Yeah agreed.  I need the remove the work "new" from all of my comments.

https://codereview.chromium.org/1273853004/diff/20001/src/codec/SkBmpRLECodec...
src/codec/SkBmpRLECodec.h:65: size_t resetStreamBuffer();
On 2015/08/12 14:14:30, scroggo wrote:
> I'm not a big fan of the name. It doesn't really explain its purpose. Maybe it
> should be something like checkForMoreData?or checkForExtraData?

I like checkForMoreData.

https://codereview.chromium.org/1273853004/diff/20001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1273853004/diff/20001/tests/CodexTest.cpp#new...
tests/CodexTest.cpp:292: test_invalid(r, "invalid_mask-bmp_ico.ico");
On 2015/08/12 14:14:30, scroggo wrote:
> Can you move this file into resources/invalid_images? That way we can run:
> 
>   dm --src image --images resources
> 
> and expect it to succeed.
> 
> I do not feel strongly about the name, but invalid will then be redundant.

Done.

[msarett, 2015-08-12 15:01:55]

The CQ bit was checked by msarett@google.com

[msarett, 2015-08-12 15:01:55]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1273853004/#ps60001
(title: "Response to comments")

[commit-bot: I haz the power, 2015-08-12 15:02:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273853004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273853004/60001

[commit-bot: I haz the power, 2015-08-12 15:09:00]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/d0375bc4607f9d3f2ec427771e90ec7d284c174d