Fixed more fuzzer issues

Fixed more fuzzer issues

- Added the "isAvailable" function to check how much bytes are remaining in the stream before doing potentially large mallocs. That way, we can signal a bad stream instead of crashing.
- Added data validation in SkImageInfo.cpp
- Added NULL pointer check in displacement
- Modified the fuzzer for randomized bitmap types

BUG=328934, 329254
Committed: http://code.google.com/p/skia/source/detail?r=12723

[reed1, 2013-12-16 21:53:13]

https://codereview.chromium.org/116773002/diff/1/include/core/SkFlattenableBu...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/116773002/diff/1/include/core/SkFlattenableBu...
include/core/SkFlattenableBuffers.h:172: virtual bool isAvailable(size_t size)
const = 0;
Do we really need this as a vritual? Do any of our readbuffers not know their
exact length?

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

https://codereview.chromium.org/116773002/diff/1/src/core/SkImageInfo.cpp#new...
src/core/SkImageInfo.cpp:11: static bool alpha_type_is_valid(SkAlphaType
alphaType) {
not sure this will always work. If you declare the type to be an enum, the
compiler may decide that it can trust you, and remove the <= last check.

I *think* the safe way is to read ints from the buffer, pass ints to the
validate helpers, and only cast them down to the enum value when writing to
this.

https://codereview.chromium.org/116773002/diff/1/src/effects/gradients/SkGrad...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/116773002/diff/1/src/effects/gradients/SkGrad...
src/effects/gradients/SkGradientShader.cpp:158: if
(buffer.validate(buffer.isAvailable(allocSize))) {
this double call pattern keeps appearing:

obj.validate(obj.isAvail())

can we combine these in a helper?

e.g.

if (buffer.validateAvailable(size)) { ...

[sugoi, 2013-12-17 15:55:45]

https://codereview.chromium.org/116773002/diff/1/include/core/SkFlattenableBu...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/116773002/diff/1/include/core/SkFlattenableBu...
include/core/SkFlattenableBuffers.h:172: virtual bool isAvailable(size_t size)
const = 0;
On 2013/12/16 21:53:13, reed1 wrote:
> Do we really need this as a virtual? Do any of our readbuffers not know their
> exact length?

They all know their exact length, but their length is provided by the SkReader32
member which is in each derived class, so I need some virtual functions
somewhere to access that data, unless I misunderstood something. Note that I'll
change this to another virtual function, validateAvailable, which will only be
implemented here and in SkValidatingReadBuffer, so this shouldn't be an issue
anymore.

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

https://codereview.chromium.org/116773002/diff/1/src/core/SkImageInfo.cpp#new...
src/core/SkImageInfo.cpp:11: static bool alpha_type_is_valid(SkAlphaType
alphaType) {
On 2013/12/16 21:53:13, reed1 wrote:
> not sure this will always work. If you declare the type to be an enum, the
> compiler may decide that it can trust you, and remove the <= last check.
> 
> I *think* the safe way is to read ints from the buffer, pass ints to the
> validate helpers, and only cast them down to the enum value when writing to
> this.

I can do that, but the examples I used, from SkValidationUtils.h, all have this
pattern, so I'd have to change these too, right ?

https://codereview.chromium.org/116773002/diff/1/src/effects/gradients/SkGrad...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/116773002/diff/1/src/effects/gradients/SkGrad...
src/effects/gradients/SkGradientShader.cpp:158: if
(buffer.validate(buffer.isAvailable(allocSize))) {
On 2013/12/16 21:53:13, reed1 wrote:
> this double call pattern keeps appearing:
> 
> obj.validate(obj.isAvail())
> 
> can we combine these in a helper?
> 
> e.g.
> 
> if (buffer.validateAvailable(size)) { ...

Will do

[reed1, 2013-12-17 16:38:05]

not a biggie, but perhaps the total length should just be provided to the
readbuffer in the constructor. Seems like a common thing to query, and I don't
think we support unknown-sized buffers.

[sugoi, 2013-12-17 18:52:05]

On 2013/12/17 16:38:05, reed1 wrote:
> not a biggie, but perhaps the total length should just be provided to the
> readbuffer in the constructor. Seems like a common thing to query, and I don't
> think we support unknown-sized buffers.

After some investigation, I'm not sure when we'd query the total size, we
usually care about the remaining (or available) size when we're reading (to make
sure we don't overflow).

[reed1, 2013-12-17 18:54:41]

On 2013/12/17 18:52:05, sugoi wrote:
> On 2013/12/17 16:38:05, reed1 wrote:
> > not a biggie, but perhaps the total length should just be provided to the
> > readbuffer in the constructor. Seems like a common thing to query, and I
don't
> > think we support unknown-sized buffers.
> 
> After some investigation, I'm not sure when we'd query the total size, we
> usually care about the remaining (or available) size when we're reading (to
make
> sure we don't overflow).

Ah. Do we not have bytesWritten available all the time? If not, then I agree
that available() is a good entrypoint.

[sugoi, 2013-12-17 19:22:47]

On 2013/12/17 18:54:41, reed1 wrote:
> On 2013/12/17 18:52:05, sugoi wrote:
> > On 2013/12/17 16:38:05, reed1 wrote:
> > > not a biggie, but perhaps the total length should just be provided to the
> > > readbuffer in the constructor. Seems like a common thing to query, and I
> don't
> > > think we support unknown-sized buffers.
> > 
> > After some investigation, I'm not sure when we'd query the total size, we
> > usually care about the remaining (or available) size when we're reading (to
> make
> > sure we don't overflow).
> 
> Ah. Do we not have bytesWritten available all the time? If not, then I agree
> that available() is a good entrypoint.

If I'm not mistaken, there's a bytesWritten in the writer classes, not in the
reader classes.

[reed1, 2013-12-17 19:25:15]

On 2013/12/17 19:22:47, sugoi wrote:
> On 2013/12/17 18:54:41, reed1 wrote:
> > On 2013/12/17 18:52:05, sugoi wrote:
> > > On 2013/12/17 16:38:05, reed1 wrote:
> > > > not a biggie, but perhaps the total length should just be provided to
the
> > > > readbuffer in the constructor. Seems like a common thing to query, and I
> > don't
> > > > think we support unknown-sized buffers.
> > > 
> > > After some investigation, I'm not sure when we'd query the total size, we
> > > usually care about the remaining (or available) size when we're reading
(to
> > make
> > > sure we don't overflow).
> > 
> > Ah. Do we not have bytesWritten available all the time? If not, then I agree
> > that available() is a good entrypoint.
> 
> If I'm not mistaken, there's a bytesWritten in the writer classes, not in the
> reader classes.

Right. Meant to say bytesRead

[sugoi, 2013-12-17 19:43:49]

On 2013/12/17 19:25:15, reed1 wrote:
> On 2013/12/17 19:22:47, sugoi wrote:
> > On 2013/12/17 18:54:41, reed1 wrote:
> > > On 2013/12/17 18:52:05, sugoi wrote:
> > > > On 2013/12/17 16:38:05, reed1 wrote:
> > > > > not a biggie, but perhaps the total length should just be provided to
> the
> > > > > readbuffer in the constructor. Seems like a common thing to query, and
I
> > > don't
> > > > > think we support unknown-sized buffers.
> > > > 
> > > > After some investigation, I'm not sure when we'd query the total size,
we
> > > > usually care about the remaining (or available) size when we're reading
> (to
> > > make
> > > > sure we don't overflow).
> > > 
> > > Ah. Do we not have bytesWritten available all the time? If not, then I
agree
> > > that available() is a good entrypoint.
> > 
> > If I'm not mistaken, there's a bytesWritten in the writer classes, not in
the
> > reader classes.
> 
> Right. Meant to say bytesRead

The closest thing that I know of is what's referred to as "offset()" in
SkOrderedReadBuffer, which is essentially the number of bytes read, but that
information is not available at the SkFlattenableReadBuffer level in the code as
it is now.

[reed1, 2013-12-17 19:46:58]

Thanks for exploring the offset/size path. I concede that available is the way
to go.

I think that making available() virtual is more generally useful than
validateAvailable(size). Does it make sense to you to do the following:

virtual size_t available();

bool validateAvailable(size_t amount) {
    return this->validate(amount <= this->available());
}

?

[sugoi, 2013-12-17 19:54:55]

On 2013/12/17 19:46:58, reed1 wrote:
> Thanks for exploring the offset/size path. I concede that available is the way
> to go.
> 
> I think that making available() virtual is more generally useful than
> validateAvailable(size). Does it make sense to you to do the following:
> 
> virtual size_t available();
> 
> bool validateAvailable(size_t amount) {
>     return this->validate(amount <= this->available());
> }
> 
> ?

Sure, will do.
The only thing I'd point out is that the default implementation of validate just
returns true, so, in that case, we'd evaluate "amount <= this->available()" even
though we wouldn't be using the result within the validate() function. If that's
not an issue, then I agree to do it this way.

[reed1, 2013-12-17 19:57:38]

On 2013/12/17 19:54:55, sugoi wrote:
> On 2013/12/17 19:46:58, reed1 wrote:
> > Thanks for exploring the offset/size path. I concede that available is the
way
> > to go.
> > 
> > I think that making available() virtual is more generally useful than
> > validateAvailable(size). Does it make sense to you to do the following:
> > 
> > virtual size_t available();
> > 
> > bool validateAvailable(size_t amount) {
> >     return this->validate(amount <= this->available());
> > }
> > 
> > ?
> 
> Sure, will do.
> The only thing I'd point out is that the default implementation of validate
just
> returns true, so, in that case, we'd evaluate "amount <= this->available()"
even
> though we wouldn't be using the result within the validate() function. If
that's
> not an issue, then I agree to do it this way.

Dang. Good point. Lets go with your current virtual validateAvailable() design.

[reed1, 2013-12-17 19:58:02]

lgtm

[commit-bot: I haz the power, 2013-12-17 20:01:46]

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

[commit-bot: I haz the power, 2013-12-17 20:49:55]

Change committed as 12723

[Stephen White, 2013-12-18 00:01:13]

https://codereview.chromium.org/116773002/diff/20001/src/core/SkColorTable.cpp
File src/core/SkColorTable.cpp (right):

https://codereview.chromium.org/116773002/diff/20001/src/core/SkColorTable.cp...
src/core/SkColorTable.cpp:95: if (buffer.validateAvailable(allocSize)) {
Not new to this patch, but I'm really starting to wonder if we should get rid of
these constructors, and replace them with factory functions that take an
SkFlattenableReadBuffer& instead. Then we could early-out and return NULL
whenever validation fails, and not worry about partially-initialized objects.

https://codereview.chromium.org/116773002/diff/20001/src/effects/SkDisplaceme...
File src/effects/SkDisplacementMapEffect.cpp (right):

https://codereview.chromium.org/116773002/diff/20001/src/effects/SkDisplaceme...
src/effects/SkDisplacementMapEffect.cpp:191: !displacementInput ||
!displacementInput->filterImage(proxy, src, ctm, &displ, offset)) {
I don't think it should abort on a NULL displacementInput. All NULL inputs
should just default to the source bitmap.

i.e., SkBitmap displ = src, color = src;

...

displacementInput && !displacementInput->filterImage(...)
  return false;