Fixed issue found by clusterfuzz

Fixed issue found by clusterfuzz

An integer overflow was causing an issue when reading a string with a very large (or negative) size.

BUG=367764
Committed: http://code.google.com/p/skia/source/detail?r=14434

[Stephen White, 2014-04-28 19:48:32]

https://codereview.chromium.org/255693003/diff/1/src/core/SkValidatingReadBuf...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/255693003/diff/1/src/core/SkValidatingReadBuf...
src/core/SkValidatingReadBuffer.cpp:94: const int32_t len32 = this->readInt();
I think we should be reading a uint32_t here (since we write one in
SkWriter32::writeString). Then it would be impossible to sign-extend this badly
into 64 bits.

[sugoi1, 2014-04-28 20:15:20]

https://codereview.chromium.org/255693003/diff/1/src/core/SkValidatingReadBuf...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/255693003/diff/1/src/core/SkValidatingReadBuf...
src/core/SkValidatingReadBuffer.cpp:94: const int32_t len32 = this->readInt();
On 2014/04/28 19:48:33, Stephen White wrote:
> I think we should be reading a uint32_t here (since we write one in
> SkWriter32::writeString). Then it would be impossible to sign-extend this
badly
> into 64 bits.

Done.

https://codereview.chromium.org/255693003/diff/20001/include/core/SkReader32.h
File include/core/SkReader32.h (left):

https://codereview.chromium.org/255693003/diff/20001/include/core/SkReader32....
include/core/SkReader32.h:43: bool isAvailable(size_t size) const { return fCurr
+ size <= fStop; }
"fCurr + size" can overflow easily since "size" can be a value read directly
from the stream, so any very large value will cause the integer overflow (sizes
are generally stored as 32 bit integers in the stream, so a 32 bit OS is more
likely to have this issue than a 64 bit OS). To avoid integer overflow, compare
size to available() instead, since fStop and fCurr are internal values to the
class and unlikely to be invalid.

https://codereview.chromium.org/255693003/diff/20001/src/core/SkValidatingRea...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/255693003/diff/20001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:94: const size_t len = this->readUInt();
Using readUInt() helps 64 bit platforms, since a "negative" int can no longer
turn into a very large 64 bit value, so an integer overflow is much less likely
on these platforms. For 32 bit platforms, see the fix to
SkReader32::isAvailable() (which would also fix the issue for 64 bit platforms).

[reed1, 2014-04-28 20:18:02]

https://codereview.chromium.org/255693003/diff/20001/include/core/SkReader32.h
File include/core/SkReader32.h (right):

https://codereview.chromium.org/255693003/diff/20001/include/core/SkReader32....
include/core/SkReader32.h:43: bool isAvailable(size_t size) const { return size
<= available(); }
nit: this->available()

https://codereview.chromium.org/255693003/diff/20001/src/core/SkValidatingRea...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/255693003/diff/20001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:94: const size_t len = this->readUInt();
Why is it readUInt() here, and readU32() in Reader.cpp?

[sugoi1, 2014-04-28 20:25:05]

https://codereview.chromium.org/255693003/diff/20001/include/core/SkReader32.h
File include/core/SkReader32.h (right):

https://codereview.chromium.org/255693003/diff/20001/include/core/SkReader32....
include/core/SkReader32.h:43: bool isAvailable(size_t size) const { return size
<= available(); }
On 2014/04/28 20:18:02, reed1 wrote:
> nit: this->available()

Done.

https://codereview.chromium.org/255693003/diff/20001/src/core/SkValidatingRea...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/255693003/diff/20001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:94: const size_t len = this->readUInt();
On 2014/04/28 20:18:02, reed1 wrote:
> Why is it readUInt() here, and readU32() in Reader.cpp?

I don't know the reason. I can see that SkReadBuffer::readUInt() calls
fReader.readU32() and it's probably been that way for a while.

[Stephen White, 2014-04-28 20:36:06]

LGTM, but will leave for Mike.

[reed1, 2014-04-28 20:49:52]

Lets make the both readU32() now, or file a bug so we don't forget. This seems
like a dangerous/misleading diff between reader and validatingreader.

[reed1, 2014-04-28 20:50:10]

lgtm w/ previous request

[sugoi1, 2014-04-29 14:17:10]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-04-29 14:17:28]

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

[borenet, 2014-04-29 15:28:44]

The CQ bit was unchecked by borenet@google.com

[borenet, 2014-04-29 15:28:51]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2014-04-29 15:29:09]

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

[commit-bot: I haz the power, 2014-04-29 15:35:36]

Change committed as 14434