Fixing another clusterfuzz issue

Fixing another clusterfuzz issue

This was introduced by removing SkValidatingReadBuffer::readBitmap in https://codereview.chromium.org/295793002/

Since SkReadBuffer::skip wasn't virtual, it was using the unsafe SkReadBuffer::skip within SkReadBuffer::readBitmap rather than using SkValidatingReadBuffer::skip. I also removed direct uses of fReader within SkReadBuffer::readBitmap so that it can use the virtual readInt / readFixed functions that have a version in SkValidatingReadBuffer.

Also, I changed SkReadBuffer::readPoint so that it uses the virtual readScalar, that way, it becomes redundant with SkValidatingReadBuffer::readPoint, which can then be removed.

BUG=380723
Committed: https://skia.googlesource.com/skia/+/0951fe12984944406e0f1bb105b9fa4c54fcdcdd

[reed1, 2014-06-05 12:33:05]

What is the minimal change needed to fix the fuzzer? Is it just making skip
virtual/overridden?

I'm less clear with this CL about the direction/plan for validation -vs-
non-validation. for instance, this CL wants us to use a validating read for each
x,y in a point... but we still return something to the caller even if there is
an error.

I'm glad you identified the bug from the fuzzer. I'd prefer that we take the big
change in two steps if possible.

1. the minimal change that fixes the fuzzer
2. lets meet again with klein to talk about the larger change/direction

https://codereview.chromium.org/317003003/diff/1/src/core/SkReadBuffer.cpp
File src/core/SkReadBuffer.cpp (right):

https://codereview.chromium.org/317003003/diff/1/src/core/SkReadBuffer.cpp#ne...
src/core/SkReadBuffer.cpp:226: const int32_t xOffset = this->readFixed();
These are ints, not fixed.

[sugoi, 2014-06-05 12:42:56]

On 2014/06/05 12:33:05, reed1 wrote:
> What is the minimal change needed to fix the fuzzer? Is it just making skip
> virtual/overridden?

Just making skip virtual will for the issue, but if I don't use the validating
read...() functions, I can't prevent this operation from reading out of bounds,
which is not the bug found by the fuzzer, but that's still an issue.

> I'm less clear with this CL about the direction/plan for validation -vs-
> non-validation. for instance, this CL wants us to use a validating read for
each
> x,y in a point... but we still return something to the caller even if there is
> an error.

As long as the reader knows that it is in an error state, this should be
gracefully recoverable.

> I'm glad you identified the bug from the fuzzer. I'd prefer that we take the
big
> change in two steps if possible.
> 
> 1. the minimal change that fixes the fuzzer
> 2. lets meet again with klein to talk about the larger change/direction

I can fix that particular bug with just the virtual skip function, but that's
just because the fuzzer hasn't caught the out-of-bounds memory accesses that the
non safe readInt() could allow, so I think this should also be fixed now. I can
remove the readPoint() change, that was just to make some more code common
between SkReadBuffer and SkValidatingReadBuffer, but that's not part of the fix.

[sugoi, 2014-06-05 12:43:15]

https://codereview.chromium.org/317003003/diff/1/src/core/SkReadBuffer.cpp
File src/core/SkReadBuffer.cpp (right):

https://codereview.chromium.org/317003003/diff/1/src/core/SkReadBuffer.cpp#ne...
src/core/SkReadBuffer.cpp:226: const int32_t xOffset = this->readFixed();
On 2014/06/05 12:33:05, reed1 wrote:
> These are ints, not fixed.

readS32() is equivalent to readFixed(), so if I use readInt(), that'll change
the reading function, unless readS32() was wrong to begin with.

[mtklein, 2014-06-05 13:06:06]

It does seem we've gotten ourselves into a fragile state if we're having to
worry about the correctness of SkValidatingReadBuffer inside SkReadBuffer (e.g.
don't call fReader, call this->something() instead).  The changes in
SkReadBuffer::readPoint() look like a no-op out of context, and I don't think
anyone would blink at at CL that undid the change you've got here, particularly
if no unit tests start fail when they do.

Not that any of this should stop you here with this CL.  This problem is
structural, falling under that larger change/direction topic we should talk
about.  I think it might be time that we graduate SkValidatingReadBuffer,
merging it with SkReadBuffer.

[sugoi1, 2014-06-05 14:00:37]

On 2014/06/05 13:06:06, mtklein wrote:
> It does seem we've gotten ourselves into a fragile state if we're having to
> worry about the correctness of SkValidatingReadBuffer inside SkReadBuffer
(e.g.
> don't call fReader, call this->something() instead).  The changes in
> SkReadBuffer::readPoint() look like a no-op out of context, and I don't think
> anyone would blink at at CL that undid the change you've got here,
particularly
> if no unit tests start fail when they do.
> 
> Not that any of this should stop you here with this CL.  This problem is
> structural, falling under that larger change/direction topic we should talk
> about.  I think it might be time that we graduate SkValidatingReadBuffer,
> merging it with SkReadBuffer.

I removed the readPoint() change to keep only the readBitmap() fix. We can VC to
discuss the future of readers whenever you want.

[sugoi1, 2014-06-06 12:57:53]

Ping.

[reed1, 2014-06-06 13:01:26]

where is the impl for validatingbuffer::skip() ?

[sugoi1, 2014-06-06 13:03:09]

On 2014/06/06 13:01:26, reed1 wrote:
> where is the impl for validatingbuffer::skip() ?

It's in SkValidatingReadBuffer.cpp. It already existed and hasn't changed.

[sugoi1, 2014-06-06 13:04:25]

On 2014/06/06 13:03:09, sugoi1 wrote:
> On 2014/06/06 13:01:26, reed1 wrote:
> > where is the impl for validatingbuffer::skip() ?
> 
> It's in SkValidatingReadBuffer.cpp. It already existed and hasn't changed.

You can see it here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...

[reed1, 2014-06-06 13:05:03]

On 2014/06/06 13:03:09, sugoi1 wrote:
> On 2014/06/06 13:01:26, reed1 wrote:
> > where is the impl for validatingbuffer::skip() ?
> 
> It's in SkValidatingReadBuffer.cpp. It already existed and hasn't changed.

Ah, it was "hiding" the base class version. wacky.

lgtm

[sugoi1, 2014-06-06 13:05:55]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-06-06 13:06:56]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/317003003/20001

[commit-bot: I haz the power, 2014-06-06 13:44:20]

Change committed as 0951fe12984944406e0f1bb105b9fa4c54fcdcdd