Fixed a few places where uninitialized memory could have been read

Fixed a few places where uninitialized memory could have been read

Also added early exit in SkImageFilter's constructor to avoid attempting to deserialize all inputs once a bad input has been found. This avoids hanging if a filter pretends to have 1 billion inputs when that's just an error on the number of inputs read by the filter.

BUG=326206, 326197, 326229
Committed: http://code.google.com/p/skia/source/detail?r=12544

[sugoi1, 2013-12-05 18:45:47]

https://codereview.chromium.org/106943002/diff/1/samplecode/SampleFilterFuzz.cpp
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/106943002/diff/1/samplecode/SampleFilterFuzz....
samplecode/SampleFilterFuzz.cpp:35: //#define SK_FUZZER_IS_VERBOSE
I got tired of always writing these, it's easier to uncomment them :)

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

https://codereview.chromium.org/106943002/diff/1/src/core/SkImageFilter.cpp#n...
src/core/SkImageFilter.cpp:73: if (buffer.validate(true) &&
buffer.validate(SkIsValidRect(rect))) {
This extra validate(true) makes sure that we don't read uninitialized memory in
SkIsValidRect.

https://codereview.chromium.org/106943002/diff/1/src/effects/SkColorFilters.cpp
File src/effects/SkColorFilters.cpp (right):

https://codereview.chromium.org/106943002/diff/1/src/effects/SkColorFilters.c...
src/effects/SkColorFilters.cpp:104: if (buffer.validate(true)) {
Don't update the cache if there's a deserialization error

https://codereview.chromium.org/106943002/diff/1/src/effects/SkColorMatrixFil...
File src/effects/SkColorMatrixFilter.cpp (right):

https://codereview.chromium.org/106943002/diff/1/src/effects/SkColorMatrixFil...
src/effects/SkColorMatrixFilter.cpp:311: if
(buffer.readScalarArray(fMatrix.fMat, 20)) {
Don't initialize the state if there's a deserialization error

https://codereview.chromium.org/106943002/diff/1/src/effects/SkMergeImageFilt...
File src/effects/SkMergeImageFilter.cpp (right):

https://codereview.chromium.org/106943002/diff/1/src/effects/SkMergeImageFilt...
src/effects/SkMergeImageFilter.cpp:168: if (buffer.readByteArray(fModes, size))
{
Don't read each individual fModes[i] element if reading fModes failed.

https://codereview.chromium.org/106943002/diff/1/src/effects/SkTileImageFilte...
File src/effects/SkTileImageFilter.cpp (right):

https://codereview.chromium.org/106943002/diff/1/src/effects/SkTileImageFilte...
src/effects/SkTileImageFilter.cpp:64: buffer.validate(buffer.validate(true) &&
SkIsValidRect(fSrcRect) && SkIsValidRect(fDstRect));
This extra validate(true) makes sure that we don't read uninitialized memory in
SkIsValidRect.

[Stephen White, 2013-12-05 20:48:22]

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

https://codereview.chromium.org/106943002/diff/1/src/core/SkBitmap.cpp#newcod...
src/core/SkBitmap.cpp:1562: buffer.validate(this->setConfig(config, width,
height, rowBytes, alphaType));
Nit: this might be clearer if the result of setConfig() was stored in a
temporary, and we buffer.validate() that. Maybe someone else could chime in. My
thought was that the intent of the setConfig() was a little hidden.

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

https://codereview.chromium.org/106943002/diff/1/src/core/SkImageFilter.cpp#n...
src/core/SkImageFilter.cpp:73: if (buffer.validate(true) &&
buffer.validate(SkIsValidRect(rect))) {
On 2013/12/05 18:45:47, sugoi1 wrote:
> This extra validate(true) makes sure that we don't read uninitialized memory
in
> SkIsValidRect.

This is ok, but I'd still prefer we had some kind of buffer.isValid() to make
the intent clearer. Could wait for a followup, I suppose. Either way, a comment
might help too.

https://codereview.chromium.org/106943002/diff/1/src/effects/SkColorFilters.cpp
File src/effects/SkColorFilters.cpp (right):

https://codereview.chromium.org/106943002/diff/1/src/effects/SkColorFilters.c...
src/effects/SkColorFilters.cpp:104: if (buffer.validate(true)) {
On 2013/12/05 18:45:47, sugoi1 wrote:
> Don't update the cache if there's a deserialization error

Same here.

[reed1, 2013-12-05 21:35:22]

https://codereview.chromium.org/106943002/diff/60001/include/core/SkFlattenab...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/106943002/diff/60001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:156: virtual bool isValid() const { return
true; }
Please add some dox. What is valid? When is it not valid?

https://codereview.chromium.org/106943002/diff/60001/samplecode/SampleFilterF...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/106943002/diff/60001/samplecode/SampleFilterF...
samplecode/SampleFilterFuzz.cpp:34: //#define SK_ADD_RANDOM_BIT_FLIPS
Can we remove these?

https://codereview.chromium.org/106943002/diff/60001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/106943002/diff/60001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:66: if (!buffer.isValid()) {
Shouldn't we just abort everything if we detect the buffer is invalid?
sk_throw()? fInputCount = 0?

https://codereview.chromium.org/106943002/diff/60001/src/effects/SkColorMatri...
File src/effects/SkColorMatrixFilter.cpp (right):

https://codereview.chromium.org/106943002/diff/60001/src/effects/SkColorMatri...
src/effects/SkColorMatrixFilter.cpp:314:
buffer.validate(SkScalarIsFinite(fMatrix.fMat[i]));
Is this really invalid? Do we check for this sort of thing in our normal
constructor? Would we every crash with infinities?

[sugoi, 2013-12-05 23:27:49]

https://codereview.chromium.org/106943002/diff/60001/samplecode/SampleFilterF...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/106943002/diff/60001/samplecode/SampleFilterF...
samplecode/SampleFilterFuzz.cpp:34: //#define SK_ADD_RANDOM_BIT_FLIPS
On 2013/12/05 21:35:23, reed1 wrote:
> Can we remove these?

I just added these. I type them every time I enable special modes in the fuzzer,
so I thought it would be simpler to uncomment them when I need them. Do you mean
that these should always be enabled (or enabled by default) ?

https://codereview.chromium.org/106943002/diff/60001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/106943002/diff/60001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:66: if (!buffer.isValid()) {
On 2013/12/05 21:35:23, reed1 wrote:
> Shouldn't we just abort everything if we detect the buffer is invalid?
> sk_throw()? fInputCount = 0?

So far, the idea has been that if a buffer is invalid, an error is signaled to
SkValidatingReadBuffer, which will eventually lead to the deletion of the entire
filter tree (and readFlattenable will return NULL). If I set fInputCount to 0,
I'll create a memory leak when this filter gets deleted and doesn't delete the
inputs that were successfully created.

https://codereview.chromium.org/106943002/diff/60001/src/effects/SkColorMatri...
File src/effects/SkColorMatrixFilter.cpp (right):

https://codereview.chromium.org/106943002/diff/60001/src/effects/SkColorMatri...
src/effects/SkColorMatrixFilter.cpp:314:
buffer.validate(SkScalarIsFinite(fMatrix.fMat[i]));
On 2013/12/05 21:35:23, reed1 wrote:
> Is this really invalid? Do we check for this sort of thing in our normal
> constructor? Would we every crash with infinities?

I doubt that infinities would be the intended behavior of any filter, but I can
test to see what would happen with infinite value here.

[reed1, 2013-12-06 14:16:29]

On 2013/12/05 23:27:49, sugoi wrote:
>
https://codereview.chromium.org/106943002/diff/60001/samplecode/SampleFilterF...
> File samplecode/SampleFilterFuzz.cpp (right):
> 
>
https://codereview.chromium.org/106943002/diff/60001/samplecode/SampleFilterF...
> samplecode/SampleFilterFuzz.cpp:34: //#define SK_ADD_RANDOM_BIT_FLIPS
> On 2013/12/05 21:35:23, reed1 wrote:
> > Can we remove these?
> 
> I just added these. I type them every time I enable special modes in the
fuzzer,
> so I thought it would be simpler to uncomment them when I need them. Do you
mean
> that these should always be enabled (or enabled by default) ?

OK, we can leave them.

> 
>
https://codereview.chromium.org/106943002/diff/60001/src/core/SkImageFilter.cpp
> File src/core/SkImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/106943002/diff/60001/src/core/SkImageFilter.c...
> src/core/SkImageFilter.cpp:66: if (!buffer.isValid()) {
> On 2013/12/05 21:35:23, reed1 wrote:
> > Shouldn't we just abort everything if we detect the buffer is invalid?
> > sk_throw()? fInputCount = 0?
> 
> So far, the idea has been that if a buffer is invalid, an error is signaled to
> SkValidatingReadBuffer, which will eventually lead to the deletion of the
entire
> filter tree (and readFlattenable will return NULL). If I set fInputCount to 0,
> I'll create a memory leak when this filter gets deleted and doesn't delete the
> inputs that were successfully created.
> 
>
https://codereview.chromium.org/106943002/diff/60001/src/effects/SkColorMatri...
> File src/effects/SkColorMatrixFilter.cpp (right):
> 
>
https://codereview.chromium.org/106943002/diff/60001/src/effects/SkColorMatri...
> src/effects/SkColorMatrixFilter.cpp:314:
> buffer.validate(SkScalarIsFinite(fMatrix.fMat[i]));
> On 2013/12/05 21:35:23, reed1 wrote:
> > Is this really invalid? Do we check for this sort of thing in our normal
> > constructor? Would we every crash with infinities?
> 
> I doubt that infinities would be the intended behavior of any filter, but I
can
> test to see what would happen with infinite value here.

Possibly, but I think that is a never-ending road, looking for meaningful input.
I'd like us to see if we can limit the scope to input-that-puts-us-at-risk.

[sugoi1, 2013-12-06 18:39:04]

https://codereview.chromium.org/106943002/diff/60001/include/core/SkFlattenab...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/106943002/diff/60001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:156: virtual bool isValid() const { return
true; }
On 2013/12/05 21:35:23, reed1 wrote:
> Please add some dox. What is valid? When is it not valid?

Done.

https://codereview.chromium.org/106943002/diff/80001/src/effects/SkColorMatri...
File src/effects/SkColorMatrixFilter.cpp (left):

https://codereview.chromium.org/106943002/diff/80001/src/effects/SkColorMatri...
src/effects/SkColorMatrixFilter.cpp:314:
buffer.validate(SkScalarIsFinite(fMatrix.fMat[i]));
I didn't find anything obviously wrong with removing the SkScalarIsFinite()
checks after doing some local tests and looking at the uses of fMat in the code.
It does use some arithmetic functions, like operators, min, abs, etc, but there
should be no further consequence to a NaN then turning the result into NaNs. If
these render on the GPU, NaNs may yield an undefined result, depending on the
hardware, but undefined behavior shouldn't cause security issues, hopefully
(maybe I'm wrong).
I don't think these calls had a significant cost (performancewise), though, so I
don't know how much can be gained by removing them, but, for now, I can just let
the fuzzer handle this and try to find issues with this, so that we can at least
know if there's an issue with this.

[reed1, 2013-12-06 19:09:29]

lgtm

[commit-bot: I haz the power, 2013-12-06 19:14:59]

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

[commit-bot: I haz the power, 2013-12-06 20:14:50]

Change committed as 12544