Initial error handling code

Initial error handling code

I made it as simple as possible. The impact seems minimal and it should do what's necessary to make this code secure.

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=11247

Committed: http://code.google.com/p/skia/source/detail?r=11922

[sugoi1, 2013-08-21 15:33:40]

https://codereview.chromium.org/23021015/diff/1/include/core/SkError.h
File include/core/SkError.h (right):

https://codereview.chromium.org/23021015/diff/1/include/core/SkError.h#newcode18
include/core/SkError.h:18: kNoError_SkError = 0x00,
Changed these values so that they can be used as flags to be able to signal
multiple ones simultaneously, in case we want to handle different cases
differently.

[reed1, 2013-08-21 15:49:24]

Just to make our decision-making easier, lets not *also* turn SkError from an
enum to a bitfield in the same CL.

For now, perhaps we just add a bool (succes/failure) to the readbuffer. We can
change it to a more formal enum/bitfield later if we want.

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

https://codereview.chromium.org/23021015/diff/1/src/core/SkImageFilter.cpp#ne...
src/core/SkImageFilter.cpp:68: buffer.setError(kInvalidArgument_SkError);
Should we "reset" ourselves since we're signaling an error?

e.g. delete all of the sub-objects in fInputs and free that array?

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

https://codereview.chromium.org/23021015/diff/1/src/core/SkOrderedReadBuffer....
src/core/SkOrderedReadBuffer.cpp:306: fError = kNoError_SkError;
Hmmm, maybe this is correct (to reset fError to none) or maybe we shouldn't hide
the fact that we had a failure... ?

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

https://codereview.chromium.org/23021015/diff/1/src/effects/SkBlurImageFilter...
src/effects/SkBlurImageFilter.cpp:23: buffer.setError(kInvalidArgument_SkError);
should we set fSigma to (0,0) to be "safe" now that we signaled an error?

[sugoi1, 2013-08-21 16:52:28]

On 2013/08/21 15:49:24, reed1 wrote:
> Just to make our decision-making easier, lets not *also* turn SkError from an
> enum to a bitfield in the same CL.
> 
> For now, perhaps we just add a bool (succes/failure) to the readbuffer. We can
> change it to a more formal enum/bitfield later if we want.

Ok, will do.

[sugoi1, 2013-08-21 16:52:35]

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

https://codereview.chromium.org/23021015/diff/1/src/core/SkImageFilter.cpp#ne...
src/core/SkImageFilter.cpp:68: buffer.setError(kInvalidArgument_SkError);
On 2013/08/21 15:49:24, reed1 wrote:
> Should we "reset" ourselves since we're signaling an error?
> 
> e.g. delete all of the sub-objects in fInputs and free that array?

I guess it depends if we intend to put error handling code everywhere or just at
the caller site. Is the caller the one to decide how to handle errors or is each
individual object responsible for that ?

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

https://codereview.chromium.org/23021015/diff/1/src/core/SkOrderedReadBuffer....
src/core/SkOrderedReadBuffer.cpp:306: fError = kNoError_SkError;
On 2013/08/21 15:49:24, reed1 wrote:
> Hmmm, maybe this is correct (to reset fError to none) or maybe we shouldn't
hide the fact that we had a failure... ?

It's only reset here so that the next read operation (or factory function call)
will not fail on the previous read operation's error. I could also set it to
"NoError" just before calling the factory function.

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

https://codereview.chromium.org/23021015/diff/1/src/effects/SkBlurImageFilter...
src/effects/SkBlurImageFilter.cpp:23: buffer.setError(kInvalidArgument_SkError);
On 2013/08/21 15:49:24, reed1 wrote:
> should we set fSigma to (0,0) to be "safe" now that we signaled an error?

I can do that, but it comes back to the original question of who is responsible
for handling errors.

[sugoi, 2013-08-21 20:19:39]

Here's the new version.

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

https://codereview.chromium.org/23021015/diff/9001/src/core/SkImageFilter.cpp...
src/core/SkImageFilter.cpp:71: (fCropRect.fTop > fCropRect.fBottom)) {
Although it's counter intuitive, "top" really has to be smaller than "bottom".

[scroggo, 2013-08-21 23:56:00]

https://codereview.chromium.org/23021015/diff/1/include/core/SkError.h
File include/core/SkError.h (right):

https://codereview.chromium.org/23021015/diff/1/include/core/SkError.h#newcode18
include/core/SkError.h:18: kNoError_SkError = 0x00,
On 2013/08/21 15:33:41, sugoi1 wrote:
> Changed these values so that they can be used as flags to be able to signal
> multiple ones simultaneously, in case we want to handle different cases
> differently.

If we're going to change this, should we change the other use of SkError (using
SkErrorInternal)?

https://codereview.chromium.org/23021015/diff/1/include/core/SkError.h#newcode24
include/core/SkError.h:24: kInvalidArgument_SkError = 0x01,
Typically we make these line up horizontally. i.e.:

kNoError_SkError         = 0x00,
kInvalidArgument_SkError = 0x01,
.
.
.

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

https://codereview.chromium.org/23021015/diff/1/src/core/SkOrderedReadBuffer....
src/core/SkOrderedReadBuffer.cpp:306: fError = kNoError_SkError;
On 2013/08/21 16:52:36, sugoi1 wrote:
> On 2013/08/21 15:49:24, reed1 wrote:
> > Hmmm, maybe this is correct (to reset fError to none) or maybe we shouldn't
> hide the fact that we had a failure... ?
> 
> It's only reset here so that the next read operation (or factory function
call)
> will not fail on the previous read operation's error. I could also set it to
> "NoError" just before calling the factory function.

This seems to be basically the same, since SkOrderedReadBuffer is going to
continue calling readFlattenable.

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

https://codereview.chromium.org/23021015/diff/9001/src/core/SkImageFilter.cpp...
src/core/SkImageFilter.cpp:71: (fCropRect.fTop > fCropRect.fBottom)) {
On 2013/08/21 20:19:39, sugoi wrote:
> Although it's counter intuitive, "top" really has to be smaller than "bottom".

Depends on which way you think the y-axis should go :)

https://codereview.chromium.org/23021015/diff/9001/src/core/SkOrderedReadBuff...
File src/core/SkOrderedReadBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/9001/src/core/SkOrderedReadBuff...
src/core/SkOrderedReadBuffer.cpp:304: if (fError) {
From here, do we assume the rest of this stream is still good and continue
reading?

[scroggo, 2013-08-21 23:57:16]

Please ignore comments on SkError changes which have been removed...

https://codereview.chromium.org/23021015/diff/1/include/core/SkError.h
File include/core/SkError.h (right):

https://codereview.chromium.org/23021015/diff/1/include/core/SkError.h#newcode18
include/core/SkError.h:18: kNoError_SkError = 0x00,
On 2013/08/21 23:56:00, scroggo wrote:
> On 2013/08/21 15:33:41, sugoi1 wrote:
> > Changed these values so that they can be used as flags to be able to signal
> > multiple ones simultaneously, in case we want to handle different cases
> > differently.
> 
> If we're going to change this, should we change the other use of SkError
(using
> SkErrorInternal)?

nvm

https://codereview.chromium.org/23021015/diff/1/include/core/SkError.h#newcode24
include/core/SkError.h:24: kInvalidArgument_SkError = 0x01,
On 2013/08/21 23:56:00, scroggo wrote:
> Typically we make these line up horizontally. i.e.:
> 
> kNoError_SkError         = 0x00,
> kInvalidArgument_SkError = 0x01,
> .
> .
> .

nvm

[sugoi1, 2013-08-22 14:26:23]

https://codereview.chromium.org/23021015/diff/9001/src/core/SkOrderedReadBuff...
File src/core/SkOrderedReadBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/9001/src/core/SkOrderedReadBuff...
src/core/SkOrderedReadBuffer.cpp:304: if (fError) {
On 2013/08/21 23:56:00, scroggo wrote:
> From here, do we assume the rest of this stream is still good and continue
> reading?

For now, since we don't what kind of error we hit, it may be a bit harsh to
assume that any error means that the entire stream is invalid. I was also unsure
about the side effects of not reading the entire size we were supposed to read
(it may be safe to quit right away, but I haven't tested it).

[reed1, 2013-08-22 15:41:14]

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

https://codereview.chromium.org/23021015/diff/9001/src/core/SkImageFilter.cpp...
src/core/SkImageFilter.cpp:66: if (!SkScalarIsFinite(fCropRect.fLeft) ||
SkIRect stores ints, so calling isfinite does not make sense.

[sugoi1, 2013-08-22 17:37:02]

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

https://codereview.chromium.org/23021015/diff/9001/src/core/SkImageFilter.cpp...
src/core/SkImageFilter.cpp:66: if (!SkScalarIsFinite(fCropRect.fLeft) ||
On 2013/08/22 15:41:14, reed1 wrote:
> SkIRect stores ints, so calling isfinite does not make sense.

Oops, fixed.

[reed1, 2013-08-22 19:45:07]

At this stage, I've run out of syntax nits, and am just interested in the larger
question:

If unflattening encounters bad data (however that is defined, which is still an
interesting debate), what should we do?

My inclination is to abort (i.e. return null) from the entire unflattening
process. Whether the caller is aware that we returned null for this reason seems
less important.

Is this a discussion we want to have over VC, or just in this CL?

[sugoi1, 2013-08-22 20:02:11]

On 2013/08/22 19:45:07, reed1 wrote:
> At this stage, I've run out of syntax nits, and am just interested in the
larger
> question:
> 
> If unflattening encounters bad data (however that is defined, which is still
an
> interesting debate), what should we do?
> 
> My inclination is to abort (i.e. return null) from the entire unflattening
> process. Whether the caller is aware that we returned null for this reason
seems
> less important.
> 
> Is this a discussion we want to have over VC, or just in this CL?

As I'm about to go home in 5 min, let's discuss here for today :)

I agree that the unflattening process should return NULL.

My only question is this : if we stop reading a stream (aka SkOrderedReadBuffer)
in the middle in Skia, are we leaving any object in a weird state ? If so, this
could be a vulnerability, I think, so that's why I let the entire process finish
even if an error has been detected. I can add early exits, if we're confident
there isn't a potential problem with doing that.

[reed1, 2013-08-22 20:14:17]

What if the stream contains count + [count] values, and the reading code
encounters -1 for the count. Not sure how to keep reading.

I think it is a valid question about leaving the stream cursor not-at-the-end,
but it does seem that sometimes we won't know how to get to the end.

[sugoi1, 2013-08-23 18:03:50]

On 2013/08/22 20:14:17, reed1 wrote:
> What if the stream contains count + [count] values, and the reading code
> encounters -1 for the count. Not sure how to keep reading.
> 
> I think it is a valid question about leaving the stream cursor not-at-the-end,
> but it does seem that sometimes we won't know how to get to the end.

Hmmm... I've been looking for a solution for this and it really isn't an obvious
problem to solve. The memory pointer logic resides inside SkReader32, which
currently has some (debug only) checks. I don't want to hurt the
readability/performance too much by adding checks everywhere, but really no read
operation should be allowed to go past the end of the stream and continue as if
nothing's happened. I can attempt to add some eof() checks within
SkOrderedReadBuffer on each of the read operations, unless you think there's a
better way of doing this.

[sugoi1, 2013-08-23 18:39:37]

I added eof() checks, just so that we can see what it would look like. This is
flawed because it doesn't verify if the current read operation went past the
eof. I could add checks directly inside SkReader32, which would probably be the
safest, but also have the most impact since I'd need to add something in the
SkReader32 API to transmit the information that an error occurred from
SkReader32 to SkOrderedReadBuffer.

[sugoi1, 2013-08-29 18:55:53]

Here's the initial version of the "secure read buffer" class. I'll add more
security checks to it, but this should be enough to tell me if this is what you
had in mind.

https://codereview.chromium.org/23021015/diff/26001/src/core/SkOrderedWriteBu...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/26001/src/core/SkOrderedWriteBu...
src/core/SkOrderedWriteBuffer.cpp:273: if (isSecure()) {
I added these 2 checks to the writer, but, hopefully, these should be small
enough to not impact performance.

https://codereview.chromium.org/23021015/diff/26001/src/core/SkSecureReadBuff...
File src/core/SkSecureReadBuffer.h (left):

https://codereview.chromium.org/23021015/diff/26001/src/core/SkSecureReadBuff...
src/core/SkSecureReadBuffer.h:2: /*
In case you you're curious, I didn't actually replace SkOrderedReadBuffer by
SkSecureReadBuffer, I added SkSecureReadBuffer, but it's probably easier to
review that way.

https://codereview.chromium.org/23021015/diff/26001/src/core/SkWriter32.cpp
File src/core/SkWriter32.cpp (right):

https://codereview.chromium.org/23021015/diff/26001/src/core/SkWriter32.cpp#n...
src/core/SkWriter32.cpp:251: // skip over the string + '\0' and then pad to a
multiple of 4
Just fixing the typo

[reed1, 2013-08-29 19:08:21]

High-level question:

Can we get away with just a SecureBuffer, instead of a SecureBuffer AND a
SecureReader? Just wanting to share/isolate code where possible.

https://codereview.chromium.org/23021015/diff/26001/src/core/SkFlattenableSer...
File src/core/SkFlattenableSerialization.cpp (right):

https://codereview.chromium.org/23021015/diff/26001/src/core/SkFlattenableSer...
src/core/SkFlattenableSerialization.cpp:8: #include
"SkFlattenableSerialization.h"
Boy oh boy do we need some documentation in this header!

https://codereview.chromium.org/23021015/diff/26001/src/core/SkFlattenableSer...
src/core/SkFlattenableSerialization.cpp:17:
writer.setFlags(SkFlattenableWriteBuffer::kSecure_Flag);
Why does Serialize now imply Secure?

https://codereview.chromium.org/23021015/diff/26001/src/core/SkOrderedWriteBu...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/26001/src/core/SkOrderedWriteBu...
src/core/SkOrderedWriteBuffer.cpp:273: if (isSecure()) {
style nit: this->isSecure()

https://codereview.chromium.org/23021015/diff/26001/src/core/SkOrderedWriteBu...
src/core/SkOrderedWriteBuffer.cpp:295: if (isSecure()) {
style nit: this->isSecure()

https://codereview.chromium.org/23021015/diff/26001/src/effects/SkBlurImageFi...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/23021015/diff/26001/src/effects/SkBlurImageFi...
src/effects/SkBlurImageFilter.cpp:26: buffer.setError();
Might add a helper for this pattern (now or later)

e.g.

buffer.setErrorIfTrue(predicate);

(with a better name)

[sugoi1, 2013-08-29 19:39:39]

On 2013/08/29 19:08:21, reed1 wrote:
> High-level question:
> 
> Can we get away with just a SecureBuffer, instead of a SecureBuffer AND a
SecureReader? Just wanting to share/isolate code where possible.

The reason for SecureReader is to add a bunch of checks without impacting the
performance of the current reader and to be able to do things like early exits
to prevent from reading past the end of the stream, which the "fast" reader may
not care about. I also removed functionality like "readFunctionPtr()" from the
secure reader, so that it's not even possible to do that.

https://codereview.chromium.org/23021015/diff/26001/src/core/SkFlattenableSer...
File src/core/SkFlattenableSerialization.cpp (right):

https://codereview.chromium.org/23021015/diff/26001/src/core/SkFlattenableSer...
src/core/SkFlattenableSerialization.cpp:8: #include
"SkFlattenableSerialization.h"
On 2013/08/29 19:08:22, reed1 wrote:
> Boy oh boy do we need some documentation in this header!

Noted, although I might do this in a separate cl.

https://codereview.chromium.org/23021015/diff/26001/src/core/SkFlattenableSer...
src/core/SkFlattenableSerialization.cpp:17:
writer.setFlags(SkFlattenableWriteBuffer::kSecure_Flag);
On 2013/08/29 19:08:22, reed1 wrote:
> Why does Serialize now imply Secure?

To be symmetrical with the secure read operation, for which security may imply a
different way of serializing data (for example: no function pointers).

https://codereview.chromium.org/23021015/diff/26001/src/core/SkOrderedWriteBu...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/26001/src/core/SkOrderedWriteBu...
src/core/SkOrderedWriteBuffer.cpp:273: if (isSecure()) {
On 2013/08/29 19:08:22, reed1 wrote:
> style nit: this->isSecure()

Done.

https://codereview.chromium.org/23021015/diff/26001/src/core/SkOrderedWriteBu...
src/core/SkOrderedWriteBuffer.cpp:295: if (isSecure()) {
On 2013/08/29 19:08:22, reed1 wrote:
> style nit: this->isSecure()

Done.

https://codereview.chromium.org/23021015/diff/26001/src/effects/SkBlurImageFi...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/23021015/diff/26001/src/effects/SkBlurImageFi...
src/effects/SkBlurImageFilter.cpp:26: buffer.setError();
On 2013/08/29 19:08:22, reed1 wrote:
> Might add a helper for this pattern (now or later)
> 
> e.g.
> 
> buffer.setErrorIfTrue(predicate);
> 
> (with a better name)

Done. Called it validateData() for now.

[reed1, 2013-08-29 19:57:56]

Do we ever have callers/users of SkSerializeFlattenable() that don't want
security? Why isn't this an optional parameter that can be passed in?

Again in an effort to try to share code, can we not put the extra "checks" that
appear in SecureReader into the caller (SecureBuffer), so that after those
checks pass, it can reuse common code in SkReader32?

[sugoi1, 2013-08-29 20:02:42]

On 2013/08/29 19:57:56, reed1 wrote:
> Do we ever have callers/users of SkSerializeFlattenable() that don't want
> security? Why isn't this an optional parameter that can be passed in?

So far, every user of SkSerializeFlattenable() wants security, but I could add a
parameter if we want the non-secure version at some point.

> Again in an effort to try to share code, can we not put the extra "checks"
that
> appear in SecureReader into the caller (SecureBuffer), so that after those
> checks pass, it can reuse common code in SkReader32?

I'll try and see if I can replicate the new behavior with functionality provided
by SkReader32.

[reed1, 2013-08-29 20:04:16]

On 2013/08/29 20:02:42, sugoi1 wrote:
> On 2013/08/29 19:57:56, reed1 wrote:
> > Do we ever have callers/users of SkSerializeFlattenable() that don't want
> > security? Why isn't this an optional parameter that can be passed in?
> 
> So far, every user of SkSerializeFlattenable() wants security, but I could add
a
> parameter if we want the non-secure version at some point.

I guess this goes back to my request for dox in the header. If this guy always
provides security, we could perhaps name it that way. If not, then we should
consider taking a parameter or something.

> 
> > Again in an effort to try to share code, can we not put the extra "checks"
> that
> > appear in SecureReader into the caller (SecureBuffer), so that after those
> > checks pass, it can reuse common code in SkReader32?
> 
> I'll try and see if I can replicate the new behavior with functionality
provided
> by SkReader32.

[sugoi1, 2013-08-29 20:09:54]

On 2013/08/29 20:04:16, reed1 wrote:
> I guess this goes back to my request for dox in the header. If this guy always
> provides security, we could perhaps name it that way. If not, then we should
> consider taking a parameter or something.

I guess for now I'd rather rename it. I don't foresee this being used in a
non-secure case in the near future. I can always change it if a use case for
this ever appears.

[reed1, 2013-08-29 20:12:11]

On 2013/08/29 20:09:54, sugoi1 wrote:
> On 2013/08/29 20:04:16, reed1 wrote:
> > I guess this goes back to my request for dox in the header. If this guy
always
> > provides security, we could perhaps name it that way. If not, then we should
> > consider taking a parameter or something.
> 
> I guess for now I'd rather rename it. I don't foresee this being used in a
> non-secure case in the near future. I can always change it if a use case for
> this ever appears.

Renaming perfectly fine with me. Makes its use-case and restrictions very clear
to the caller.

[mtklein, 2013-08-30 15:28:23]

On 2013/08/29 19:57:56, reed1 wrote:
> Do we ever have callers/users of SkSerializeFlattenable() that don't want
> security? Why isn't this an optional parameter that can be passed in?
> 
> Again in an effort to try to share code, can we not put the extra "checks"
that
> appear in SecureReader into the caller (SecureBuffer), so that after those
> checks pass, it can reuse common code in SkReader32?

My hunch is that we're sharing too much code already.  I'd really like to be
able to change SkReader and SkWriter without ever worrying about security; even
with wrapped versions I'd still worry.

I'd like take a swing at this to maybe come up with a counter proposal CL, or at
worst help my understanding of the details of the problem.

[sugoi1, 2013-08-30 15:51:07]

So, for reference, here's what it would look like if we used SkReader32
directly.

[sugoi1, 2013-09-03 20:18:13]

On 2013/08/30 15:28:23, mtklein wrote:
> I'd like take a swing at this to maybe come up with a counter proposal CL, or
at
> worst help my understanding of the details of the problem.

Tell me how this is going. In the meantime, I've added to this cl individual
checks to constructors called during the deserialization of SkImageFilter
objects.

[reed1, 2013-09-03 20:25:20]

https://codereview.chromium.org/23021015/diff/47001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/23021015/diff/47001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:225: static bool IsValidConfig(Config config);
can we put this in some helper file for validation, and not in this public .h?

https://codereview.chromium.org/23021015/diff/47001/include/core/SkFlattenabl...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/23021015/diff/47001/include/core/SkFlattenabl...
include/core/SkFlattenableBuffers.h:44: kSecure_Flag            = 1 << 3,
I wonder if kValidating_Flag or something like that is more descriptive/specific
that "secure". Either way, I think we should add some dox here as to what this
flag does.

https://codereview.chromium.org/23021015/diff/47001/include/core/SkFlattenabl...
File include/core/SkFlattenableSerialization.h (right):

https://codereview.chromium.org/23021015/diff/47001/include/core/SkFlattenabl...
include/core/SkFlattenableSerialization.h:16: SK_API SkData*
SkSafeSerializeFlattenable(SkFlattenable*);
SkValidatingSerializeFlattenables() ?

add some dox, even brief ones?

https://codereview.chromium.org/23021015/diff/47001/include/core/SkRect.h
File include/core/SkRect.h (right):

https://codereview.chromium.org/23021015/diff/47001/include/core/SkRect.h#new...
include/core/SkRect.h:103: bool isValid() const { return (fLeft <= fRight) &&
(fTop <= fBottom); }
I think we already have the notion of "inverted" for rects. Seems like this
could use that name somehow.

isInverted() perhaps. That at least is more specific than "valid".

BTW - is it valid/safe if left < right, but right - left overflows a signed int?

https://codereview.chromium.org/23021015/diff/47001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/23021015/diff/47001/include/core/SkXfermode.h...
include/core/SkXfermode.h:84: static bool IsValidCoeff(Coeff coeff);
Move this to some validating helper file?

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

https://codereview.chromium.org/23021015/diff/47001/src/core/SkBitmap.cpp#new...
src/core/SkBitmap.cpp:269: bool SkBitmap::IsValidConfig(Config config) {
I'm not 100% sure that the compile will always execute this. Is there ever a
chance that an optimizing one will decide that the list is complete, and turn
the entire function into

return true;

[sugoi1, 2013-09-04 18:01:10]

Minor tweaks and fixes.

@mtklein: Since your design may still take a while, would it be acceptable for
you if we went ahead with this change, and you could propose your changes later
by modifying the guts of SkSecureReadBuffer in a follow-up ?

https://codereview.chromium.org/23021015/diff/47001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/23021015/diff/47001/include/core/SkBitmap.h#n...
include/core/SkBitmap.h:225: static bool IsValidConfig(Config config);
On 2013/09/03 20:25:21, reed1 wrote:
> can we put this in some helper file for validation, and not in this public .h?

Done.

https://codereview.chromium.org/23021015/diff/47001/include/core/SkFlattenabl...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/23021015/diff/47001/include/core/SkFlattenabl...
include/core/SkFlattenableBuffers.h:44: kSecure_Flag            = 1 << 3,
On 2013/09/03 20:25:21, reed1 wrote:
> I wonder if kValidating_Flag or something like that is more
descriptive/specific
> that "secure". Either way, I think we should add some dox here as to what this
> flag does.

Done.

https://codereview.chromium.org/23021015/diff/47001/include/core/SkFlattenabl...
File include/core/SkFlattenableSerialization.h (right):

https://codereview.chromium.org/23021015/diff/47001/include/core/SkFlattenabl...
include/core/SkFlattenableSerialization.h:16: SK_API SkData*
SkSafeSerializeFlattenable(SkFlattenable*);
On 2013/09/03 20:25:21, reed1 wrote:
> SkValidatingSerializeFlattenables() ?
> 
> add some dox, even brief ones?

Done.

https://codereview.chromium.org/23021015/diff/47001/include/core/SkRect.h
File include/core/SkRect.h (right):

https://codereview.chromium.org/23021015/diff/47001/include/core/SkRect.h#new...
include/core/SkRect.h:103: bool isValid() const { return (fLeft <= fRight) &&
(fTop <= fBottom); }
On 2013/09/03 20:25:21, reed1 wrote:
> BTW - is it valid/safe if left < right, but right - left overflows a signed
int?

Good point, I added a check to make sure width() and height() aren't oveflowing
the signed int.

https://codereview.chromium.org/23021015/diff/47001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/23021015/diff/47001/include/core/SkXfermode.h...
include/core/SkXfermode.h:84: static bool IsValidCoeff(Coeff coeff);
On 2013/09/03 20:25:21, reed1 wrote:
> Move this to some validating helper file?

Done.

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

https://codereview.chromium.org/23021015/diff/47001/src/core/SkBitmap.cpp#new...
src/core/SkBitmap.cpp:269: bool SkBitmap::IsValidConfig(Config config) {
On 2013/09/03 20:25:21, reed1 wrote:
> I'm not 100% sure that the compile will always execute this. Is there ever a
> chance that an optimizing one will decide that the list is complete, and turn
> the entire function into
> 
> return true;

Not sure, but I've changed it just to be certain.

[reed1, 2013-09-04 18:49:35]

https://codereview.chromium.org/23021015/diff/56001/include/core/SkFlattenabl...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/23021015/diff/56001/include/core/SkFlattenabl...
include/core/SkFlattenableBuffers.h:57: bool isSecure() const { return
SkToBool(fFlags & kValidation_Flag); }
isValidating()?

https://codereview.chromium.org/23021015/diff/56001/include/core/SkFlattenabl...
include/core/SkFlattenableBuffers.h:184: bool isSecure() const {
isValidating()?

https://codereview.chromium.org/23021015/diff/56001/include/core/SkRect.h
File include/core/SkRect.h (right):

https://codereview.chromium.org/23021015/diff/56001/include/core/SkRect.h#new...
include/core/SkRect.h:105: bool isValid() const {
Is this the same as

return this->width() >= 0 && this->height() >= 0;

https://codereview.chromium.org/23021015/diff/56001/include/core/SkRect.h#new...
include/core/SkRect.h:105: bool isValid() const {
Suggest we move this into SkValidatingUtils.h

https://codereview.chromium.org/23021015/diff/56001/include/core/SkRect.h#new...
include/core/SkRect.h:433: bool isValid() const {
Given the size an somewhat arbitrary nature of "valid" for a rect, I suggest
this be moved into SkValidatinUtils.h. There we can explicitly document all of
the aspects of "validness" we care about.

https://codereview.chromium.org/23021015/diff/56001/src/core/SkSecureReadBuff...
File src/core/SkSecureReadBuffer.h (right):

https://codereview.chromium.org/23021015/diff/56001/src/core/SkSecureReadBuff...
src/core/SkSecureReadBuffer.h:8: #ifndef SkSecureReadBuffer_DEFINED
nit: is SkValidatingReadBuffer.h a better name?

https://codereview.chromium.org/23021015/diff/56001/src/core/SkValidation.h
File src/core/SkValidation.h (right):

https://codereview.chromium.org/23021015/diff/56001/src/core/SkValidation.h#n...
src/core/SkValidation.h:8: #ifndef SkValidation_DEFINED
SkValidationUtils.h might be a more similar naming scheme for skia

[sugoi1, 2013-09-04 20:14:51]

https://codereview.chromium.org/23021015/diff/56001/include/core/SkFlattenabl...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/23021015/diff/56001/include/core/SkFlattenabl...
include/core/SkFlattenableBuffers.h:57: bool isSecure() const { return
SkToBool(fFlags & kValidation_Flag); }
On 2013/09/04 18:49:36, reed1 wrote:
> isValidating()?

Done.

https://codereview.chromium.org/23021015/diff/56001/include/core/SkFlattenabl...
include/core/SkFlattenableBuffers.h:184: bool isSecure() const {
On 2013/09/04 18:49:36, reed1 wrote:
> isValidating()?

Done.

https://codereview.chromium.org/23021015/diff/56001/include/core/SkRect.h
File include/core/SkRect.h (right):

https://codereview.chromium.org/23021015/diff/56001/include/core/SkRect.h#new...
include/core/SkRect.h:105: bool isValid() const {
On 2013/09/04 18:49:36, reed1 wrote:
> Suggest we move this into SkValidatingUtils.h

Done.

https://codereview.chromium.org/23021015/diff/56001/include/core/SkRect.h#new...
include/core/SkRect.h:105: bool isValid() const {
On 2013/09/04 18:49:36, reed1 wrote:
> Is this the same as
> 
> return this->width() >= 0 && this->height() >= 0;

Hmmm... I'm not sure. In the case of width(), for example, if "fRight - fLeft" >
SK_MaxS32, in the case where fRight is a valid large positive number and fLeft
is a valid large negative number, then the arithmetic should overflow into
negative numbers and make the width() test work, but is that behavior guaranteed
on all platforms ? Can we rely on common behavior for integer overflow and
should we trigger one on purpose when we can avoid it ?

https://codereview.chromium.org/23021015/diff/56001/include/core/SkRect.h#new...
include/core/SkRect.h:433: bool isValid() const {
On 2013/09/04 18:49:36, reed1 wrote:
> Given the size an somewhat arbitrary nature of "valid" for a rect, I suggest
> this be moved into SkValidatinUtils.h. There we can explicitly document all of
> the aspects of "validness" we care about.

Done.

https://codereview.chromium.org/23021015/diff/56001/src/core/SkSecureReadBuff...
File src/core/SkSecureReadBuffer.h (right):

https://codereview.chromium.org/23021015/diff/56001/src/core/SkSecureReadBuff...
src/core/SkSecureReadBuffer.h:8: #ifndef SkSecureReadBuffer_DEFINED
On 2013/09/04 18:49:36, reed1 wrote:
> nit: is SkValidatingReadBuffer.h a better name?

Done.

https://codereview.chromium.org/23021015/diff/56001/src/core/SkValidation.h
File src/core/SkValidation.h (right):

https://codereview.chromium.org/23021015/diff/56001/src/core/SkValidation.h#n...
src/core/SkValidation.h:8: #ifndef SkValidation_DEFINED
On 2013/09/04 18:49:36, reed1 wrote:
> SkValidationUtils.h might be a more similar naming scheme for skia

Done.

[reed1, 2013-09-04 20:37:41]

https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtils.h
File src/core/SkValidationUtils.h (right):

https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
src/core/SkValidationUtils.h:37: ((rect.fLeft >= 0) || (rect.fRight <= 0) ||
Is this section the same as

rect.width() >= 0 && rect.height() >= 0

?

https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
src/core/SkValidationUtils.h:46: return SkScalarIsFinite(rect.fLeft) &&
Is all of this the same as

return !rect.inverted() && SkScalarIsFinite(rect.width()) &&
SkScalarIsFinite(rect.height());

?

[sugoi1, 2013-09-05 14:50:16]

On 2013/09/04 20:37:41, reed1 wrote:
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtils.h
> File src/core/SkValidationUtils.h (right):
> 
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> src/core/SkValidationUtils.h:37: ((rect.fLeft >= 0) || (rect.fRight <= 0) ||
> Is this section the same as
> 
> rect.width() >= 0 && rect.height() >= 0
> 
> ?
> 
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> src/core/SkValidationUtils.h:46: return SkScalarIsFinite(rect.fLeft) &&
> Is all of this the same as
> 
> return !rect.inverted() && SkScalarIsFinite(rect.width()) &&
> SkScalarIsFinite(rect.height());
> 
> ?

This comes back to my original question : Can we rely on int/float overflow
behavior being constant on all platforms for these tests and, if so, do we want
to trigger overflows when they can be avoided ? If it's ok to do that, I'll
simplify these tests.

[reed1, 2013-09-05 15:23:21]

On 2013/09/05 14:50:16, sugoi1 wrote:
> On 2013/09/04 20:37:41, reed1 wrote:
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtils.h
> > File src/core/SkValidationUtils.h (right):
> > 
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> > src/core/SkValidationUtils.h:37: ((rect.fLeft >= 0) || (rect.fRight <= 0) ||
> > Is this section the same as
> > 
> > rect.width() >= 0 && rect.height() >= 0
> > 
> > ?
> > 
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> > src/core/SkValidationUtils.h:46: return SkScalarIsFinite(rect.fLeft) &&
> > Is all of this the same as
> > 
> > return !rect.inverted() && SkScalarIsFinite(rect.width()) &&
> > SkScalarIsFinite(rect.height());
> > 
> > ?
> 
> This comes back to my original question : Can we rely on int/float overflow
> behavior being constant on all platforms for these tests

Are you asking if width() < 0 indicates an overflow for ints? If so, then I
think yes, that is well defined for 2's compliment ints. !isFinite(width()) is
well defined for IEEE floating point math.

> and, if so, do we want
> to trigger overflows when they can be avoided ? If it's ok to do that, I'll
> simplify these tests.

I'm not sure what you mean by trigger overflows and "avoided". If we have a
rect, it is possible that some piece of code will want to know is width(), so
I'm not sure that we can safely avoid that overflow (if it exists).

[sugoi1, 2013-09-05 15:40:52]

On 2013/09/05 15:23:21, reed1 wrote:
> On 2013/09/05 14:50:16, sugoi1 wrote:
> > On 2013/09/04 20:37:41, reed1 wrote:
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtils.h
> > > File src/core/SkValidationUtils.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> > > src/core/SkValidationUtils.h:37: ((rect.fLeft >= 0) || (rect.fRight <= 0)
||
> > > Is this section the same as
> > > 
> > > rect.width() >= 0 && rect.height() >= 0
> > > 
> > > ?
> > > 
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> > > src/core/SkValidationUtils.h:46: return SkScalarIsFinite(rect.fLeft) &&
> > > Is all of this the same as
> > > 
> > > return !rect.inverted() && SkScalarIsFinite(rect.width()) &&
> > > SkScalarIsFinite(rect.height());
> > > 
> > > ?
> > 
> > This comes back to my original question : Can we rely on int/float overflow
> > behavior being constant on all platforms for these tests
> 
> Are you asking if width() < 0 indicates an overflow for ints? If so, then I
> think yes, that is well defined for 2's compliment ints. !isFinite(width()) is
> well defined for IEEE floating point math.
> 
> > and, if so, do we want
> > to trigger overflows when they can be avoided ? If it's ok to do that, I'll
> > simplify these tests.
> 
> I'm not sure what you mean by trigger overflows and "avoided". If we have a
> rect, it is possible that some piece of code will want to know is width(), so
> I'm not sure that we can safely avoid that overflow (if it exists).

What I mean is that if the difference between fRight and fLeft, for example, is
larger than the largest int/float value, the current code in
SkIsValidRect/SkIsValidRect will not perform an arithmetic operation that will
result in an overflow because the checks are made to avoid doing that and,
presumably, once the error is trapped, that SkRect or SkIRect object shouldn't
be used anymore. If width() is called directly, then that arithmetic operation
"fRight - fLeft" will cause an overflow, which can cause the OS to send a
signal, like SIGFPE, for example. If avoiding the potential overflow caused by
the subtraction done within the width() function is not important, I have no
problem changing it, I just want to validate that this causes no issue.

[reed1, 2013-09-05 16:36:19]

On 2013/09/05 15:40:52, sugoi1 wrote:
> On 2013/09/05 15:23:21, reed1 wrote:
> > On 2013/09/05 14:50:16, sugoi1 wrote:
> > > On 2013/09/04 20:37:41, reed1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtils.h
> > > > File src/core/SkValidationUtils.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> > > > src/core/SkValidationUtils.h:37: ((rect.fLeft >= 0) || (rect.fRight <=
0)
> ||
> > > > Is this section the same as
> > > > 
> > > > rect.width() >= 0 && rect.height() >= 0
> > > > 
> > > > ?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> > > > src/core/SkValidationUtils.h:46: return SkScalarIsFinite(rect.fLeft) &&
> > > > Is all of this the same as
> > > > 
> > > > return !rect.inverted() && SkScalarIsFinite(rect.width()) &&
> > > > SkScalarIsFinite(rect.height());
> > > > 
> > > > ?
> > > 
> > > This comes back to my original question : Can we rely on int/float
overflow
> > > behavior being constant on all platforms for these tests
> > 
> > Are you asking if width() < 0 indicates an overflow for ints? If so, then I
> > think yes, that is well defined for 2's compliment ints. !isFinite(width())
is
> > well defined for IEEE floating point math.
> > 
> > > and, if so, do we want
> > > to trigger overflows when they can be avoided ? If it's ok to do that,
I'll
> > > simplify these tests.
> > 
> > I'm not sure what you mean by trigger overflows and "avoided". If we have a
> > rect, it is possible that some piece of code will want to know is width(),
so
> > I'm not sure that we can safely avoid that overflow (if it exists).
> 
> What I mean is that if the difference between fRight and fLeft, for example,
is
> larger than the largest int/float value, the current code in
> SkIsValidRect/SkIsValidRect will not perform an arithmetic operation that will
> result in an overflow because the checks are made to avoid doing that and,
> presumably, once the error is trapped, that SkRect or SkIRect object shouldn't
> be used anymore. If width() is called directly, then that arithmetic operation
> "fRight - fLeft" will cause an overflow, which can cause the OS to send a
> signal, like SIGFPE, for example. If avoiding the potential overflow caused by
> the subtraction done within the width() function is not important, I have no
> problem changing it, I just want to validate that this causes no issue.

Are there OS signals that get fired when we overflow the subtraction of two
ints? This is news to me.

[sugoi1, 2013-09-05 18:17:31]

On 2013/09/05 16:36:19, reed1 wrote:
> On 2013/09/05 15:40:52, sugoi1 wrote:
> > On 2013/09/05 15:23:21, reed1 wrote:
> > > On 2013/09/05 14:50:16, sugoi1 wrote:
> > > > On 2013/09/04 20:37:41, reed1 wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtils.h
> > > > > File src/core/SkValidationUtils.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> > > > > src/core/SkValidationUtils.h:37: ((rect.fLeft >= 0) || (rect.fRight <=
> 0)
> > ||
> > > > > Is this section the same as
> > > > > 
> > > > > rect.width() >= 0 && rect.height() >= 0
> > > > > 
> > > > > ?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> > > > > src/core/SkValidationUtils.h:46: return SkScalarIsFinite(rect.fLeft)
&&
> > > > > Is all of this the same as
> > > > > 
> > > > > return !rect.inverted() && SkScalarIsFinite(rect.width()) &&
> > > > > SkScalarIsFinite(rect.height());
> > > > > 
> > > > > ?
> > > > 
> > > > This comes back to my original question : Can we rely on int/float
> overflow
> > > > behavior being constant on all platforms for these tests
> > > 
> > > Are you asking if width() < 0 indicates an overflow for ints? If so, then
I
> > > think yes, that is well defined for 2's compliment ints.
!isFinite(width())
> is
> > > well defined for IEEE floating point math.
> > > 
> > > > and, if so, do we want
> > > > to trigger overflows when they can be avoided ? If it's ok to do that,
> I'll
> > > > simplify these tests.
> > > 
> > > I'm not sure what you mean by trigger overflows and "avoided". If we have
a
> > > rect, it is possible that some piece of code will want to know is width(),
> so
> > > I'm not sure that we can safely avoid that overflow (if it exists).
> > 
> > What I mean is that if the difference between fRight and fLeft, for example,
> is
> > larger than the largest int/float value, the current code in
> > SkIsValidRect/SkIsValidRect will not perform an arithmetic operation that
will
> > result in an overflow because the checks are made to avoid doing that and,
> > presumably, once the error is trapped, that SkRect or SkIRect object
shouldn't
> > be used anymore. If width() is called directly, then that arithmetic
operation
> > "fRight - fLeft" will cause an overflow, which can cause the OS to send a
> > signal, like SIGFPE, for example. If avoiding the potential overflow caused
by
> > the subtraction done within the width() function is not important, I have no
> > problem changing it, I just want to validate that this causes no issue.
> 
> Are there OS signals that get fired when we overflow the subtraction of two
> ints? This is news to me.

You're right, this doesn't apply for integers. For SIGFPE, "These errors include
floating-point overflow, floating-point underflow, and either integer- or
floating-point division by 0. Note that integer overflow never causes a signal;
when integer overflow occurs, the result is reduced to 32 bits by discarding the
most significant bits and is then interpreted as a signed integer.". Ok, so do I
simplify the SkIRect case and leave the SkRect case as is or do I simplify both
? What's your preference for handling the floating point case ?

[reed1, 2013-09-05 18:49:26]

On 2013/09/05 18:17:31, sugoi1 wrote:
> On 2013/09/05 16:36:19, reed1 wrote:
> > On 2013/09/05 15:40:52, sugoi1 wrote:
> > > On 2013/09/05 15:23:21, reed1 wrote:
> > > > On 2013/09/05 14:50:16, sugoi1 wrote:
> > > > > On 2013/09/04 20:37:41, reed1 wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtils.h
> > > > > > File src/core/SkValidationUtils.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> > > > > > src/core/SkValidationUtils.h:37: ((rect.fLeft >= 0) || (rect.fRight
<=
> > 0)
> > > ||
> > > > > > Is this section the same as
> > > > > > 
> > > > > > rect.width() >= 0 && rect.height() >= 0
> > > > > > 
> > > > > > ?
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> > > > > > src/core/SkValidationUtils.h:46: return SkScalarIsFinite(rect.fLeft)
> &&
> > > > > > Is all of this the same as
> > > > > > 
> > > > > > return !rect.inverted() && SkScalarIsFinite(rect.width()) &&
> > > > > > SkScalarIsFinite(rect.height());
> > > > > > 
> > > > > > ?
> > > > > 
> > > > > This comes back to my original question : Can we rely on int/float
> > overflow
> > > > > behavior being constant on all platforms for these tests
> > > > 
> > > > Are you asking if width() < 0 indicates an overflow for ints? If so,
then
> I
> > > > think yes, that is well defined for 2's compliment ints.
> !isFinite(width())
> > is
> > > > well defined for IEEE floating point math.
> > > > 
> > > > > and, if so, do we want
> > > > > to trigger overflows when they can be avoided ? If it's ok to do that,
> > I'll
> > > > > simplify these tests.
> > > > 
> > > > I'm not sure what you mean by trigger overflows and "avoided". If we
have
> a
> > > > rect, it is possible that some piece of code will want to know is
width(),
> > so
> > > > I'm not sure that we can safely avoid that overflow (if it exists).
> > > 
> > > What I mean is that if the difference between fRight and fLeft, for
example,
> > is
> > > larger than the largest int/float value, the current code in
> > > SkIsValidRect/SkIsValidRect will not perform an arithmetic operation that
> will
> > > result in an overflow because the checks are made to avoid doing that and,
> > > presumably, once the error is trapped, that SkRect or SkIRect object
> shouldn't
> > > be used anymore. If width() is called directly, then that arithmetic
> operation
> > > "fRight - fLeft" will cause an overflow, which can cause the OS to send a
> > > signal, like SIGFPE, for example. If avoiding the potential overflow
caused
> by
> > > the subtraction done within the width() function is not important, I have
no
> > > problem changing it, I just want to validate that this causes no issue.
> > 
> > Are there OS signals that get fired when we overflow the subtraction of two
> > ints? This is news to me.
> 
> You're right, this doesn't apply for integers. For SIGFPE, "These errors
include
> floating-point overflow, floating-point underflow, and either integer- or
> floating-point division by 0. Note that integer overflow never causes a
signal;
> when integer overflow occurs, the result is reduced to 32 bits by discarding
the
> most significant bits and is then interpreted as a signed integer.". Ok, so do
I
> simplify the SkIRect case and leave the SkRect case as is or do I simplify
both
> ? What's your preference for handling the floating point case ?

I didn't know that any of our target machines ever signal that for floats. I see
NaN and infinities all the time from Chrome (and so I check for them) but I
never have heard of these triggering any signal. Do we know that this is an
issue for us?

[sugoi1, 2013-09-05 18:50:23]

On 2013/09/05 18:49:26, reed1 wrote:
> On 2013/09/05 18:17:31, sugoi1 wrote:
> > On 2013/09/05 16:36:19, reed1 wrote:
> > > On 2013/09/05 15:40:52, sugoi1 wrote:
> > > > On 2013/09/05 15:23:21, reed1 wrote:
> > > > > On 2013/09/05 14:50:16, sugoi1 wrote:
> > > > > > On 2013/09/04 20:37:41, reed1 wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtils.h
> > > > > > > File src/core/SkValidationUtils.h (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> > > > > > > src/core/SkValidationUtils.h:37: ((rect.fLeft >= 0) ||
(rect.fRight
> <=
> > > 0)
> > > > ||
> > > > > > > Is this section the same as
> > > > > > > 
> > > > > > > rect.width() >= 0 && rect.height() >= 0
> > > > > > > 
> > > > > > > ?
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/23021015/diff/66001/src/core/SkValidationUtil...
> > > > > > > src/core/SkValidationUtils.h:46: return
SkScalarIsFinite(rect.fLeft)
> > &&
> > > > > > > Is all of this the same as
> > > > > > > 
> > > > > > > return !rect.inverted() && SkScalarIsFinite(rect.width()) &&
> > > > > > > SkScalarIsFinite(rect.height());
> > > > > > > 
> > > > > > > ?
> > > > > > 
> > > > > > This comes back to my original question : Can we rely on int/float
> > > overflow
> > > > > > behavior being constant on all platforms for these tests
> > > > > 
> > > > > Are you asking if width() < 0 indicates an overflow for ints? If so,
> then
> > I
> > > > > think yes, that is well defined for 2's compliment ints.
> > !isFinite(width())
> > > is
> > > > > well defined for IEEE floating point math.
> > > > > 
> > > > > > and, if so, do we want
> > > > > > to trigger overflows when they can be avoided ? If it's ok to do
that,
> > > I'll
> > > > > > simplify these tests.
> > > > > 
> > > > > I'm not sure what you mean by trigger overflows and "avoided". If we
> have
> > a
> > > > > rect, it is possible that some piece of code will want to know is
> width(),
> > > so
> > > > > I'm not sure that we can safely avoid that overflow (if it exists).
> > > > 
> > > > What I mean is that if the difference between fRight and fLeft, for
> example,
> > > is
> > > > larger than the largest int/float value, the current code in
> > > > SkIsValidRect/SkIsValidRect will not perform an arithmetic operation
that
> > will
> > > > result in an overflow because the checks are made to avoid doing that
and,
> > > > presumably, once the error is trapped, that SkRect or SkIRect object
> > shouldn't
> > > > be used anymore. If width() is called directly, then that arithmetic
> > operation
> > > > "fRight - fLeft" will cause an overflow, which can cause the OS to send
a
> > > > signal, like SIGFPE, for example. If avoiding the potential overflow
> caused
> > by
> > > > the subtraction done within the width() function is not important, I
have
> no
> > > > problem changing it, I just want to validate that this causes no issue.
> > > 
> > > Are there OS signals that get fired when we overflow the subtraction of
two
> > > ints? This is news to me.
> > 
> > You're right, this doesn't apply for integers. For SIGFPE, "These errors
> include
> > floating-point overflow, floating-point underflow, and either integer- or
> > floating-point division by 0. Note that integer overflow never causes a
> signal;
> > when integer overflow occurs, the result is reduced to 32 bits by discarding
> the
> > most significant bits and is then interpreted as a signed integer.". Ok, so
do
> I
> > simplify the SkIRect case and leave the SkRect case as is or do I simplify
> both
> > ? What's your preference for handling the floating point case ?
> 
> I didn't know that any of our target machines ever signal that for floats. I
see
> NaN and infinities all the time from Chrome (and so I check for them) but I
> never have heard of these triggering any signal. Do we know that this is an
> issue for us?

Maybe not, I'm just being extra careful since this is security related code.

[sugoi1, 2013-09-05 18:57:49]

Changed the SkRect/SkIRect validity tests for the simplified ones.

[reed1, 2013-09-05 19:06:38]

header section lgtm

adding senorblanco for the imagefilters

https://codereview.chromium.org/23021015/diff/77001/include/core/SkRect.h
File include/core/SkRect.h (right):

https://codereview.chromium.org/23021015/diff/77001/include/core/SkRect.h#new...
include/core/SkRect.h:101: bool isEmpty() const { return fLeft >= fRight || fTop
>= fBottom; }
Separate note: given our recent realization about overflow even in ints, perhaps
we should change isEmpty and isInverted...

isEmpty : return fRight - fLeft <= 0
isInverted : return fRight - fLeft < 0

We can think about that after this CL, just wanted to mention it.

[Stephen White, 2013-09-09 17:42:59]

I like it. The changes to the serialization code in the subclasses are nice and
readable. The only bikeshedding nit I'd add is to change validateData() to just
validate(), since validateData() suggests its parameter is data, but it's just a
bool IIRC. (Feel free to ignore me if this has already been discussed.)

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

https://codereview.chromium.org/23021015/diff/77001/src/core/SkImageFilter.cp...
src/core/SkImageFilter.cpp:66: buffer.validateData(SkIsValidRect(fCropRect));
Should we also be validating the number of input SkImageFilters deserialized
against that which we were promised? Or is that handled at a lower level?

https://codereview.chromium.org/23021015/diff/77001/src/effects/SkMatrixConvo...
File src/effects/SkMatrixConvolutionImageFilter.cpp (right):

https://codereview.chromium.org/23021015/diff/77001/src/effects/SkMatrixConvo...
src/effects/SkMatrixConvolutionImageFilter.cpp:61: fKernel =
SkNEW_ARRAY(SkScalar, size);
We should probably add some test cases with impossible-to-allocate sizes, if we
don't have them already.

https://codereview.chromium.org/23021015/diff/77001/src/effects/SkMatrixConvo...
src/effects/SkMatrixConvolutionImageFilter.cpp:69: fConvolveAlpha =
buffer.readBool();
Can readBool() ever get weird values? Or does it internally self-validate?

https://codereview.chromium.org/23021015/diff/77001/src/effects/SkMergeImageF...
File src/effects/SkMergeImageFilter.cpp (right):

https://codereview.chromium.org/23021015/diff/77001/src/effects/SkMergeImageF...
src/effects/SkMergeImageFilter.cpp:162: SkASSERT(buffer.getArrayCount() ==
nbInputs * sizeof(fModes[0]));
Should this also be validated, or is asserting good enough at this point?

[Stephen White, 2013-09-09 17:43:50]

LGTM

[commit-bot: I haz the power, 2013-09-10 14:25:35]

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

[commit-bot: I haz the power, 2013-09-13 12:39:51]

Change committed as 11247

[borenet, 2013-09-13 15:08:37]

The Chrome canary bots are broken, and I think this change is to blame:
http://173.255.115.253:10117/builders/Canary-Chrome-Ubuntu12-Ninja-x86_64-Def...
http://173.255.115.253:10117/builders/Canary-Chrome-Win7-Ninja-x86-SharedLib/...

[sugoi1, 2013-09-13 15:15:54]

On 2013/09/13 15:08:37, borenet wrote:
> The Chrome canary bots are broken, and I think this change is to blame:
>
http://173.255.115.253:10117/builders/Canary-Chrome-Ubuntu12-Ninja-x86_64-Def...
>
http://173.255.115.253:10117/builders/Canary-Chrome-Win7-Ninja-x86-SharedLib/...

Yeah, but the changes in API were supposed to be resolved in the DEPS roll, no ?
The functions simply changed names.

[reed1, 2013-09-13 15:16:31]

[06:17:30.817668] FAILED: g++ -MMD -MF
obj/content/common/content_common.cc_messages.o.d -DCONTENT_IMPLEMENTATION
-DANGLE_DX11 -DWTF_VECTOR_INITIAL_SIZE=16 -D_FILE_OFFSET_BITS=64
-DCHROMIUM_BUILD -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1 -DUSE_NSS=1
-DUSE_X11=1 -DENABLE_ONE_CLICK_SIGNIN -DGTK_DISABLE_SINGLE_INCLUDES=1
-DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_CDMS
-DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS
-DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1
-DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1
-DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1
-DENABLE_GOOGLE_NOW=1 -DENABLE_FULL_PRINTING=1 -DENABLE_PRINTING=1
-DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
-DENABLE_MANAGED_USERS=1 -DENABLE_MDNS=1 -DGL_GLEXT_PROTOTYPES
-DSK_ENABLE_INST_COUNT=0 -DSK_SUPPORT_GPU=1
'-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DSK_USE_POSIX_THREADS
-DSK_DEFERRED_CANVAS_USES_FACTORIES=1 -DU_USING_ICU_NAMESPACE=0
-DU_STATIC_IMPLEMENTATION -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
-DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -I../..
-I../../third_party/khronos -I../../gpu -I../../third_party/skia/src/core
-I../../skia/config -I../../third_party/skia/include/config
-I../../third_party/skia/include/core -I../../third_party/skia/include/effects
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports
-I../../third_party/skia/include/utils -I../../skia/ext
-I../../third_party/icu/source/common -I../../third_party/icu/source/i18n
-Igen/content -Igen/ui/gl -I../../third_party/mesa/src/include
-I../../third_party/WebKit -I../../third_party/npapi
-I../../third_party/npapi/bindings -I../../v8/include -fstack-protector
--param=ssp-buffer-size=4 -Werror -pthread -fno-exceptions -fno-strict-aliasing
-Wall -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden
-pipe -fPIC -pthread -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/gtk-2.0
-I/usr/lib/x86_64-linux-gnu/gtk-2.0/include -I/usr/include/atk-1.0
-I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0
-I/usr/include/gio-unix-2.0/ -I/usr/include/pixman-1 -I/usr/include/freetype2
-I/usr/include/libpng12 -pthread -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -Wno-unknown-pragmas -pthread
-I/usr/include/pango-1.0 -I/usr/include/cairo -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1
-I/usr/include/freetype2 -I/usr/include/libpng12 -O0 -g -fno-rtti
-fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare  -c
../../content/common/cc_messages.cc -o
obj/content/common/content_common.cc_messages.o
[06:17:36.340381] ../../content/common/cc_messages.cc: In static member function
‘static void IPC::ParamTraits<skia::RefPtr<SkImageFilter>
>::Write(IPC::Message*, const param_type&)’:
[06:17:36.340477] ../../content/common/cc_messages.cc:197:77: error:
‘SkSerializeFlattenable’ was not declared in this scope
[06:17:36.340517] ../../content/common/cc_messages.cc: In static member function
‘static bool IPC::ParamTraits<skia::RefPtr<SkImageFilter> >::Read(const
IPC::Message*, PickleIterator*, IPC::ParamTraits<skia::RefPtr<SkImageFilter>
>::param_type*)’:
[06:17:36.340556] ../../content/common/cc_messages.cc:212:71: error:
‘SkDeserializeFlattenable’ was not declared in this scope
[06:17:36.340592] ninja: build stopped: subcommand failed.


We need to fix these, or we'll have to revert this change.

[sugoi1, 2013-09-13 15:23:04]

On 2013/09/13 15:16:31, reed1 wrote:
> [06:17:30.817668] FAILED: g++ -MMD -MF
> obj/content/common/content_common.cc_messages.o.d -DCONTENT_IMPLEMENTATION
> -DANGLE_DX11 -DWTF_VECTOR_INITIAL_SIZE=16 -D_FILE_OFFSET_BITS=64
> -DCHROMIUM_BUILD -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1
-DUSE_NSS=1
> -DUSE_X11=1 -DENABLE_ONE_CLICK_SIGNIN -DGTK_DISABLE_SINGLE_INCLUDES=1
> -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_CDMS
> -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS
> -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DENABLE_TASK_MANAGER=1
-DENABLE_EXTENSIONS=1
> -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1
> -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1
> -DENABLE_GOOGLE_NOW=1 -DENABLE_FULL_PRINTING=1 -DENABLE_PRINTING=1
> -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
> -DENABLE_MANAGED_USERS=1 -DENABLE_MDNS=1 -DGL_GLEXT_PROTOTYPES
> -DSK_ENABLE_INST_COUNT=0 -DSK_SUPPORT_GPU=1
> '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DSK_USE_POSIX_THREADS
> -DSK_DEFERRED_CANVAS_USES_FACTORIES=1 -DU_USING_ICU_NAMESPACE=0
> -DU_STATIC_IMPLEMENTATION -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
> -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG
-I../..
> -I../../third_party/khronos -I../../gpu -I../../third_party/skia/src/core
> -I../../skia/config -I../../third_party/skia/include/config
> -I../../third_party/skia/include/core -I../../third_party/skia/include/effects
> -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu
> -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
> -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports
> -I../../third_party/skia/include/utils -I../../skia/ext
> -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n
> -Igen/content -Igen/ui/gl -I../../third_party/mesa/src/include
> -I../../third_party/WebKit -I../../third_party/npapi
> -I../../third_party/npapi/bindings -I../../v8/include -fstack-protector
> --param=ssp-buffer-size=4 -Werror -pthread -fno-exceptions
-fno-strict-aliasing
> -Wall -Wno-unused-parameter -Wno-missing-field-initializers
-fvisibility=hidden
> -pipe -fPIC -pthread -I/usr/include/glib-2.0
> -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/gtk-2.0
> -I/usr/lib/x86_64-linux-gnu/gtk-2.0/include -I/usr/include/atk-1.0
> -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0
> -I/usr/include/gio-unix-2.0/ -I/usr/include/pixman-1 -I/usr/include/freetype2
> -I/usr/include/libpng12 -pthread -I/usr/include/glib-2.0
> -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -Wno-unknown-pragmas -pthread
> -I/usr/include/pango-1.0 -I/usr/include/cairo -I/usr/include/glib-2.0
> -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1
> -I/usr/include/freetype2 -I/usr/include/libpng12 -O0 -g -fno-rtti
> -fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare  -c
> ../../content/common/cc_messages.cc -o
> obj/content/common/content_common.cc_messages.o
> [06:17:36.340381] ../../content/common/cc_messages.cc: In static member
function
> ‘static void IPC::ParamTraits<skia::RefPtr<SkImageFilter>
> >::Write(IPC::Message*, const param_type&)’:
> [06:17:36.340477] ../../content/common/cc_messages.cc:197:77: error:
> ‘SkSerializeFlattenable’ was not declared in this scope
> [06:17:36.340517] ../../content/common/cc_messages.cc: In static member
function
> ‘static bool IPC::ParamTraits<skia::RefPtr<SkImageFilter> >::Read(const
> IPC::Message*, PickleIterator*, IPC::ParamTraits<skia::RefPtr<SkImageFilter>
> >::param_type*)’:
> [06:17:36.340556] ../../content/common/cc_messages.cc:212:71: error:
> ‘SkDeserializeFlattenable’ was not declared in this scope
> [06:17:36.340592] ninja: build stopped: subcommand failed.
> 
> 
> We need to fix these, or we'll have to revert this change.

We can easily simply revert the name change, no need to revert everything.
Preferably, we should make the changes in content/common/cc_messages.cc, if
that's possible.

[reed1, 2013-09-13 15:28:09]

If you're referring to the change to "Validating" in the api name.. I think that
is an important change. If you can create a CL for the cc file that uses the new
name, that should be straight-forward for Rob to incorporate into our next DEPS
roll.

[sugoi1, 2013-09-18 15:14:03]

I reopened this issue because it had been reverted. The reason why it had been
reverted is that the mechanism I was using to serialize (using
SkFlattenable::FactoryToName()) did not work since, after doing some tests, I
realized that the "factory to name map" isn't initialized in the renderer
process, so the map was empty and no serialization was being written (the
problem was in SkOrderedWriteBuffer.cpp).

I thought about the issue and after a talk with mtklein@, I took a day to
investigate implementing another way of doing the serialization. Here's the
result :
1 ) Each class has an id (or SkFlattenable::Type, in the code) which is used for
serialization. These ids are fixed, so no need for a dictionary of anything like
that. Only an integer is required to identify a class.
2 ) No need to go through the registrar process either. A function is used to
get the CreateProc function from an id. The same functionality can also be used
to perform type checks (through the SkFlattenable::IsA function here) so that we
can effectively verify that a type is what we're expecting before doing the
deserialization. You can see that check in
SkValidatingReadBuffer::readFlattenable(). Note that this would only be used for
secure serialization, since sample code or gms, for example, can't use this
because of the dependency issues this would cause.

Other notes :
- All SkFlattenable derived classes that will be serialized have a corresponding
enum value in SkFlattenable::Type. *** IMPORTANT NOTE *** SkFlattenable objects
outside of the main skia library (like gms, sample code, etc) are not given a
Type, noly those that could require ipc serialization will be given a Type.
- I added macros to easily define GetType() / IsA() / getFlattenableType() in
each SkFlattenable derived class.
- I moved the INHERITED typedef at the top of each class. I had an issue where
using INHERITED inside the header before its declaration would actually use the
INHERITED typedef of the parent class, instead of using its own INHERITED
typedef, effectively skipping the parent class.
- SkFlattenableUtils was created to hold the class specific functions
- readFlattenableT() now provides a type to readFlattenable() so that a type
check can be performed before the deserialization occurs.

Note that I included the canary fix inside this cl to avoid getting the issue.

I know this is quite large, but it includes all the main concepts I wanted to
include in the more secure version of the serialization. Tell me what you think
!

[sugoi1, 2013-09-18 15:16:18]

Oh yeah, there are a lot of changes, but the main changes are in :
include/core/SkFlattenable.h
src/effects/SkFlattenableUtils.cpp (new file)
So you can start with that. Thanks.

[sugoi1, 2013-09-18 18:30:43]

Before I forget, I'll add one more thing :
I'm aware that an easier solution would have been to call SkGraphics::Init() in
the renderer process to fix the issue (I don't know if there's an issue with
that solution, but that's definitely still a possibility).
What I wanted to show here is that using type IDs also allows us to do early
type checking, which is why I'm proposing this other solution. I think it has
more value that just using strings or partial dictionaries to deserialize.
Anyway, I'll be waiting for comments / opinions on the matter. Thanks.

[mtklein, 2013-09-24 22:52:17]

Okay, I have a zillion comments.  Don't be scared.  I have a zillion comments
because I like this approach and I want to see it happen.

I have to admit I did glaze over a bit towards the end.  I promise to look more
carefully there next round.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkAnnotation.h
File include/core/SkAnnotation.h (right):

https://codereview.chromium.org/23021015/diff/94001/include/core/SkAnnotation...
include/core/SkAnnotation.h:24: typedef SkFlattenable INHERITED;
Can you come up with a small test case that shows this matters?  I'm just very
curious.  It seems like it shouldn't.

I tried this code and was unable to reproduce what you're seeing.  It didn't
matter where I put the typedefs, I always get A, B, B.

#include <stdio.h>

struct A {
    virtual void print() { printf("A\n"); }
};  

struct B : public A {
    typedef A INHERITED;
    virtual void print() { printf("B\n"); }
};  

struct C : public B {
    virtual void print() { this->INHERITED::print(); }
    typedef B INHERITED;
};

int main(int, char**) {
    A* a;

    a = new A;
    a->print();
    a = new B;
    a->print();
    a = new C;
    a->print();

    return 0;
}

https://codereview.chromium.org/23021015/diff/94001/include/core/SkColorFilter.h
File include/core/SkColorFilter.h (right):

https://codereview.chromium.org/23021015/diff/94001/include/core/SkColorFilte...
include/core/SkColorFilter.h:126: SK_DECLARE_FLATTENABLE_REGISTRAR_GROUP()
I think we're going to have to work on some of these names.  I know that they
all have something to do with flattening / serialization, but I've got no clue
exactly what each means without looking it up in the header.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenable.h
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:32: #define
SK_DEFINE_FLATTENABLE_TYPE_FCNT(flattenable) \
OK, I give up.  What's FCNT stand for / short for?

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:40: #define SK_DEFINE_FLATTENABLE_TYPE(flattenable)
\
Normally I love macros which cleverly do things like k##flattenable, but I think
as a rule of thumb it should probably only be done when both the macro's
definition and all uses fit on the same screen in the same file.  (Yes, if you
look you'll find places where I've broken this rule myself in the last week...) 
Given that this is spread out over many files and the reader is unlikely to know
what's going on, I think I'd prefer to read

SK_DEFINE_FLATTENABLE_TYPE(SkFoo, SkFlattenable::kSkFoo)

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:48: #define
SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_UTILS() \
May SK_DECLARE_FLATTENABLE_DESERIALIZATION and
SK_DEFINE_FLATTENABLE_DESERIALIZATION for the next one?  What are you trying to
convey with PUBLIC here?  Without seeing this file, I assumed it literally would
be some macro wrapped with a  "public: " and "private: ".

Maybe even we want these to be

SK_{DECLARE,DEFINE}_API_FOR_SK_FLATTENABLE_SERIALIZATION(class)

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:60: #define
SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(flattenable) \
I think we'll be better off in the long term if we cut out these two
middle-level macros.  The fewer SK_DESOMETHING_SOMETHING_ABOUT_SERIALIZATION
macros we have, the more comprehensible this is going to be in the end, even if
that means we need to spend 3 or 4 lines expressing what each class can or can't
do.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:84: enum Type {
Do you have any opinion on whether we want to limit the types which are
compatible with this new code to just the image filters?  Is it easier to
support all SkFlattenables (which I'm assuming is this list?)?  Equivalent for
you from a security perspective?

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:84: enum Type {
It'll help to add some comments here about how this is a closed set of
SkFlattenables that have special serializable properties: type-checking,
stability across binary versions, etc.

I think it's also _essential_ that we remind the reader very loudly that if you
reorder this enum the world will burn down (but appending new values is okay).

It'd also be nice to remind the reader and compiler how many bytes we use to
encode this (presumably 4?) with an SK_COMPILE_ASSERT(kTypeCount < something,
"Sorry, we're out of SkFlattenable::Types!").

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:85: kNone,
Oh I so hate to ask you to do this, but you're going to need to fire up find and
replace.  Skia style wants this to look like

enum Type {
  kNone_Type,
  kSkFlattenable_Type,
  kBATShader_Type,
  ...
  kTypeCount
};

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:183: };
Can you add a comment betwe kSrcOver_SkModeColorFilter and kTypeCount that says
something like "// New values go here"?  Not always obvious that the last guy is
a floating count.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:202: /** Returns the class id
Let's flesh out this (and related comments):
  /** GetType methods on classes descending from SkFlattenable should return the
SkFlattenable::Type associated with that class, or SkFlattenable::kNoneType if
they're not part of the subset of SkFlattenables that can be used with
TypeToFactory.

  (Typically you'd use SK_MACRO_FOO to define GetType(), getFlattenableType()
and isA() in any subclass of SkFlattenable, or SK_MACRO_BAR will define them for
you to return kNone.)

Something like that?

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:226: static bool TypeIsA(SkFlattenable::Type typeA,
SkFlattenable::Type typeB);
Does this need to be public?

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenableBuffers.h:73: virtual SkFlattenable* readFlattenable(
Add a note about what the argument means?  Presumably, return NULL if the
serialized object is not a type?  Does isValidating() have to be set?

https://codereview.chromium.org/23021015/diff/94001/include/core/SkRect.h
File include/core/SkRect.h (right):

https://codereview.chromium.org/23021015/diff/94001/include/core/SkRect.h#new...
include/core/SkRect.h:103: bool isInverted() const { return fLeft > fRight ||
fTop > fBottom; }
Is there any performance lost by implementing this inline in SkValidationUtils? 
Not sure I see why this needs to be a new method if only used there.

https://codereview.chromium.org/23021015/diff/94001/include/effects/SkFlatten...
File include/effects/SkFlattenableUtils.h (right):

https://codereview.chromium.org/23021015/diff/94001/include/effects/SkFlatten...
include/effects/SkFlattenableUtils.h:17: SkFlattenable::Factory
TypeToFactory(SkFlattenable::Type type);
I'm confused why we have both these two functions and the suspiciously similar
static methods on SkFlattenable.

What does this return for kNone?  kSkFlattenable?  Other abstract classes? 
Important to document. I guess, a factory that always returns NULL?

https://codereview.chromium.org/23021015/diff/94001/src/core/SkFlattenableSer...
File src/core/SkFlattenableSerialization.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/core/SkFlattenableSer...
src/core/SkFlattenableSerialization.cpp:25: SkFlattenable*
SkValidatingDeserializeFlattenable(const void* data, size_t size) {
I noticed a comment on the chromium code that hooked into this that we'll want
to avoid the cast from SkFlattenable* back to SkImageFilter*.  Planning to
follow up by adding our spiffy new type checking here?

https://codereview.chromium.org/23021015/diff/94001/src/core/SkOrderedReadBuf...
File src/core/SkOrderedReadBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/core/SkOrderedReadBuf...
src/core/SkOrderedReadBuffer.cpp:271: SkFlattenable*
SkOrderedReadBuffer::readFlattenable(SkFlattenable::Type) {
A little // We ignore this Type parameter here because we're assuming the data
in the SkOrderedReadBuffer is trusted and we don't need to do type checking ?

Now that I'm thinking about it, I'll have to remember to come back through here
and _do_ use the Type for type checking in Debug builds, at least where
possible.

https://codereview.chromium.org/23021015/diff/94001/src/core/SkOrderedWriteBu...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/core/SkOrderedWriteBu...
src/core/SkOrderedWriteBuffer.cpp:273: if (this->isValidating()) {
Seems like we just shouldn't get into this situation if isValidating is true?

https://codereview.chromium.org/23021015/diff/94001/src/core/SkOrderedWriteBu...
src/core/SkOrderedWriteBuffer.cpp:284: *  We can write 1 of 3 versions of the
flattenable:
Needs an update :)

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
src/core/SkValidatingReadBuffer.cpp:92: }
Some comments in here about our validation strategy would be very helpful.

I gather it's something like

// All the methods in this file funnel down into either readInt() or skip()
followed by a memcpy.
// So we've got all our validation in readInt() and skip(); if they fail they'll
return a zero value or skip nothing, respectively.

Some sort of visual segregation of 
  -- these are the primitives we trust
  -- these are the core validating functions
  -- these are methods that build only on the core
would really help maintenance here.

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
src/core/SkValidatingReadBuffer.cpp:272: // failure was ours, not the writer.
SkASSERT it doesn't happen?

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
File src/core/SkValidatingReadBuffer.h (right):

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
src/core/SkValidatingReadBuffer.h:24: class SkValidatingReadBuffer : public
SkFlattenableReadBuffer {
Looks like a solid copy and paste.  :)  How much of this API (and members) can
we cut away?  At a glance, seems like getReader32 should go, as should the
various setBitmapStorage, setTypefaceArray, setFactoryPlayback, etc.

In fact, are there any methods that aren't SK_OVERRIDE that we can't remove? 
Let's just blow away the lot to get this as trimmed down as possible.

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
src/core/SkValidatingReadBuffer.h:116: static bool ptr_align_4(const void* ptr)
{
I think we typically make static methods camel case.  IsPtrAlign4?

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
src/core/SkValidatingReadBuffer.h:117: return (((const char*)ptr - (const
char*)NULL) & 3) == 0;
return (uintptr_t)ptr & 3 == 0?,  or better yet SkIsAlign4((uintptr_t)ptr) ?

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
src/core/SkValidatingReadBuffer.h:133: #ifdef DEBUG_NON_DETERMINISTIC_ASSERT
I think we can throw this feature out too for now.  It's probably not even used
in SkOrderedReadBuffer anymore.

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidationUtils.h
File src/core/SkValidationUtils.h (right):

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidationUtil...
src/core/SkValidationUtils.h:16: static inline bool
SkIsValidCoeff(SkXfermode::Coeff coeff) {
I sort of want these to live with their corresponding enums.  It'll teach people
to write them by example.

I acknowledge that this somewhat contradicts my comment in SkRect about moving
all the code here.  Maybe I was wrong and all the isValid methods belong with
the types?

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidationUtil...
src/core/SkValidationUtils.h:34: static inline bool SkIsValidRect(const SkIRect&
rect) {
This is going to be a little confusing.  Let's call it SkIsValidIRect.

That is, unless you want this to just be 

template <typename T>
bool SkIsValid(const T& t);

and then specialize

template <> bool SkIsValid(const SkIRect& irect) { ... }
template <> bool SkIsValid(const SkRect& rect) { ... }

etc.  I think we can argue for either way, though Skia does tend to follow the
old C way of dispatching by putting the type in the method name as you've got
here.  Skia's kind of crotchety like that.  We're even still suspicious of this
newfangled STL and C++98...

https://codereview.chromium.org/23021015/diff/94001/src/effects/SkArithmeticM...
File src/effects/SkArithmeticMode.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/effects/SkArithmeticM...
src/effects/SkArithmeticMode.cpp:235: switch(type) {
For these small guys, I think you're going to be leaving folks scratching their
heads why you didn't write

if (SkFlattenable::kSkArithmeticMode_scalar == type) {
  return SkArithmeticMode_scalar::CreateProc;
}
return NULL;

Similar below and I'm guessing on many files to come.

https://codereview.chromium.org/23021015/diff/94001/src/effects/SkBicubicImag...
File src/effects/SkBicubicImageFilter.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/effects/SkBicubicImag...
src/effects/SkBicubicImageFilter.cpp:48:
buffer.validate(SkScalarIsFinite(fScale.fWidth) &&
The more I read these, the more I want to rename validate to assert or check or
failIfNot.

https://codereview.chromium.org/23021015/diff/94001/src/effects/SkFlattenable...
File src/effects/SkFlattenableUtils.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/effects/SkFlattenable...
src/effects/SkFlattenableUtils.cpp:77: const TypeFunctions&
GetFunctions(SkFlattenable::Type type) {
We're going to have to come up with some way to manage the complexity here. 
This is sort of, unmaintainable.

Is there any way we can make this part of the registration?  If not, this may be
the place to use a judicious macro or two.

[sugoi1, 2013-09-25 21:15:26]

I quickly fixed anything that was trivial to fix and I tried to answer all the
comments.

More to come tomorrow.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkAnnotation.h
File include/core/SkAnnotation.h (right):

https://codereview.chromium.org/23021015/diff/94001/include/core/SkAnnotation...
include/core/SkAnnotation.h:24: typedef SkFlattenable INHERITED;
On 2013/09/24 22:52:18, mtklein wrote:
> Can you come up with a small test case that shows this matters?  I'm just very
> curious.  It seems like it shouldn't.
> 
> I tried this code and was unable to reproduce what you're seeing.  It didn't
> matter where I put the typedefs, I always get A, B, B.
> 
> #include <stdio.h>
> 
> struct A {
>     virtual void print() { printf("A\n"); }
> };  
> 
> struct B : public A {
>     typedef A INHERITED;
>     virtual void print() { printf("B\n"); }
> };  
> 
> struct C : public B {
>     virtual void print() { this->INHERITED::print(); }
>     typedef B INHERITED;
> };
> 
> int main(int, char**) {
>     A* a;
> 
>     a = new A;
>     a->print();
>     a = new B;
>     a->print();
>     a = new C;
>     a->print();
> 
>     return 0;
> }

I got this error while writing some code last week, and I found it to be pretty
bad. Essentially, it works for calling virtual functions, but it doesn't work
for function parameters. For example, If I'd write this :

struct A {
    enum Type { k0, k1, k2 };
};

struct B : public A {
    enum Type { kA, kB, kC };
    typedef A INHERITED;
};  

struct C : public B {
    void doSomething(INHERITED::Type type = INHERITED::kA) {} // Error here
    typedef B INHERITED;
};

I'd get an error :

error: default argument for parameter of type ‘A::Type’ has type ‘B::Type’

which is fairly easy to miss, since, looking at the code, we'd think both the
parameter type and the default argument are of the same type.

If I change the struct C to :

struct C : public B {
    typedef B INHERITED;
    void doSomething(INHERITED::Type type = INHERITED::kA) {}
};

then everything works fine. I didn't want to have issues or limitations because
of this and I didn't want others to get puzzled by errors like these, so I
decided to preemptively fix this kind of thing right away.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkColorFilter.h
File include/core/SkColorFilter.h (right):

https://codereview.chromium.org/23021015/diff/94001/include/core/SkColorFilte...
include/core/SkColorFilter.h:126: SK_DECLARE_FLATTENABLE_REGISTRAR_GROUP()
On 2013/09/24 22:52:18, mtklein wrote:
> I think we're going to have to work on some of these names.  I know that they
> all have something to do with flattening / serialization, but I've got no clue
> exactly what each means without looking it up in the header.

You're right.

SK_DEFINE_FLATTENABLE_TYPE defines all the type related functions like GetType()
and IsA(), so this is all "type checking" related.

SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_UTILS could easily have a better
name. It declares static accessor functions required by every SkFlattenable
derived class that doesn't expose them through their public APIs. Currently, it
declares functions to return function pointers to the "factory" and the "type
checking" functions of each class. Again, this is only necessary if these aren't
accessible in the public section of a class in a header file.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenable.h
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:32: #define
SK_DEFINE_FLATTENABLE_TYPE_FCNT(flattenable) \
On 2013/09/24 22:52:18, mtklein wrote:
> OK, I give up.  What's FCNT stand for / short for?

Sorry, typo, I meant FNCT, as in function. The name is pretty generic. I wasn't
particularly inspired for that macro :)

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:40: #define SK_DEFINE_FLATTENABLE_TYPE(flattenable)
\
On 2013/09/24 22:52:18, mtklein wrote:
> Normally I love macros which cleverly do things like k##flattenable, but I
think
> as a rule of thumb it should probably only be done when both the macro's
> definition and all uses fit on the same screen in the same file.  (Yes, if you
> look you'll find places where I've broken this rule myself in the last
week...) 
> Given that this is spread out over many files and the reader is unlikely to
know
> what's going on, I think I'd prefer to read
> 
> SK_DEFINE_FLATTENABLE_TYPE(SkFoo, SkFlattenable::kSkFoo)

I not against this, but I sort of like the fact that the macro user didn't have
to care about the type and that it would be automatically created for him. The
reason why I hadn't done this is that you have to add you enum inside
SkFlattenable::Type anyway, so the compiler will tell you what's going on even
if you just write SK_DEFINE_FLATTENABLE_TYPE(SkFoo) since it won't compile until
you add your enum value here.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:48: #define
SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_UTILS() \
On 2013/09/24 22:52:18, mtklein wrote:
> May SK_DECLARE_FLATTENABLE_DESERIALIZATION and
> SK_DEFINE_FLATTENABLE_DESERIALIZATION for the next one?  What are you trying
to
> convey with PUBLIC here?  Without seeing this file, I assumed it literally
would
> be some macro wrapped with a  "public: " and "private: ".
> 
> Maybe even we want these to be
> 
> SK_{DECLARE,DEFINE}_API_FOR_SK_FLATTENABLE_SERIALIZATION(class)

I didn't invent the PUBLIC name, it was there before, I simply cut it into
pieces, but I can rename the old one too.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:60: #define
SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(flattenable) \
On 2013/09/24 22:52:18, mtklein wrote:
> I think we'll be better off in the long term if we cut out these two
> middle-level macros.  The fewer SK_DESOMETHING_SOMETHING_ABOUT_SERIALIZATION
> macros we have, the more comprehensible this is going to be in the end, even
if
> that means we need to spend 3 or 4 lines expressing what each class can or
can't
> do.

All right, I'll try to combine them into fewer macros so that each class has
less macros in its declaration.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:84: enum Type {
On 2013/09/24 22:52:18, mtklein wrote:
> Do you have any opinion on whether we want to limit the types which are
> compatible with this new code to just the image filters?  Is it easier to
> support all SkFlattenables (which I'm assuming is this list?)?  Equivalent for
> you from a security perspective?

I don't see any reason to exclude types from it. This allows type checking to
differentiate between "wrong type" and "complete garbage" (I'm not doing this in
this cl, but it's possible). It also makes it more future proof. If we'd depend
on this for security, it would mean that we'd open security breaches when adding
types in this enum, which would be pretty bad. I'm counting on type checking to
make the unused types irrelevant, and therefore not a security issue.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:84: enum Type {
On 2013/09/24 22:52:18, mtklein wrote:
> It'll help to add some comments here about how this is a closed set of
> SkFlattenables that have special serializable properties: type-checking,
> stability across binary versions, etc.
> 
> I think it's also _essential_ that we remind the reader very loudly that if
you
> reorder this enum the world will burn down (but appending new values is okay).
> 
> It'd also be nice to remind the reader and compiler how many bytes we use to
> encode this (presumably 4?) with an SK_COMPILE_ASSERT(kTypeCount < something,
> "Sorry, we're out of SkFlattenable::Types!").

I added the comment about not reordering and a note about types, but I'll wait
until this is complete to see if I can add the compile assert. I'm not certain
how to handle (and if we have to handle) SkFlattenable classes declared outside
of skia yet. I'm hoping that won't be necessary.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:85: kNone,
On 2013/09/24 22:52:18, mtklein wrote:
> Oh I so hate to ask you to do this, but you're going to need to fire up find
and
> replace.  Skia style wants this to look like
> 
> enum Type {
>   kNone_Type,
>   kSkFlattenable_Type,
>   kBATShader_Type,
>   ...
>   kTypeCount
> };

No problem, that's easy enough.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:183: };
On 2013/09/24 22:52:18, mtklein wrote:
> Can you add a comment betwe kSrcOver_SkModeColorFilter and kTypeCount that
says
> something like "// New values go here"?  Not always obvious that the last guy
is
> a floating count.

Done.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:202: /** Returns the class id
On 2013/09/24 22:52:18, mtklein wrote:
> Let's flesh out this (and related comments):
>   /** GetType methods on classes descending from SkFlattenable should return
the
> SkFlattenable::Type associated with that class, or SkFlattenable::kNoneType if
> they're not part of the subset of SkFlattenables that can be used with
> TypeToFactory.
> 
>   (Typically you'd use SK_MACRO_FOO to define GetType(), getFlattenableType()
> and isA() in any subclass of SkFlattenable, or SK_MACRO_BAR will define them
for
> you to return kNone.)
> 
> Something like that?

Done.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenable.h:226: static bool TypeIsA(SkFlattenable::Type typeA,
SkFlattenable::Type typeB);
On 2013/09/24 22:52:18, mtklein wrote:
> Does this need to be public?

It needs to be accessible inside SkValidatingReadBuffer::readFlattenable(), so
most likely this needs to be public.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
File include/core/SkFlattenableBuffers.h (right):

https://codereview.chromium.org/23021015/diff/94001/include/core/SkFlattenabl...
include/core/SkFlattenableBuffers.h:73: virtual SkFlattenable* readFlattenable(
On 2013/09/24 22:52:18, mtklein wrote:
> Add a note about what the argument means?  Presumably, return NULL if the
> serialized object is not a type?  Does isValidating() have to be set?

Done.

https://codereview.chromium.org/23021015/diff/94001/include/core/SkRect.h
File include/core/SkRect.h (right):

https://codereview.chromium.org/23021015/diff/94001/include/core/SkRect.h#new...
include/core/SkRect.h:103: bool isInverted() const { return fLeft > fRight ||
fTop > fBottom; }
On 2013/09/24 22:52:18, mtklein wrote:
> Is there any performance lost by implementing this inline in
SkValidationUtils? 
> Not sure I see why this needs to be a new method if only used there.

It's just that I'm using this functionality in SkValidationUtils.h and in
SkImageFilter.cpp, so I didn't want to duplicate it. Also, the word "inverted"
is already used in this file with "setLargestInverted()", so it seemed to follow
the general nomenclature of SkRect.

https://codereview.chromium.org/23021015/diff/94001/include/effects/SkFlatten...
File include/effects/SkFlattenableUtils.h (right):

https://codereview.chromium.org/23021015/diff/94001/include/effects/SkFlatten...
include/effects/SkFlattenableUtils.h:17: SkFlattenable::Factory
TypeToFactory(SkFlattenable::Type type);
On 2013/09/24 22:52:18, mtklein wrote:
> I'm confused why we have both these two functions and the suspiciously similar
> static methods on SkFlattenable.
> 
> What does this return for kNone?  kSkFlattenable?  Other abstract classes? 
> Important to document. I guess, a factory that always returns NULL?

It's only a question of dependencies. I need to be in "effects" to have access
to all the SkFlattenable derived classes, but SkFlattenable is in "core", so I
wrote the bulk of the functionality in a separate namespace in the effects
library.

https://codereview.chromium.org/23021015/diff/94001/src/core/SkFlattenableSer...
File src/core/SkFlattenableSerialization.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/core/SkFlattenableSer...
src/core/SkFlattenableSerialization.cpp:25: SkFlattenable*
SkValidatingDeserializeFlattenable(const void* data, size_t size) {
On 2013/09/24 22:52:18, mtklein wrote:
> I noticed a comment on the chromium code that hooked into this that we'll want
> to avoid the cast from SkFlattenable* back to SkImageFilter*.  Planning to
> follow up by adding our spiffy new type checking here?

Absolutely, that's the main reason why I have to create this new type checking
code :)

https://codereview.chromium.org/23021015/diff/94001/src/core/SkOrderedReadBuf...
File src/core/SkOrderedReadBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/core/SkOrderedReadBuf...
src/core/SkOrderedReadBuffer.cpp:271: SkFlattenable*
SkOrderedReadBuffer::readFlattenable(SkFlattenable::Type) {
On 2013/09/24 22:52:18, mtklein wrote:
> A little // We ignore this Type parameter here because we're assuming the data
> in the SkOrderedReadBuffer is trusted and we don't need to do type checking ?
> 
> Now that I'm thinking about it, I'll have to remember to come back through
here
> and _do_ use the Type for type checking in Debug builds, at least where
> possible.

Done.

https://codereview.chromium.org/23021015/diff/94001/src/core/SkOrderedWriteBu...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/core/SkOrderedWriteBu...
src/core/SkOrderedWriteBuffer.cpp:273: if (this->isValidating()) {
On 2013/09/24 22:52:18, mtklein wrote:
> Seems like we just shouldn't get into this situation if isValidating is true?

I added an assert.

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
src/core/SkValidatingReadBuffer.cpp:92: }
On 2013/09/24 22:52:18, mtklein wrote:
> Some comments in here about our validation strategy would be very helpful.
> 
> I gather it's something like
> 
> // All the methods in this file funnel down into either readInt() or skip()
> followed by a memcpy.
> // So we've got all our validation in readInt() and skip(); if they fail
they'll
> return a zero value or skip nothing, respectively.

I've added the comment.

> Some sort of visual segregation of 
>   -- these are the primitives we trust
>   -- these are the core validating functions
>   -- these are methods that build only on the core
> would really help maintenance here.

Can you give me an example of what you mean here ? This can be very short or
very detailed, I just want to know what you're expecting. Thanks.

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
src/core/SkValidatingReadBuffer.cpp:272: // failure was ours, not the writer.
On 2013/09/24 22:52:18, mtklein wrote:
> SkASSERT it doesn't happen?

Done.

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
File src/core/SkValidatingReadBuffer.h (right):

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
src/core/SkValidatingReadBuffer.h:24: class SkValidatingReadBuffer : public
SkFlattenableReadBuffer {
On 2013/09/24 22:52:18, mtklein wrote:
> Looks like a solid copy and paste.  :)  How much of this API (and members) can
> we cut away?  At a glance, seems like getReader32 should go, as should the
> various setBitmapStorage, setTypefaceArray, setFactoryPlayback, etc.
> 
> In fact, are there any methods that aren't SK_OVERRIDE that we can't remove? 
> Let's just blow away the lot to get this as trimmed down as possible.

I removed everything except skip(), which I'm using, and readTypeface() now
returns NULL until we need to implement it (but I still need to declare it
here).

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
src/core/SkValidatingReadBuffer.h:116: static bool ptr_align_4(const void* ptr)
{
On 2013/09/24 22:52:18, mtklein wrote:
> I think we typically make static methods camel case.  IsPtrAlign4?

Done.

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
src/core/SkValidatingReadBuffer.h:117: return (((const char*)ptr - (const
char*)NULL) & 3) == 0;
On 2013/09/24 22:52:18, mtklein wrote:
> return (uintptr_t)ptr & 3 == 0?,  or better yet SkIsAlign4((uintptr_t)ptr) ?

Done.

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidatingRead...
src/core/SkValidatingReadBuffer.h:133: #ifdef DEBUG_NON_DETERMINISTIC_ASSERT
On 2013/09/24 22:52:18, mtklein wrote:
> I think we can throw this feature out too for now.  It's probably not even
used
> in SkOrderedReadBuffer anymore.

Done.

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidationUtils.h
File src/core/SkValidationUtils.h (right):

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidationUtil...
src/core/SkValidationUtils.h:16: static inline bool
SkIsValidCoeff(SkXfermode::Coeff coeff) {
On 2013/09/24 22:52:18, mtklein wrote:
> I sort of want these to live with their corresponding enums.  It'll teach
people
> to write them by example.
> 
> I acknowledge that this somewhat contradicts my comment in SkRect about moving
> all the code here.  Maybe I was wrong and all the isValid methods belong with
> the types?

I'm not sure about this, but I don't have a strong opinion either way. I'll
follow any guideline that everyone else agrees on :)

https://codereview.chromium.org/23021015/diff/94001/src/core/SkValidationUtil...
src/core/SkValidationUtils.h:34: static inline bool SkIsValidRect(const SkIRect&
rect) {
On 2013/09/24 22:52:18, mtklein wrote:
> This is going to be a little confusing.  Let's call it SkIsValidIRect.

Done.

> That is, unless you want this to just be 
> 
> template <typename T>
> bool SkIsValid(const T& t);
> 
> and then specialize
> 
> template <> bool SkIsValid(const SkIRect& irect) { ... }
> template <> bool SkIsValid(const SkRect& rect) { ... }
> 
> etc.  I think we can argue for either way, though Skia does tend to follow the
> old C way of dispatching by putting the type in the method name as you've got
> here.  Skia's kind of crotchety like that.  We're even still suspicious of
this
> newfangled STL and C++98...

Yeah, I enjoy templates very much, but it's not really necessary to use them
here, I think. I use them mainly when there's a good use case to significantly
reduce the number of lines of code I have to write or when performance is
critical and I don't care about have the same loop 20 times, with slight
modifications, bloating the executable.

https://codereview.chromium.org/23021015/diff/94001/src/effects/SkArithmeticM...
File src/effects/SkArithmeticMode.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/effects/SkArithmeticM...
src/effects/SkArithmeticMode.cpp:235: switch(type) {
On 2013/09/24 22:52:18, mtklein wrote:
> For these small guys, I think you're going to be leaving folks scratching
their
> heads why you didn't write
> 
> if (SkFlattenable::kSkArithmeticMode_scalar == type) {
>   return SkArithmeticMode_scalar::CreateProc;
> }
> return NULL;
> 
> Similar below and I'm guessing on many files to come.

Done.

https://codereview.chromium.org/23021015/diff/94001/src/effects/SkBicubicImag...
File src/effects/SkBicubicImageFilter.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/effects/SkBicubicImag...
src/effects/SkBicubicImageFilter.cpp:48:
buffer.validate(SkScalarIsFinite(fScale.fWidth) &&
On 2013/09/24 22:52:18, mtklein wrote:
> The more I read these, the more I want to rename validate to assert or check
or
> failIfNot.

Again, as long as everyone agrees on a name, I have no attachment to function
names, as long as they're somewhat clear about what they do.

https://codereview.chromium.org/23021015/diff/94001/src/effects/SkFlattenable...
File src/effects/SkFlattenableUtils.cpp (right):

https://codereview.chromium.org/23021015/diff/94001/src/effects/SkFlattenable...
src/effects/SkFlattenableUtils.cpp:77: const TypeFunctions&
GetFunctions(SkFlattenable::Type type) {
On 2013/09/24 22:52:18, mtklein wrote:
> We're going to have to come up with some way to manage the complexity here. 
> This is sort of, unmaintainable.
> 
> Is there any way we can make this part of the registration?  If not, this may
be
> the place to use a judicious macro or two.

Agreed. I'll try to think of a way to make the code clearer here. It shouldn't
be too hard :)

[sugoi1, 2013-09-26 19:23:15]

Ok, here's the new version. Things to note :

- Cleaned up SkArithmeticMode by removing its inheritance to SkXfermode, which
wasn't necessary, and made the class uninstantiatable.
- Made macros in SkFlattenable.h such that each class only needs 1 line  to
declare the required SkFlattenable functionality
- Added macros to SkFlattenableUtils.cpp to simplify the code, I also reordered
them since the registration order had no importance (first section is ordered
alphabetically by class name, and 2nd section is ordered alphabetically by base
class name). It should look much cleaner now.

[sugoi1, 2013-09-27 18:24:46]

Minor update:
- Fixed windows warnings as errors (that was a bad merge on my side)
- Added validation code to the new SkTileImageFilter class
- Reverted changes to SkFlattenableSerialization.[h,cpp]. I think it's too early
for this. The reason for this is that I don't want to get burned by the canary
build and have to revert this cl. By not using SkValidateReadBuffer right away
in Chrome, it should prevent most issues that Chrome would have with this cl. So
my cl strategy would be :
1 ) Land this cl
2 ) Land the follow up (https://codereview.chromium.org/23548034/)
3 ) Land the integration of serialization (using SkValidateReadBuffer) into the
fuzzer (and perform some tests afterwards)
4 ) Land changes to SkFlattenableSerialization.[h,cpp].

[sugoi1, 2013-10-02 17:19:14]

I reverted the typedef changes that were put into
https://codereview.chromium.org/25430005/ from this cl so that the 2 cls are now
independent from each other.

[Stephen White, 2013-10-02 17:41:42]

https://codereview.chromium.org/23021015/diff/132001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/132001/include/core/SkFlattenab...
include/core/SkFlattenable.h:60: /** For SkFlattenable derived objects defined
outside of the skia (sample code, gms, etc).
Nit: the skia?

https://codereview.chromium.org/23021015/diff/132001/include/core/SkFlattenab...
include/core/SkFlattenable.h:61: Since these external object won't go through
the ipc serialization, they are not given
Nit: Does skia know that this functionality is used for IPC, or is that
Chrome-specific? If the latter, perhaps we don't need to mention it here. Maybe
"won't go through secure deserialization"?

https://codereview.chromium.org/23021015/diff/132001/include/core/SkFlattenab...
include/core/SkFlattenable.h:65: #define
SK_DEFINE_FLATTENABLE_EXTERN(flattenable) \
<bikeshed> It might be a good idea to name this SK_DEFINE_FLATTENABLE_NONSECURE,
or _INSECURE, so it's easy to spot accidental use in sensitive code. </bikeshed>

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

https://codereview.chromium.org/23021015/diff/132001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:121: fError |= !isValid;
Nit: bitwise operator on a bool; I prefer the logical operators for bools, but
perhaps I'm just pedantic

[sugoi1, 2013-10-02 19:59:32]

https://codereview.chromium.org/23021015/diff/132001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/132001/include/core/SkFlattenab...
include/core/SkFlattenable.h:60: /** For SkFlattenable derived objects defined
outside of the skia (sample code, gms, etc).
On 2013/10/02 17:41:43, Stephen White wrote:
> Nit: the skia?

Haha. I wanted to say either "the skia library" or "skia", although "the skia"
gives it some character :) Fixed.

https://codereview.chromium.org/23021015/diff/132001/include/core/SkFlattenab...
include/core/SkFlattenable.h:61: Since these external object won't go through
the ipc serialization, they are not given
On 2013/10/02 17:41:43, Stephen White wrote:
> Nit: Does skia know that this functionality is used for IPC, or is that
> Chrome-specific? If the latter, perhaps we don't need to mention it here.
Maybe
> "won't go through secure deserialization"?

Done.

https://codereview.chromium.org/23021015/diff/132001/include/core/SkFlattenab...
include/core/SkFlattenable.h:65: #define
SK_DEFINE_FLATTENABLE_EXTERN(flattenable) \
On 2013/10/02 17:41:43, Stephen White wrote:
> <bikeshed> It might be a good idea to name this
SK_DEFINE_FLATTENABLE_NONSECURE,
> or _INSECURE, so it's easy to spot accidental use in sensitive code.
</bikeshed>

It could also be something generic, like _TYPELESS, although I like the idea of
having "insecure" classes :)

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

https://codereview.chromium.org/23021015/diff/132001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:121: fError |= !isValid;
On 2013/10/02 17:41:43, Stephen White wrote:
> Nit: bitwise operator on a bool; I prefer the logical operators for bools, but
> perhaps I'm just pedantic

Done.

[reed1, 2013-10-03 08:41:08]

From 10K feet, I think I appreciate wanting to flag unflatten_flattenable with
the desired/expected effect type [shader, xfermode, etc], since our unflattening
code needs to do a cast to that type. I don't understand the requirement that we
encode the exact subclass in the stream (and I find that way ucky, and
un-library-like).

Is there an overview doc that describes the need/use-case for these various
proposals?

https://codereview.chromium.org/23021015/diff/138001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/138001/include/core/SkFlattenab...
include/core/SkFlattenable.h:135: enum Type {
*if* we have to have an enumeration of known subclasses, lets store it outside
of this header. In fact, can we structure the code so that Skia itself doesn't
need to know this list at all, but it can be something the client specifies
(both for writing, and for verifying during reading)?

https://codereview.chromium.org/23021015/diff/138001/include/core/SkRect.h
File include/core/SkRect.h (right):

https://codereview.chromium.org/23021015/diff/138001/include/core/SkRect.h#ne...
include/core/SkRect.h:103: bool isInverted() const { return fLeft > fRight ||
fTop > fBottom; }
Starting to get odd to have isEmpty and isInverted. Does inverted need to be in
the public header?

If we have a rect which is { 1, 1, 1, 0 }, is it empty or inverted? At the
moment, that depends on which predicate you call.

If this is *only* used by validation, perhaps it should live in some
validation-specific (private?) header.

https://codereview.chromium.org/23021015/diff/138001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/23021015/diff/138001/include/core/SkXfermode....
include/core/SkXfermode.h:263:
SK_DECLARE_FLATTENABLE_BASE_SERIALIZABLE_TYPE(SkProcXfermode)
What does base_serializable_type mean? My first guess was that this would mean
"what base class am I derived from", which would be xfermode in this case.

[sugoi1, 2013-10-03 14:58:23]

On 2013/10/03 08:41:08, reed1 wrote:
> From 10K feet, I think I appreciate wanting to flag unflatten_flattenable with
> the desired/expected effect type [shader, xfermode, etc], since our
unflattening
> code needs to do a cast to that type. I don't understand the requirement that
we
> encode the exact subclass in the stream (and I find that way ucky, and
> un-library-like).

What do you mean by "encode the exact subclass in the stream" ? There's always
been a string or an ID or something to identify the exact subclass in the
stream, otherwise, we wouldn't be able to deserialize it.

> Is there an overview doc that describes the need/use-case for these various
proposals?

The only use case right now is transferring SkImageFilter objects over the IPC
channel in chromium. What level of detail do you need for these use cases ?
 
>
https://codereview.chromium.org/23021015/diff/138001/include/core/SkFlattenab...
> File include/core/SkFlattenable.h (right):
> 
>
https://codereview.chromium.org/23021015/diff/138001/include/core/SkFlattenab...
> include/core/SkFlattenable.h:135: enum Type {
> *if* we have to have an enumeration of known subclasses, lets store it outside
> of this header. In fact, can we structure the code so that Skia itself doesn't
> need to know this list at all, but it can be something the client specifies
> (both for writing, and for verifying during reading)?

I can put it in a different header, but asking a client to know every class in
skia in not really reliable when we add new classes in skia and makes secure
IsA() checks harder to implement. I'm not sure that would be viable in the
longer term.

> https://codereview.chromium.org/23021015/diff/138001/include/core/SkRect.h
> File include/core/SkRect.h (right):
> 
>
https://codereview.chromium.org/23021015/diff/138001/include/core/SkRect.h#ne...
> include/core/SkRect.h:103: bool isInverted() const { return fLeft > fRight ||
> fTop > fBottom; }
> Starting to get odd to have isEmpty and isInverted. Does inverted need to be
in
> the public header?

I can move it to SkValidationUtils.h if you want.

> If we have a rect which is { 1, 1, 1, 0 }, is it empty or inverted? At the
> moment, that depends on which predicate you call.

Does it really matter that it can be both ? An inverted rectangle is always
empty by our definition of empty since empty includes inverted rectangles.

> If this is *only* used by validation, perhaps it should live in some
> validation-specific (private?) header.
> 
> https://codereview.chromium.org/23021015/diff/138001/include/core/SkXfermode.h
> File include/core/SkXfermode.h (right):
> 
>
https://codereview.chromium.org/23021015/diff/138001/include/core/SkXfermode....
> include/core/SkXfermode.h:263:
> SK_DECLARE_FLATTENABLE_BASE_SERIALIZABLE_TYPE(SkProcXfermode)
> What does base_serializable_type mean? My first guess was that this would mean
> "what base class am I derived from", which would be xfermode in this case.

These macros are all defined in SkFlattenable.h. Specifically,
SK_DECLARE_FLATTENABLE_BASE_SERIALIZABLE_TYPE means that it's a BASE class to
other classes unavailable in header files (most likely because their
implementation is hidden in a cpp file), that it's SERIALIZABLE through the
regular (non-secure) serialization, and that it has a TYPE, which means that we
can make type checks on it (and use its type for the secure serialization). I
use keywords to avoid 200 character long macro names. Each one is explained with
a comment in SkFlattenable.h.

[reed1, 2013-10-03 15:25:29]

On 2013/10/03 14:58:23, sugoi1 wrote:
> On 2013/10/03 08:41:08, reed1 wrote:
> > From 10K feet, I think I appreciate wanting to flag unflatten_flattenable
with
> > the desired/expected effect type [shader, xfermode, etc], since our
> unflattening
> > code needs to do a cast to that type. I don't understand the requirement
that
> we
> > encode the exact subclass in the stream (and I find that way ucky, and
> > un-library-like).
> 
> What do you mean by "encode the exact subclass in the stream" ? There's always
> been a string or an ID or something to identify the exact subclass in the
> stream, otherwise, we wouldn't be able to deserialize it.

Right. Why isn't that enough? I can see some value in specifying the expected
"baseclass" of the effect at Factory time (e.g. I expect a subclass of
SkShader), but I don't see the need to specify an enum for the particular
subclass of Shader. The former "baseclass" info is core to Skia, and need not be
kept in-sync with any particular client's subclasses, so it feels like a
reasonable addition to the core.

> 
> > Is there an overview doc that describes the need/use-case for these various
> proposals?
> 
> The only use case right now is transferring SkImageFilter objects over the IPC
> channel in chromium. What level of detail do you need for these use cases ?

Sorry, I was asking about a description of the sorts of exploits you're
interested in blocking, and the designs you think will help you do that, and
why.

>  
> >
>
https://codereview.chromium.org/23021015/diff/138001/include/core/SkFlattenab...
> > File include/core/SkFlattenable.h (right):
> > 
> >
>
https://codereview.chromium.org/23021015/diff/138001/include/core/SkFlattenab...
> > include/core/SkFlattenable.h:135: enum Type {
> > *if* we have to have an enumeration of known subclasses, lets store it
outside
> > of this header. In fact, can we structure the code so that Skia itself
doesn't
> > need to know this list at all, but it can be something the client specifies
> > (both for writing, and for verifying during reading)?
> 
> I can put it in a different header, but asking a client to know every class in
> skia in not really reliable when we add new classes in skia and makes secure
> IsA() checks harder to implement. I'm not sure that would be viable in the
> longer term.
> 
> > https://codereview.chromium.org/23021015/diff/138001/include/core/SkRect.h
> > File include/core/SkRect.h (right):
> > 
> >
>
https://codereview.chromium.org/23021015/diff/138001/include/core/SkRect.h#ne...
> > include/core/SkRect.h:103: bool isInverted() const { return fLeft > fRight
||
> > fTop > fBottom; }
> > Starting to get odd to have isEmpty and isInverted. Does inverted need to be
> in
> > the public header?
> 
> I can move it to SkValidationUtils.h if you want.
> 
> > If we have a rect which is { 1, 1, 1, 0 }, is it empty or inverted? At the
> > moment, that depends on which predicate you call.
> 
> Does it really matter that it can be both ? An inverted rectangle is always
> empty by our definition of empty since empty includes inverted rectangles.
> 
> > If this is *only* used by validation, perhaps it should live in some
> > validation-specific (private?) header.
> > 
> >
https://codereview.chromium.org/23021015/diff/138001/include/core/SkXfermode.h
> > File include/core/SkXfermode.h (right):
> > 
> >
>
https://codereview.chromium.org/23021015/diff/138001/include/core/SkXfermode....
> > include/core/SkXfermode.h:263:
> > SK_DECLARE_FLATTENABLE_BASE_SERIALIZABLE_TYPE(SkProcXfermode)
> > What does base_serializable_type mean? My first guess was that this would
mean
> > "what base class am I derived from", which would be xfermode in this case.
> 
> These macros are all defined in SkFlattenable.h. Specifically,
> SK_DECLARE_FLATTENABLE_BASE_SERIALIZABLE_TYPE means that it's a BASE class to
> other classes unavailable in header files (most likely because their
> implementation is hidden in a cpp file), that it's SERIALIZABLE through the
> regular (non-secure) serialization, and that it has a TYPE, which means that
we
> can make type checks on it (and use its type for the secure serialization). I
> use keywords to avoid 200 character long macro names. Each one is explained
with
> a comment in SkFlattenable.h.

[sugoi1, 2013-10-03 15:53:05]

On 2013/10/03 15:25:29, reed1 wrote:
> Right. Why isn't that enough? I can see some value in specifying the expected
> "baseclass" of the effect at Factory time (e.g. I expect a subclass of
> SkShader), but I don't see the need to specify an enum for the particular
> subclass of Shader. The former "baseclass" info is core to Skia, and need not
be
> kept in-sync with any particular client's subclasses, so it feels like a
> reasonable addition to the core.

Currently, we identify an object either by its factory function pointer, which I
can't do in this case for obvious reasons, or with a partial dictionary, which
doesn't allow me to do type checking, since it's a partial dictionary. Also, I'm
pretty sure someone could hack a dictionary to produce undesired behavior.

> Sorry, I was asking about a description of the sorts of exploits you're
> interested in blocking, and the designs you think will help you do that, and
> why.

Well, I'm not a security expert and I mostly rely on what I've been told, but :
1 ) It's pretty obvious that functions pointers are forbidden, so no factory
function pointers are to be encoded in the stream.
2 ) Some skia objects, like SkProcXfermode, contain a function pointer. Again, I
can't let someone deserialize an object and cast it into another object, because
someone could potentially find a way write a different object, then cast it as a
SkProcXfermode, for example, which could be done to override SkProcXfermode's
function pointer and cause a security breach. That's why I want the type
checking to be rock solid and to not allow the deserialization of the wrong type
of object.
3 ) Security is also fairly paranoid about integer overflow. There's quite a bit
of documentation of this regarding security :
http://en.wikipedia.org/wiki/Integer_overflow
4 ) The rest of it has just been to find current numerical instabilities/issues
inside skia (mostly with the help of the fuzzer) and fix them when possible,
which has similar implications to the integer overflow mentioned above.

[reed1, 2013-10-03 15:59:11]

On 2013/10/03 15:53:05, sugoi1 wrote:
> On 2013/10/03 15:25:29, reed1 wrote:
> > Right. Why isn't that enough? I can see some value in specifying the
expected
> > "baseclass" of the effect at Factory time (e.g. I expect a subclass of
> > SkShader), but I don't see the need to specify an enum for the particular
> > subclass of Shader. The former "baseclass" info is core to Skia, and need
not
> be
> > kept in-sync with any particular client's subclasses, so it feels like a
> > reasonable addition to the core.
> 
> Currently, we identify an object either by its factory function pointer, which
I
> can't do in this case for obvious reasons, or with a partial dictionary, which
> doesn't allow me to do type checking, since it's a partial dictionary. Also,
I'm
> pretty sure someone could hack a dictionary to produce undesired behavior.

Hmmm, not obvious to me that the (partial?) dictionary isn't sufficient. Perhaps
we can discuss that in the context of your design-doc.

> 
> > Sorry, I was asking about a description of the sorts of exploits you're
> > interested in blocking, and the designs you think will help you do that, and
> > why.
> 
> Well, I'm not a security expert and I mostly rely on what I've been told, but
:
> 1 ) It's pretty obvious that functions pointers are forbidden, so no factory
> function pointers are to be encoded in the stream.
> 2 ) Some skia objects, like SkProcXfermode, contain a function pointer. Again,
I
> can't let someone deserialize an object and cast it into another object,
because
> someone could potentially find a way write a different object, then cast it as
a
> SkProcXfermode, for example, which could be done to override SkProcXfermode's
> function pointer and cause a security breach. That's why I want the type
> checking to be rock solid and to not allow the deserialization of the wrong
type
> of object.
> 3 ) Security is also fairly paranoid about integer overflow. There's quite a
bit
> of documentation of this regarding security :
> http://en.wikipedia.org/wiki/Integer_overflow
> 4 ) The rest of it has just been to find current numerical
instabilities/issues
> inside skia (mostly with the help of the fuzzer) and fix them when possible,
> which has similar implications to the integer overflow mentioned above.

The above 1-4 look like a good start. Lets paste that into a doc and iterate on
it. I have some design questions that might be better addressed in that context,
than in a CL.

[sugoi1, 2013-10-03 18:49:06]

On 2013/10/03 15:59:11, reed1 wrote:
> On 2013/10/03 15:53:05, sugoi1 wrote:
> > On 2013/10/03 15:25:29, reed1 wrote:
> > > Right. Why isn't that enough? I can see some value in specifying the
> expected
> > > "baseclass" of the effect at Factory time (e.g. I expect a subclass of
> > > SkShader), but I don't see the need to specify an enum for the particular
> > > subclass of Shader. The former "baseclass" info is core to Skia, and need
> not
> > be
> > > kept in-sync with any particular client's subclasses, so it feels like a
> > > reasonable addition to the core.
> > 
> > Currently, we identify an object either by its factory function pointer,
which
> I
> > can't do in this case for obvious reasons, or with a partial dictionary,
which
> > doesn't allow me to do type checking, since it's a partial dictionary. Also,
> I'm
> > pretty sure someone could hack a dictionary to produce undesired behavior.
> 
> Hmmm, not obvious to me that the (partial?) dictionary isn't sufficient.
Perhaps
> we can discuss that in the context of your design-doc.

Added to the document, but I thought I'd post it here too :

***

One of the important things to mention. The new SkValidatingReadBuffer class can
work as is (standalone), whereas the SkOrderedReadBuffer class requires a bit of
setup before it can work. Any time we want to use it, we need this :

1 ) The registration process must have been done (so, a call to
SkFlattenable::InitializeFlattenables(), which means a call to
SkGraphics::Init()). Right now, SkFlattenable::InitializeFlattenables()'s
implementation in SkGlobalInitialization_chromium.cpp would need to be modified
to match the one in SkGlobalInitialization_default.cpp for this to work in
Chrome and SkGraphics::Init() would need to be called in all processes.

2 ) Then, all the logic needed to initialize SkOrderedReadBuffer is inside
SkGPipeReader. Basically, all flattened SkFlattenable classes' names are written
to the stream, and then read back into a table. The reading operation will read
that table and decode the entire stream with it (using the NameToFactory()
function that requires step 1). Right now, you can easily hack the stream by
substituting a name by another one and all objects of a given class would be
deserialized by another class' factory function, and an object of the "wrong"
class would be cast into and object of the "right" class.

Nothing in the current serialization process provides type checking
functionality and every way the stream is generated seems to be easily hackable.
I'm open to suggestions, but I don't see how to protect the stream from being
easily compromised without type checking.

If we wanted the secure serialization to be somewhat closer to the current one,
we could make sure SkGraphics::Init() in called in every process in chromium
(which I personally find somewhat flaky and error prone already) and then send
the array, as we do now and use NameToFactory(), as we do now. What we'd get is
a longer stream, less efficient deserialization code and we'd still need all
that SkFlattenable::Type logic for type checking, since nothing here provides
the desired IsA() functionality.

***

So far, there's been hesitation regarding this new SkFlattenable::Type enum (or
whatever it's called). I would really like to know what alternate solution could
provide the same functionality as the SkFlattenable::Type enum without using an
enum (or something equivalent), so that I can attempt an implementation of it,
because there were different problems to all other ideas I've had so far and I'm
really curious to know what the enum-less solution to this issue is, if there is
one. Please feel free to brainstorm in the design document.

[Stephen White, 2013-10-04 14:38:07]

On 2013/10/03 18:49:06, sugoi1 wrote:
> On 2013/10/03 15:59:11, reed1 wrote:
> > On 2013/10/03 15:53:05, sugoi1 wrote:
> > > On 2013/10/03 15:25:29, reed1 wrote:
> > > > Right. Why isn't that enough? I can see some value in specifying the
> > expected
> > > > "baseclass" of the effect at Factory time (e.g. I expect a subclass of
> > > > SkShader), but I don't see the need to specify an enum for the
particular
> > > > subclass of Shader. The former "baseclass" info is core to Skia, and
need
> > not
> > > be
> > > > kept in-sync with any particular client's subclasses, so it feels like a
> > > > reasonable addition to the core.
> > > 
> > > Currently, we identify an object either by its factory function pointer,
> which
> > I
> > > can't do in this case for obvious reasons, or with a partial dictionary,
> which
> > > doesn't allow me to do type checking, since it's a partial dictionary.
Also,
> > I'm
> > > pretty sure someone could hack a dictionary to produce undesired behavior.
> > 
> > Hmmm, not obvious to me that the (partial?) dictionary isn't sufficient.
> Perhaps
> > we can discuss that in the context of your design-doc.
> 
> Added to the document, but I thought I'd post it here too :
> 
> ***
> 
> One of the important things to mention. The new SkValidatingReadBuffer class
can
> work as is (standalone), whereas the SkOrderedReadBuffer class requires a bit
of
> setup before it can work. Any time we want to use it, we need this :
> 
> 1 ) The registration process must have been done (so, a call to
> SkFlattenable::InitializeFlattenables(), which means a call to
> SkGraphics::Init()). Right now, SkFlattenable::InitializeFlattenables()'s
> implementation in SkGlobalInitialization_chromium.cpp would need to be
modified
> to match the one in SkGlobalInitialization_default.cpp for this to work in
> Chrome and SkGraphics::Init() would need to be called in all processes.
> 
> 2 ) Then, all the logic needed to initialize SkOrderedReadBuffer is inside
> SkGPipeReader. Basically, all flattened SkFlattenable classes' names are
written
> to the stream, and then read back into a table. The reading operation will
read
> that table and decode the entire stream with it (using the NameToFactory()
> function that requires step 1). Right now, you can easily hack the stream by
> substituting a name by another one and all objects of a given class would be
> deserialized by another class' factory function, and an object of the "wrong"
> class would be cast into and object of the "right" class.
> 
> Nothing in the current serialization process provides type checking
> functionality and every way the stream is generated seems to be easily
hackable.
> I'm open to suggestions, but I don't see how to protect the stream from being
> easily compromised without type checking.
> 
> If we wanted the secure serialization to be somewhat closer to the current
one,
> we could make sure SkGraphics::Init() in called in every process in chromium
> (which I personally find somewhat flaky and error prone already) and then send
> the array, as we do now and use NameToFactory(), as we do now. What we'd get
is
> a longer stream, less efficient deserialization code and we'd still need all
> that SkFlattenable::Type logic for type checking, since nothing here provides
> the desired IsA() functionality.
> 
> ***
> 
> So far, there's been hesitation regarding this new SkFlattenable::Type enum
(or
> whatever it's called). I would really like to know what alternate solution
could
> provide the same functionality as the SkFlattenable::Type enum without using
an
> enum (or something equivalent), so that I can attempt an implementation of it,
> because there were different problems to all other ideas I've had so far and
I'm
> really curious to know what the enum-less solution to this issue is, if there
is
> one. Please feel free to brainstorm in the design document.

Here's an alternative that I discussed with Alexis yesterday that would avoid a
hardcoded enum list:  monotonically increment an int on startup, and assign it
to a static int member on each class. Then use that for isA(). Would still
require SkGraphics::Init(), though.

[sugoi1, 2013-10-07 15:54:07]

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:44: return (strcmp(flattenable::GetName(), name) ==
0) || INHERITED::IsA(name); \
These type checks are now done using string comparisons.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkFlattenable.cpp
File src/core/SkFlattenable.cpp (right):

https://codereview.chromium.org/23021015/diff/150001/src/core/SkFlattenable.c...
src/core/SkFlattenable.cpp:111: InitializeFlattenables();
Initialization is now "on demand", so any usage of these functionalities will
trigger the initialization the 1st time it's called. It will no longer rely on
SkGraphics::Init being called.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkOrderedWriteB...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/150001/src/core/SkOrderedWriteB...
src/core/SkOrderedWriteBuffer.cpp:275: SkASSERT(NULL == flattenable); // We
shouldn't get in here in this scenario
I made a small change here, since NULL flattenable is a valid state here.

[mtklein, 2013-10-07 19:29:54]

Do you have someone from Chrome security you can loop in to vet this approach
and to look at SkValidatingReadBuffer closely?  It's either still rather easy to
break or I'm very confused... either way, I'd like to get more eyes on it.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/23021015/diff/150001/include/core/SkBitmap.h#...
include/core/SkBitmap.h:55: kLastConfig = kARGB_8888_Config,
Can you use kConfigCount below instead?

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:34: #define
SK_DEFINE_FLATTENABLE_SERIALIZABLE(flattenable) \
Maybe SK_DEFINE_FLATTENABLE_DESERIALIZATION_PROCS?  _SERIALIZABLE is a little
vague, particularly given that these are method for deserialization.  +1 to
DECLARE -> DEFINE.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:54: #define SK_DEFINE_FLATTENABLE_TYPE(flattenable)
\
Is there any case where we need to call SK_DEFINE_FLATTENABLE_TYPE_CHECKING
where we can't just call SK_DEFINE_FLATTENABLE_TYPE?  Why is GetName() separate?
 Can we merge these together into SK_DEFINE_FLATTENABLE_TYPE_CHECKING_PROCS?

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:62: #define
SK_DEFINE_FLATTENABLE_SERIALIZABLE_TYPE(flattenable) \
I think I may have been unclear before.  It looks like you're building up enough
levels of macros here so that each implementation class needs to call just one.
I appreciate the abstraction, but I'd prefer it the other way around.

I'd rather define just the very lowest-level macros here and have each class
call one, two, three, four, however many macros are needed to instantiate the
features that the class needs, ideally with names that make each feature
obvious.

E.g, unfold
  SK_DEFINE_FLATTENABLE_SERIALIZABLE_TYPE(foo)
into
  SK_DEFINE_FLATTENABLE_DESERIALIZATION_PROCS(foo)
  SK_DEFINE_FLATTENABLE_TYPE_CHECKING_PROCS(foo)

Ideally I'd like to be able to read one of the flattenable's headers and
understand why we've called each of the macros we call, without too much
cross-referencing into this file.  As it is right now I'm worried we're just
going to have our brains shut down as soon as we stumble across one of the many
similarly named SK_DEFINE_FLATTENABLE_SERIALZIABLE_TYPE_PROCS_CHECKING_GROUP
macros.  SK_DEFINE_FLATTENABLE_TYPE AND SK_DEFINE_FLATTENABLE_SERIALIZABLE_TYPE
in particular sound "the same" to me.

If our existing names aren't helpful, I'm strongly in favor of doing what you've
done with SK_DECLARE_PUBLIC_FLATETNABLE_DESERIALIZATION_PROCS, changing them for
the better with backwards compatibility.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:107: the name associated with that class, or
SkFlattenable::kNoneType if
kNoneType?  comment generally needs an update?

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:112: the "_TYPE" suffix, so most classes will have
these declared by default.
I get that this seems convenient, but the implicit definition of the type
checking code is going to be confusing.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:116: static const char* GetName() { return
"SkFlattenable"; }
GetTypeName?

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:118: /** Returns the class id of an object
class id of an object -> name of the object's class?

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:120: virtual const char* getFlattenableName() = 0;
getTypeName()?

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:122: /** Returns whether the current object is of
the given type
There's no current object here?  How about

/** Returns whether this class is itself or inherits from a class with this type
name.
 *  (SkFlattenable is the root of this type checking system.)
 */

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:129: * For example, if typeA is kSkImageFilter and
typeB is kSkFlattenable, this would return true
"SkImageFilter" and "SkFlattenable"?

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

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:79: bypassed by providing the type
SkFlattenable::kSkFlattenable_Type here, since
kSkFlattenableType_Type -> GetName()?

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:125: bool fError;
This seems a little odd to add this state here if it's only used by the
validating read buffer.  

Can we do this with:

virtual bool error() const { return false; }
virtual void validate(bool isValid) {}

and have the validating buffer override these?  Actually, maybe we don't even
need error()?

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:180: *  is symmetric with the read operation
using the equivalent flag
I don't think I understand what this comment is trying to say.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkFlattenable.cpp
File src/core/SkFlattenable.cpp (right):

https://codereview.chromium.org/23021015/diff/150001/src/core/SkFlattenable.c...
src/core/SkFlattenable.cpp:76: };
Why not merge this together?

struct Entry {
  const char*              fName;
  SkFlattenable::Factory   fFactory;
  SkFlattenable::TypeCheck fTypeCheck;
};
?

https://codereview.chromium.org/23021015/diff/150001/src/core/SkFlattenable.c...
src/core/SkFlattenable.cpp:125: InitializeFlattenables();
Shall we take the opportunity to rename it InitializeFlattenablesIfNeeded() or
something like that?  It's not clear here at the use sites that
InitializeFlattenables() is a one-time cost.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkFlattenableBu...
File src/core/SkFlattenableBuffers.cpp (right):

https://codereview.chromium.org/23021015/diff/150001/src/core/SkFlattenableBu...
src/core/SkFlattenableBuffers.cpp:13: fError(false) {
This is what makes me think fError doesn't really want to be on
SkFlattenableReadBuffer.  It doesn't use it.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkOrderedWriteB...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/150001/src/core/SkOrderedWriteB...
src/core/SkOrderedWriteBuffer.cpp:275: SkASSERT(NULL == flattenable); // We
shouldn't get in here in this scenario
On 2013/10/07 15:54:08, sugoi1 wrote:
> I made a small change here, since NULL flattenable is a valid state here.

Huh.  Have you seen a NULL flattenable come through here?  It'd surprise me if
we were going through here with a NULL intentionally.

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

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:14:
SkValidatingReadBuffer::SkValidatingReadBuffer() : INHERITED() {
Are all these constructors in use?  Let's cut out any code we can get away with
not having in here.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:14:
SkValidatingReadBuffer::SkValidatingReadBuffer() : INHERITED() {
I think you can get away without calling INHERITED().  At least be consistent
across the constructors.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:22: fMemoryPtr = NULL;
This is, um, confusing?  We're going to need names here that make it clearer
that setMemory and fMemoryPtr have nothing to do with each other here.  Maybe
fMemoryPtr-> fOwnedMemory?

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:24:
setFlags(SkFlattenableReadBuffer::kValidation_Flag);
this->, and in a few other places

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:27:
SkValidatingReadBuffer::SkValidatingReadBuffer(SkStream* stream) {
We've got to be careful here.  Not every SkStream has a sensible getLength()
method.

We'll need to check hasLength() first, or use SkStreamAsset* which is a subclass
where hasLength() is always true and getLength() works how you'd think.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:37: sk_free(fMemoryPtr);
Would prefer it if you made fMemoryPtr an SkAutoFree or similar instead of an
explicit destructor.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:41: fError |= (!IsPtrAlign4(data) ||
(SkAlign4(size) != size));
This is a nit, but it'd be nice to consistently parenthesize these types of
statements.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:41: fError |= (!IsPtrAlign4(data) ||
(SkAlign4(size) != size));
Yeah, I think I would also prefer to work on booleans using the doubled
operators, |= -> ||=.  |= makes me think bits.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:48: size_t inc = SkAlign4(size);
const size_t inc, and below

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:59: // value or skip nothing, respectively.
And I guess ReadScalar?  Add a note about how they'll set fError which should be
checked by callers?

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:102: }
Shouldn't we be checking that there is indeed an \0 in here?  Or do we not care
because we're only copying len characters no matter what?

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:108: fError = true;
for consistency, fError ||= (encodingType == encoding); ?

I don't quite get why this is an error.

Don't we want to bail out as early as possible if we fail?  Why continue to read
the length?

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:126: size_t size =
matrix->readFromMemory(fReader.peek());
Don't we need to skip this memory first to make sure there's enough to read a
matrix?  Same for region, path, etc?

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:134: memcpy(rect,
this->skip(sizeof(SkIRect)), sizeof(SkIRect));
I'm not sure I understand the failure strategy of skip.

If skip fails, we'll just memcpy an SkIRect from where we started.  Isn't that
the same as the normal unsecure path?  Don't we need to check the result of skip
before trying a memcpy?  Same for rect, byte array, color array, etc?

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:200: if (length != 0) {
fError ||= (length != 0); ?

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
File src/core/SkValidatingReadBuffer.h (right):

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.h:20: #if defined(SK_DEBUG) &&
defined(SK_BUILD_FOR_MAC)
Is this left over from an old version?  Any idea what it's doing here?

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.h:67: // TODO: Implement this (with securely)
when needed
with securely?

https://codereview.chromium.org/23021015/diff/150001/src/ports/SkGlobalInitia...
File src/ports/SkGlobalInitialization_chromium.cpp (right):

https://codereview.chromium.org/23021015/diff/150001/src/ports/SkGlobalInitia...
src/ports/SkGlobalInitialization_chromium.cpp:8: #include "SkTypes.h"
Is this file now the same as SkGlobalInitialization_default?

[sugoi1, 2013-10-08 20:23:09]

I've tried to upload it 3 times, but I keep getting this error :

The following errors occured:
Base files missing.
Try to upload the changeset again.

Hopefully, you can still review it...

Anyway, about security, I'll gladly show it to security people once I've done
all that I can to make it more secure, since I already know that more things are
coming down the pipeline. Once the Skia people agree with this and everything
works properly and all the bots are happy, I'll get someone from security to
help out with the paranoia.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.chromium.org/23021015/diff/150001/include/core/SkBitmap.h#...
include/core/SkBitmap.h:55: kLastConfig = kARGB_8888_Config,
On 2013/10/07 19:29:56, mtklein wrote:
> Can you use kConfigCount below instead?

Done.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:34: #define
SK_DEFINE_FLATTENABLE_SERIALIZABLE(flattenable) \
On 2013/10/07 19:29:56, mtklein wrote:
> Maybe SK_DEFINE_FLATTENABLE_DESERIALIZATION_PROCS?  _SERIALIZABLE is a little
> vague, particularly given that these are method for deserialization.  +1 to
> DECLARE -> DEFINE.

Done.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:54: #define SK_DEFINE_FLATTENABLE_TYPE(flattenable)
\
On 2013/10/07 19:29:56, mtklein wrote:
> Is there any case where we need to call SK_DEFINE_FLATTENABLE_TYPE_CHECKING
> where we can't just call SK_DEFINE_FLATTENABLE_TYPE?  Why is GetName()
separate?
>  Can we merge these together into SK_DEFINE_FLATTENABLE_TYPE_CHECKING_PROCS?

Yeah, I had already done this locally.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:62: #define
SK_DEFINE_FLATTENABLE_SERIALIZABLE_TYPE(flattenable) \
On 2013/10/07 19:29:56, mtklein wrote:
> I think I may have been unclear before.  It looks like you're building up
enough
> levels of macros here so that each implementation class needs to call just
one.
> I appreciate the abstraction, but I'd prefer it the other way around.
> 
> I'd rather define just the very lowest-level macros here and have each class
> call one, two, three, four, however many macros are needed to instantiate the
> features that the class needs, ideally with names that make each feature
> obvious.
> 
> E.g, unfold
>   SK_DEFINE_FLATTENABLE_SERIALIZABLE_TYPE(foo)
> into
>   SK_DEFINE_FLATTENABLE_DESERIALIZATION_PROCS(foo)
>   SK_DEFINE_FLATTENABLE_TYPE_CHECKING_PROCS(foo)
> 
> Ideally I'd like to be able to read one of the flattenable's headers and
> understand why we've called each of the macros we call, without too much
> cross-referencing into this file.  As it is right now I'm worried we're just
> going to have our brains shut down as soon as we stumble across one of the
many
> similarly named SK_DEFINE_FLATTENABLE_SERIALZIABLE_TYPE_PROCS_CHECKING_GROUP
> macros.  SK_DEFINE_FLATTENABLE_TYPE AND
SK_DEFINE_FLATTENABLE_SERIALIZABLE_TYPE
> in particular sound "the same" to me.
> 
> If our existing names aren't helpful, I'm strongly in favor of doing what
you've
> done with SK_DECLARE_PUBLIC_FLATETNABLE_DESERIALIZATION_PROCS, changing them
for
> the better with backwards compatibility.

Done.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:107: the name associated with that class, or
SkFlattenable::kNoneType if
On 2013/10/07 19:29:56, mtklein wrote:
> kNoneType?  comment generally needs an update?

Done.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:112: the "_TYPE" suffix, so most classes will have
these declared by default.
On 2013/10/07 19:29:56, mtklein wrote:
> I get that this seems convenient, but the implicit definition of the type
> checking code is going to be confusing.

Done. I did leave it in SK_DEFINE_FLATTENABLE_DESERIALIZATION_PROCS(), since
type checking will be mandatory to deserialize.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:116: static const char* GetName() { return
"SkFlattenable"; }
On 2013/10/07 19:29:56, mtklein wrote:
> GetTypeName?

Done.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:118: /** Returns the class id of an object
On 2013/10/07 19:29:56, mtklein wrote:
> class id of an object -> name of the object's class?

Done.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:120: virtual const char* getFlattenableName() = 0;
On 2013/10/07 19:29:56, mtklein wrote:
> getTypeName()?

Done.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:122: /** Returns whether the current object is of
the given type
On 2013/10/07 19:29:56, mtklein wrote:
> There's no current object here?  How about
> 
> /** Returns whether this class is itself or inherits from a class with this
type
> name.
>  *  (SkFlattenable is the root of this type checking system.)
>  */

Done.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenable.h:129: * For example, if typeA is kSkImageFilter and
typeB is kSkFlattenable, this would return true
On 2013/10/07 19:29:56, mtklein wrote:
> "SkImageFilter" and "SkFlattenable"?

Done.

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

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:79: bypassed by providing the type
SkFlattenable::kSkFlattenable_Type here, since
On 2013/10/07 19:29:56, mtklein wrote:
> kSkFlattenableType_Type -> GetName()?

Done.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:125: bool fError;
On 2013/10/07 19:29:56, mtklein wrote:
> This seems a little odd to add this state here if it's only used by the
> validating read buffer.  
> 
> Can we do this with:
> 
> virtual bool error() const { return false; }
> virtual void validate(bool isValid) {}
> 
> and have the validating buffer override these?  Actually, maybe we don't even
> need error()?

Done.

https://codereview.chromium.org/23021015/diff/150001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:180: *  is symmetric with the read operation
using the equivalent flag
On 2013/10/07 19:29:56, mtklein wrote:
> I don't think I understand what this comment is trying to say.

We can phrase it any other way, but essentially : The reading operation is the
one that needs to be secure, while the writing operation can send any stream it
wants (since the stream can be hacked after it is written, we can't really make
any security assumptions on this side). In this case, since the secure version
of the reading operation has its own way of reading the data, then the writing
operation must write the data in a way that's expected by the reading operation.
Ping me on the chat if this isn't clear.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkFlattenable.cpp
File src/core/SkFlattenable.cpp (right):

https://codereview.chromium.org/23021015/diff/150001/src/core/SkFlattenable.c...
src/core/SkFlattenable.cpp:76: };
On 2013/10/07 19:29:56, mtklein wrote:
> Why not merge this together?
> 
> struct Entry {
>   const char*              fName;
>   SkFlattenable::Factory   fFactory;
>   SkFlattenable::TypeCheck fTypeCheck;
> };
> ?

Done.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkFlattenable.c...
src/core/SkFlattenable.cpp:125: InitializeFlattenables();
On 2013/10/07 19:29:56, mtklein wrote:
> Shall we take the opportunity to rename it InitializeFlattenablesIfNeeded() or
> something like that?  It's not clear here at the use sites that
> InitializeFlattenables() is a one-time cost.

Done.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkOrderedWriteB...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/150001/src/core/SkOrderedWriteB...
src/core/SkOrderedWriteBuffer.cpp:275: SkASSERT(NULL == flattenable); // We
shouldn't get in here in this scenario
On 2013/10/07 19:29:56, mtklein wrote:
> On 2013/10/07 15:54:08, sugoi1 wrote:
> > I made a small change here, since NULL flattenable is a valid state here.
> 
> Huh.  Have you seen a NULL flattenable come through here?  It'd surprise me if
we were going through here with a NULL intentionally.

Yes, in SkXfermodeImageFilter, fMode can be NULL, and we unconditionally call
buffer.writeFlattenable(fMode) inside SkXfermodeImageFilter::flatten() (found by
the fuzzer).

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

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:14:
SkValidatingReadBuffer::SkValidatingReadBuffer() : INHERITED() {
On 2013/10/07 19:29:56, mtklein wrote:
> Are all these constructors in use?  Let's cut out any code we can get away
with
> not having in here.

Done.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:14:
SkValidatingReadBuffer::SkValidatingReadBuffer() : INHERITED() {
On 2013/10/07 19:29:56, mtklein wrote:
> I think you can get away without calling INHERITED().  At least be consistent
> across the constructors.

Done.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:22: fMemoryPtr = NULL;
On 2013/10/07 19:29:56, mtklein wrote:
> This is, um, confusing?  We're going to need names here that make it clearer
> that setMemory and fMemoryPtr have nothing to do with each other here.  Maybe
> fMemoryPtr-> fOwnedMemory?

I removed fMemoryPtr for now.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:24:
setFlags(SkFlattenableReadBuffer::kValidation_Flag);
On 2013/10/07 19:29:56, mtklein wrote:
> this->, and in a few other places

Done.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:27:
SkValidatingReadBuffer::SkValidatingReadBuffer(SkStream* stream) {
On 2013/10/07 19:29:56, mtklein wrote:
> We've got to be careful here.  Not every SkStream has a sensible getLength()
> method.
> 
> We'll need to check hasLength() first, or use SkStreamAsset* which is a
subclass
> where hasLength() is always true and getLength() works how you'd think.

I removed the stream constructor for now, as it is unused. Only the data / size
constructor will be kept.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:37: sk_free(fMemoryPtr);
On 2013/10/07 19:29:56, mtklein wrote:
> Would prefer it if you made fMemoryPtr an SkAutoFree or similar instead of an
> explicit destructor.

fMemoryPtr is gone

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:41: fError |= (!IsPtrAlign4(data) ||
(SkAlign4(size) != size));
On 2013/10/07 19:29:56, mtklein wrote:
> Yeah, I think I would also prefer to work on booleans using the doubled
> operators, |= -> ||=.  |= makes me think bits.

Done.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:48: size_t inc = SkAlign4(size);
On 2013/10/07 19:29:56, mtklein wrote:
> const size_t inc, and below

Done.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:59: // value or skip nothing, respectively.
On 2013/10/07 19:29:56, mtklein wrote:
> And I guess ReadScalar?  Add a note about how they'll set fError which should
be
> checked by callers?

Done.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:102: }
On 2013/10/07 19:29:56, mtklein wrote:
> Shouldn't we be checking that there is indeed an \0 in here?  Or do we not
care
> because we're only copying len characters no matter what?

I added a check. Doesn't hurt to be extra careful.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:108: fError = true;
On 2013/10/07 19:29:56, mtklein wrote:
> for consistency, fError ||= (encodingType == encoding); ?
> 
> I don't quite get why this is an error.
> 
> Don't we want to bail out as early as possible if we fail?  Why continue to
read
> the length?

That was a mistake. Fixed it by writing :
fError = fError || (encodingType != encoding);

If we want to bail as early as we fail, I'd have to write something like :

    const int32_t encodingType = fReader.readInt();
    fError = fError || (encodingType != encoding);
    if (fError) {
        return NULL;
    }
    *length = this->readInt();
    if (fError) {
        return NULL;
    }
    const void* ptr = this->skip(SkAlign4(*length));
    if (fError) {
        return NULL;
    }

Seems overkill, since each of these functions check fError internally.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:126: size_t size =
matrix->readFromMemory(fReader.peek());
On 2013/10/07 19:29:56, mtklein wrote:
> Don't we need to skip this memory first to make sure there's enough to read a
> matrix?  Same for region, path, etc?

Chicken and egg problem. Right now, we don't know ahead of time how many bytes
we'll be reading (ok, in the matrix's case, it's always 9 * sizeof(SkScalar), so
I could fix that one), but we'd need to add functions to precompute the read
size for SkRegion and SkPath, so should I do that ?

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:134: memcpy(rect,
this->skip(sizeof(SkIRect)), sizeof(SkIRect));
On 2013/10/07 19:29:56, mtklein wrote:
> I'm not sure I understand the failure strategy of skip.
> 
> If skip fails, we'll just memcpy an SkIRect from where we started.  Isn't that
> the same as the normal unsecure path?  Don't we need to check the result of
skip
> before trying a memcpy?  Same for rect, byte array, color array, etc?

Right, fixed.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.cpp:200: if (length != 0) {
On 2013/10/07 19:29:56, mtklein wrote:
> fError ||= (length != 0); ?

Done.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
File src/core/SkValidatingReadBuffer.h (right):

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.h:20: #if defined(SK_DEBUG) &&
defined(SK_BUILD_FOR_MAC)
On 2013/10/07 19:29:56, mtklein wrote:
> Is this left over from an old version?  Any idea what it's doing here?

Yeah, that's from SkOrderedReadBuffer.h. I removed it.

https://codereview.chromium.org/23021015/diff/150001/src/core/SkValidatingRea...
src/core/SkValidatingReadBuffer.h:67: // TODO: Implement this (with securely)
when needed
On 2013/10/07 19:29:56, mtklein wrote:
> with securely?

That's what happens when I'm thinking of 2 sentences at the same time :) I'll
remove the "with".

https://codereview.chromium.org/23021015/diff/150001/src/ports/SkGlobalInitia...
File src/ports/SkGlobalInitialization_chromium.cpp (right):

https://codereview.chromium.org/23021015/diff/150001/src/ports/SkGlobalInitia...
src/ports/SkGlobalInitialization_chromium.cpp:8: #include "SkTypes.h"
On 2013/10/07 19:29:56, mtklein wrote:
> Is this file now the same as SkGlobalInitialization_default?

I'll have to double check. I had to merge some things in the chromium one, but
maybe they cancelled out.

[sugoi1, 2013-10-09 14:29:56]

I found a few minor things to fix in last night's cl. I also retried uploading
the code, but I still get the error (it seems to be ok in the code review page,
though).

https://codereview.chromium.org/23021015/diff/169001/include/effects/SkArithm...
File include/effects/SkArithmeticMode.h (right):

https://codereview.chromium.org/23021015/diff/169001/include/effects/SkArithm...
include/effects/SkArithmeticMode.h:31: SK_DECLARE_FLATTENABLE_REGISTRAR_GROUP()
I added the missing semicolon to remove this line difference.

https://codereview.chromium.org/23021015/diff/169001/samplecode/ClockFaceView...
File samplecode/ClockFaceView.cpp (right):

https://codereview.chromium.org/23021015/diff/169001/samplecode/ClockFaceView...
samplecode/ClockFaceView.cpp:74:
SK_DEFINE_FLATTENABLE_TYPE_CHECKING_PROCS(Dot2DPathEffect)
Both should be SK_DEFINE_FLATTENABLE_DESERIALIZATION_PROCS instead of
SK_DEFINE_FLATTENABLE_TYPE_CHECKING_PROCS in this file.

https://codereview.chromium.org/23021015/diff/169001/samplecode/SampleAll.cpp
File samplecode/SampleAll.cpp (right):

https://codereview.chromium.org/23021015/diff/169001/samplecode/SampleAll.cpp...
samplecode/SampleAll.cpp:167:
SK_DEFINE_FLATTENABLE_TYPE_CHECKING_PROCS(Dot2DPathEffect)
Should be SK_DEFINE_FLATTENABLE_DESERIALIZATION_PROCS

https://codereview.chromium.org/23021015/diff/169001/src/ports/SkGlobalInitia...
File src/ports/SkGlobalInitialization_chromium.cpp (right):

https://codereview.chromium.org/23021015/diff/169001/src/ports/SkGlobalInitia...
src/ports/SkGlobalInitialization_chromium.cpp:66: static bool initialized =
false;
Perfect candidate for SK_ONCE, I'll change it as soon as it's available.

[sugoi1, 2013-10-10 15:27:58]

So this is my attempt at integrating Mike's cl into this one. Here's some extra
info :

What I had to do in this cl :
- Made readFlattenable() protected
- Made readFlattenableT() private and used it in all new readFoo() functions
- Had to fix SkScalerContext.cpp, SkGPipeRead.cpp and ColorFilterTest.cpp so
that they use the new readFoo() functions instead of the generic
readFlattenable(), which is no longer available.

What I didn't do :
- I didn't remove the current type checking implementation. That can be
simplified to only include the base classes from now on, but it doesn't affect
the current outcome, which is that only the readFoo() functions can be used.
- I didn't fix the SkLight problem yet (it is deserialized as a generic
SkFlattenable), I'll need some input as to what the best solution is for this
before I implement it.

[sugoi1, 2013-10-15 18:12:24]

Integrated the effect types into this cl.

I forgot to send notes when I uploaded patch 26 last friday, so here they are

Notes for patch 26 :

- Moved SkEffectType to SkFlattenable.h, for convenience and added an Invalid
enum for when the association with the name fails
- Added type for SkAnnotation, SkColorFilter, SkColorTable, SkData and
SkDataSet, which will be required until these no longer inherit from
SkFlattenable.
- Added a utility function called SkFlattenable::getTypeName()
- Added a utility function called SkFlattenable::NameToType() (similar to
NameToFactory())
- Registrar now uses SkEffectType instead of an IsA type checking function
- All classes that are base classes, or effect types, have a new function called
GetEffectType(), which is defined by using the    
SK_DEFINE_FLATTENABLE_EFFECT_TYPE macro
- Adapted SkFlattenable::InitializeFlattenablesIfNeeded() to SK_ONCE (I didn't
see an easy way to make this works for functions without any argument, so I
added a dummy int for now)

Notes for patch 27 :

- Now that SkAnnotation, SkLight, SkData and SkDataSet are done, only
SkColorTable and SkDataTable remain in the list of temporary types that should
go away soon.
- Cleaned up and removed changes that were no longer necessary
- Both initialization files (SkGlobalInitialization_default.cpp and
SkGlobalInitialization_chromium.cpp) are now identical.

[sugoi1, 2013-10-16 15:05:31]

Here's the new version.

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
include/core/SkFlattenable.h:46: enum SkEffectType {
I need this here for NameToType(), and so that all base classes can access this
enum.

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
File include/core/SkFlattenableBuffers.h (left):

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:35: enum SkEffectType {
Moved to SkFlattenable.h

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:82: virtual SkFlattenable*
readFlattenable(SkEffectType) = 0;
I'm still going to argue that have a duplicate interface of
readFlattenable(type) + readFoo() is bad. I removed it. If you dislike my
solutions for removing readFlattenable() from the public interface, we can
discuss them, but I really think this has to go.

https://codereview.chromium.org/23021015/diff/188001/src/core/SkFlattenable.cpp
File src/core/SkFlattenable.cpp (right):

https://codereview.chromium.org/23021015/diff/188001/src/core/SkFlattenable.c...
src/core/SkFlattenable.cpp:130: return kInvalid_SkEffectType;
I added the Invalid type to have something to return here in case of failure.

[reed1, 2013-10-16 15:20:19]

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
include/core/SkFlattenable.h:47: kInvalid_SkEffectType,
Hmmm, that might be the right way. I think sometimes its hurts a little not to
have a closed-set. Perhaps NameToType could return a bool for success/failure,
and return the effectype as a var-arg...? Seems a little awkward, but it means
all of the other sites that *don't* expect kInvalid don't have to check for it
(e.g. Register)

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

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:129: virtual SkFlattenable* readFlattenable(
Why make this guy protected instead of public?

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:131: @param type   This parameter is only
used when using SkValidatingReadBuffer. It will verify
can we move the comment to above the function, instead of inside the parens?

https://codereview.chromium.org/23021015/diff/188001/include/effects/SkArithm...
File include/effects/SkArithmeticMode.h (right):

https://codereview.chromium.org/23021015/diff/188001/include/effects/SkArithm...
include/effects/SkArithmeticMode.h:16: class SK_API SkArithmeticMode {
nice catch

[sugoi1, 2013-10-16 15:36:24]

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
include/core/SkFlattenable.h:47: kInvalid_SkEffectType,
On 2013/10/16 15:20:20, reed1 wrote:
> Hmmm, that might be the right way. I think sometimes its hurts a little not to
> have a closed-set. Perhaps NameToType could return a bool for success/failure,
> and return the effectype as a var-arg...? Seems a little awkward, but it means
> all of the other sites that *don't* expect kInvalid don't have to check for it
> (e.g. Register)

Done.

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

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:129: virtual SkFlattenable* readFlattenable(
On 2013/10/16 15:20:20, reed1 wrote:
> Why make this guy protected instead of public?

So that everyone has to use the specialized readFoo() functions instead of the
generic readFlattenable() function.

https://codereview.chromium.org/23021015/diff/188001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:131: @param type   This parameter is only
used when using SkValidatingReadBuffer. It will verify
On 2013/10/16 15:20:20, reed1 wrote:
> can we move the comment to above the function, instead of inside the parens?

Done.

[mtklein, 2013-10-16 18:54:08]

I think I'm getting down to small enough things that I could be swayed to just
pull the trigger.  The only hugely important thing I really commented on here
was fMat[0] v.s fMat[i].

https://codereview.chromium.org/23021015/diff/195001/gyp/core.gypi
File gyp/core.gypi (right):

https://codereview.chromium.org/23021015/diff/195001/gyp/core.gypi#newcode170
gyp/core.gypi:170: '<(skia_src_path)/core/SkValidatingReadBuffer.cpp',
Nit: this is a strange place to put this line.  Alphabetically?

https://codereview.chromium.org/23021015/diff/195001/include/core/SkColorFilt...
File include/core/SkColorFilter.h (right):

https://codereview.chromium.org/23021015/diff/195001/include/core/SkColorFilt...
include/core/SkColorFilter.h:134: SK_DEFINE_FLATTENABLE_EFFECT_TYPE(ColorFilter)
I started a little confused that these don't quite match the class names, e.g.
SK_DEFINE_FLATTENABLE_EFFECT_TYPE(SkColorFilter).  I don't think this matters
much, but it'd probably be less jarring with the Sk?

https://codereview.chromium.org/23021015/diff/195001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/195001/include/core/SkFlattenab...
include/core/SkFlattenable.h:19: SkFlattenable::Registrar(#flattenable,
flattenable::CreateProc, flattenable::GetEffectType());
Sadly this has wrapped to 102 columns.

https://codereview.chromium.org/23021015/diff/195001/src/core/SkFlattenable.cpp
File src/core/SkFlattenable.cpp (right):

https://codereview.chromium.org/23021015/diff/195001/src/core/SkFlattenable.c...
src/core/SkFlattenable.cpp:119: bool SkFlattenable::NameToType(const char
name[], SkEffectType& type) {
Would prefer type to be a pointer to make it clear at the call site that type is
modified.

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

https://codereview.chromium.org/23021015/diff/195001/src/effects/SkColorMatri...
src/effects/SkColorMatrixFilter.cpp:314:
buffer.validate(SkScalarIsFinite(fMatrix.fMat[0]));
fMat[i]?

https://codereview.chromium.org/23021015/diff/195001/src/effects/SkMatrixConv...
File src/effects/SkMatrixConvolutionImageFilter.cpp (right):

https://codereview.chromium.org/23021015/diff/195001/src/effects/SkMatrixConv...
src/effects/SkMatrixConvolutionImageFilter.cpp:26: bool
TileModeIsValid(SkMatrixConvolutionImageFilter::TileMode tileMode) {
I just have this feeling tfarina is going to swoop in here and convert this to
  static bool tile_mode_is_valid(...) { ... }
to match the Skia style guide.  If you feel like getting ahead of the curve, you
can do that now. :)

https://codereview.chromium.org/23021015/diff/195001/src/ports/SkGlobalInitia...
File src/ports/SkGlobalInitialization_chromium.cpp (right):

https://codereview.chromium.org/23021015/diff/195001/src/ports/SkGlobalInitia...
src/ports/SkGlobalInitialization_chromium.cpp:8: #include "SkTypes.h"
I think I've asked this one before but now I forget... these two
SkGlobalInitialization are now the same right?  We'll want to clean these up at
some point.  Not now obviously.

https://codereview.chromium.org/23021015/diff/195001/src/ports/SkGlobalInitia...
File src/ports/SkGlobalInitialization_default.cpp (right):

https://codereview.chromium.org/23021015/diff/195001/src/ports/SkGlobalInitia...
src/ports/SkGlobalInitialization_default.cpp:61:
DEF_SK_ONCE(InitializeFlattenables, int*) {
Should be safe to use void* and NULL here too.  This is fine by me as-is.

[sugoi1, 2013-10-16 20:07:40]

https://codereview.chromium.org/23021015/diff/195001/gyp/core.gypi
File gyp/core.gypi (right):

https://codereview.chromium.org/23021015/diff/195001/gyp/core.gypi#newcode170
gyp/core.gypi:170: '<(skia_src_path)/core/SkValidatingReadBuffer.cpp',
On 2013/10/16 18:54:09, mtklein wrote:
> Nit: this is a strange place to put this line.  Alphabetically?

Oops. It used to be SkSecure... Fixed.

https://codereview.chromium.org/23021015/diff/195001/include/core/SkColorFilt...
File include/core/SkColorFilter.h (right):

https://codereview.chromium.org/23021015/diff/195001/include/core/SkColorFilt...
include/core/SkColorFilter.h:134: SK_DEFINE_FLATTENABLE_EFFECT_TYPE(ColorFilter)
On 2013/10/16 18:54:09, mtklein wrote:
> I started a little confused that these don't quite match the class names, e.g.
> SK_DEFINE_FLATTENABLE_EFFECT_TYPE(SkColorFilter).  I don't think this matters
> much, but it'd probably be less jarring with the Sk?

There's no Sk in the enum, but I'll add it.

https://codereview.chromium.org/23021015/diff/195001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/195001/include/core/SkFlattenab...
include/core/SkFlattenable.h:19: SkFlattenable::Registrar(#flattenable,
flattenable::CreateProc, flattenable::GetEffectType());
On 2013/10/16 18:54:09, mtklein wrote:
> Sadly this has wrapped to 102 columns.

Noooooo ! Fixed

https://codereview.chromium.org/23021015/diff/195001/src/core/SkFlattenable.cpp
File src/core/SkFlattenable.cpp (right):

https://codereview.chromium.org/23021015/diff/195001/src/core/SkFlattenable.c...
src/core/SkFlattenable.cpp:119: bool SkFlattenable::NameToType(const char
name[], SkEffectType& type) {
On 2013/10/16 18:54:09, mtklein wrote:
> Would prefer type to be a pointer to make it clear at the call site that type
is
> modified.

Done.

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

https://codereview.chromium.org/23021015/diff/195001/src/effects/SkColorMatri...
src/effects/SkColorMatrixFilter.cpp:314:
buffer.validate(SkScalarIsFinite(fMatrix.fMat[0]));
On 2013/10/16 18:54:09, mtklein wrote:
> fMat[i]?

ouf... good catch! Fixed

https://codereview.chromium.org/23021015/diff/195001/src/effects/SkMatrixConv...
File src/effects/SkMatrixConvolutionImageFilter.cpp (right):

https://codereview.chromium.org/23021015/diff/195001/src/effects/SkMatrixConv...
src/effects/SkMatrixConvolutionImageFilter.cpp:26: bool
TileModeIsValid(SkMatrixConvolutionImageFilter::TileMode tileMode) {
On 2013/10/16 18:54:09, mtklein wrote:
> I just have this feeling tfarina is going to swoop in here and convert this to
>   static bool tile_mode_is_valid(...) { ... }
> to match the Skia style guide.  If you feel like getting ahead of the curve,
you
> can do that now. :)

Done.

https://codereview.chromium.org/23021015/diff/195001/src/ports/SkGlobalInitia...
File src/ports/SkGlobalInitialization_chromium.cpp (right):

https://codereview.chromium.org/23021015/diff/195001/src/ports/SkGlobalInitia...
src/ports/SkGlobalInitialization_chromium.cpp:8: #include "SkTypes.h"
On 2013/10/16 18:54:09, mtklein wrote:
> I think I've asked this one before but now I forget... these two
> SkGlobalInitialization are now the same right?  We'll want to clean these up
at
> some point.  Not now obviously.

Right, we'll need to change all affected build files if we merge them.

https://codereview.chromium.org/23021015/diff/195001/src/ports/SkGlobalInitia...
File src/ports/SkGlobalInitialization_default.cpp (right):

https://codereview.chromium.org/23021015/diff/195001/src/ports/SkGlobalInitia...
src/ports/SkGlobalInitialization_default.cpp:61:
DEF_SK_ONCE(InitializeFlattenables, int*) {
On 2013/10/16 18:54:09, mtklein wrote:
> Should be safe to use void* and NULL here too.  This is fine by me as-is.

I think you need an actual type for the template, which NULL does not provide.
We should add a way to pass no arguments in the SK_ONCE related macros.

[mtklein, 2013-10-16 20:09:39]

https://codereview.chromium.org/23021015/diff/195001/src/ports/SkGlobalInitia...
File src/ports/SkGlobalInitialization_default.cpp (right):

https://codereview.chromium.org/23021015/diff/195001/src/ports/SkGlobalInitia...
src/ports/SkGlobalInitialization_default.cpp:61:
DEF_SK_ONCE(InitializeFlattenables, int*) {
On 2013/10/16 20:07:41, sugoi1 wrote:
> On 2013/10/16 18:54:09, mtklein wrote:
> > Should be safe to use void* and NULL here too.  This is fine by me as-is.
> 
> I think you need an actual type for the template, which NULL does not provide.
> We should add a way to pass no arguments in the SK_ONCE related macros.

I just mean, write DEF_SK_ONCE(InitializeFlattenables, void*) { ... } and
SK_ONCE(InitializeFlattenables, NULL)

[mtklein, 2013-10-16 20:18:06]

On 2013/10/16 20:09:39, mtklein wrote:
>
https://codereview.chromium.org/23021015/diff/195001/src/ports/SkGlobalInitia...
> File src/ports/SkGlobalInitialization_default.cpp (right):
> 
>
https://codereview.chromium.org/23021015/diff/195001/src/ports/SkGlobalInitia...
> src/ports/SkGlobalInitialization_default.cpp:61:
> DEF_SK_ONCE(InitializeFlattenables, int*) {
> On 2013/10/16 20:07:41, sugoi1 wrote:
> > On 2013/10/16 18:54:09, mtklein wrote:
> > > Should be safe to use void* and NULL here too.  This is fine by me as-is.
> > 
> > I think you need an actual type for the template, which NULL does not
provide.
> > We should add a way to pass no arguments in the SK_ONCE related macros.
> 
> I just mean, write DEF_SK_ONCE(InitializeFlattenables, void*) { ... } and
> SK_ONCE(InitializeFlattenables, NULL)

Nevermind!  I made it so you can't do this.  My bad.

[Stephen White, 2013-10-16 20:27:23]

https://codereview.chromium.org/23021015/diff/205001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/205001/include/core/SkFlattenab...
include/core/SkFlattenable.h:47: enum SkEffectType {
Just to bikeshed a little, is SkEffectType a good name for this? It seems to
include things which are not effects (e.g., PixelRef, UnitMapper, etc). Maybe
SkFlattenableType? (or SkBaseClass, SkFlattenableBaseClass or .. ?)

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

https://codereview.chromium.org/23021015/diff/205001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:70: // to be larger than SK_MaxS32 in this specific
case. Check inversion instead.
Actually, this should no longer be the case. The "unspecified" state has been
pulled out into the fFlags, so you should be able to use SkIsValidRect() on the
rect now.

https://codereview.chromium.org/23021015/diff/205001/src/ports/SkGlobalInitia...
File src/ports/SkGlobalInitialization_chromium.cpp (right):

https://codereview.chromium.org/23021015/diff/205001/src/ports/SkGlobalInitia...
src/ports/SkGlobalInitialization_chromium.cpp:11: #include "SkMallocPixelRef.h"
How many diffs are there between this and SkGlobalInitialization_default.cpp?
Can we refactor them at all?

[reed1, 2013-10-16 20:38:14]

https://codereview.chromium.org/23021015/diff/205001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/205001/include/core/SkFlattenab...
include/core/SkFlattenable.h:47: enum SkEffectType {
On 2013/10/16 20:27:24, Stephen White wrote:
> Just to bikeshed a little, is SkEffectType a good name for this? It seems to
> include things which are not effects (e.g., PixelRef, UnitMapper, etc). Maybe
> SkFlattenableType? (or SkBaseClass, SkFlattenableBaseClass or .. ?)

Totally agree. I started that name, but it doesn't really convey whatever it is
we think it is supposed to mean.  SkFactoryable?

[sugoi1, 2013-10-17 15:20:38]

- Changed SkEffectType -> SkFlattenable::Type. Back to the original name, it
should be simple and good enough.
- Removed extra SK_OVERRIDE causing build issues in the trybots
- Fixed the rect validation test in SkImageFilter

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

https://codereview.chromium.org/23021015/diff/205001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:70: // to be larger than SK_MaxS32 in this specific
case. Check inversion instead.
On 2013/10/16 20:27:24, Stephen White wrote:
> Actually, this should no longer be the case. The "unspecified" state has been
> pulled out into the fFlags, so you should be able to use SkIsValidRect() on
the
> rect now.

Done.

https://codereview.chromium.org/23021015/diff/205001/src/ports/SkGlobalInitia...
File src/ports/SkGlobalInitialization_chromium.cpp (right):

https://codereview.chromium.org/23021015/diff/205001/src/ports/SkGlobalInitia...
src/ports/SkGlobalInitialization_chromium.cpp:11: #include "SkMallocPixelRef.h"
On 2013/10/16 20:27:24, Stephen White wrote:
> How many diffs are there between this and SkGlobalInitialization_default.cpp?
> Can we refactor them at all?

They should be the identical (or almost identical) after this cl. I'm not going
to merge them in this cl, though. That will be done as a follow up, if that's
something we want to do.

[Stephen White, 2013-10-17 15:27:54]

https://codereview.chromium.org/23021015/diff/217001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/217001/include/core/SkFlattenab...
include/core/SkFlattenable.h:42: static Type GetType() { \
+1 for the scoped enum. I like it! But I think this function should be
GetFlattenableType(), in order to avoid polluting the subclass's namespace with
something that sounds pretty generic. GetType() could imply it was the derived
class's type, where in actuality it's the base class's.

[sugoi1, 2013-10-17 15:33:53]

https://codereview.chromium.org/23021015/diff/217001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/217001/include/core/SkFlattenab...
include/core/SkFlattenable.h:42: static Type GetType() { \
On 2013/10/17 15:27:56, Stephen White wrote:
> +1 for the scoped enum. I like it! But I think this function should be
> GetFlattenableType(), in order to avoid polluting the subclass's namespace
with
> something that sounds pretty generic. GetType() could imply it was the derived
> class's type, where in actuality it's the base class's.

Done.

[sugoi1, 2013-10-21 14:57:57]

Removed the SkGPipeRead changes in an effort to finally land this cl.

[reed1, 2013-10-21 20:54:47]

just naming nit. are there other comments?

https://codereview.chromium.org/23021015/diff/269001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/269001/include/core/SkFlattenab...
include/core/SkFlattenable.h:56: kSkColorFilter_Type,
If these are going to be inside an SkClass of some sort (as they are here), we
don't want the Sk on their name...

e.g.

kSkColorFilter_Type -> kColorFilter_Type

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

https://codereview.chromium.org/23021015/diff/269001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:137: template <typename T> T*
readFlattenableT();
don't really care, but why is this guy private?

[reed1, 2013-10-21 20:55:29]

are we in a position with this CL to write tests that exercise/trigger the error
detectors?

[mtklein, 2013-10-21 21:12:32]

lgtm

https://codereview.chromium.org/23021015/diff/269001/include/core/SkFlattenab...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/23021015/diff/269001/include/core/SkFlattenab...
include/core/SkFlattenable.h:56: kSkColorFilter_Type,
On 2013/10/21 20:54:48, reed1 wrote:
> If these are going to be inside an SkClass of some sort (as they are here), we
> don't want the Sk on their name...
> 
> e.g.
> 
> kSkColorFilter_Type -> kColorFilter_Type

Ah, this one's on me.  I think we want a slight style exception here.  These
guys have the full SkFoo name in them so that we can drop the full class name
into SK_DEFINE_FLATTENABLE_TYPE().  We can concat in a macro but not cut:

SK_DEFINE_FLATTENABLE_TYPE(SkDrawLooper)  ---> kSkDrawLooper_Type OK
SK_DEFINE_FLATTENABLE_TYPE(DrawLooper)    ---> kSkDrawLooper_Type OK
SK_DEFINE_FLATTENABLE_TYPE(DrawLooper)    ---> kDrawLooper_Type   OK
SK_DEFINE_FLATTENABLE_TYPE(SkDrawLooper)  ---> kDrawLooper_Type   NOPE

I personally thought it looked funny to see
SK_DEFINE_FLATTENABLE_TYPE(DrawLooper) inside SkDrawLooper.

[Stephen White, 2013-10-21 21:22:36]

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

https://codereview.chromium.org/23021015/diff/269001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1614: this->setIsOpaque(buffer.readBool());
Where is this bool written? I don't see it in flatten().

https://codereview.chromium.org/23021015/diff/269001/src/core/SkOrderedWriteB...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/269001/src/core/SkOrderedWriteB...
src/core/SkOrderedWriteBuffer.cpp:274: this->writeString("");
Will this change the serialization of SkPictures? If so, we'll need to bump the
version number and regenerate them. (I don't know how this is done or who's on
the hook, I just wanted to raise it in case someone more clueful than me needed
to know.)

[scroggo, 2013-10-21 21:37:21]

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

https://codereview.chromium.org/23021015/diff/269001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1614: this->setIsOpaque(buffer.readBool());
On 2013/10/21 21:22:37, Stephen White wrote:
> Where is this bool written? I don't see it in flatten().

This line was deleted in a recent CL, and this one failed to merge that
deletion. This will break unflattening bitmaps. I don't think this will affect
SkPicture, though, or our picture tests, since they skip unflatten. Good catch!

https://codereview.chromium.org/23021015/diff/269001/src/core/SkOrderedWriteB...
File src/core/SkOrderedWriteBuffer.cpp (right):

https://codereview.chromium.org/23021015/diff/269001/src/core/SkOrderedWriteB...
src/core/SkOrderedWriteBuffer.cpp:274: this->writeString("");
On 2013/10/21 21:22:37, Stephen White wrote:
> Will this change the serialization of SkPictures? If so, we'll need to bump
the
> version number and regenerate them. (I don't know how this is done or who's on
> the hook, I just wanted to raise it in case someone more clueful than me
needed
> to know.)

I'm guessing this is fine - I think isValidating should return false for
SkPictures.

[sugoi1, 2013-10-22 12:09:55]

On 2013/10/21 20:55:29, reed1 wrote:
> are we in a position with this CL to write tests that exercise/trigger the
error detectors?

Yes, I have another cl with changes to the fuzzer that does that locally.
Of course, unit tests will also be added before SkValidatingReadBuffer is used.

[sugoi1, 2013-10-22 12:12:09]

On 2013/10/21 21:37:21, scroggo wrote:
> https://codereview.chromium.org/23021015/diff/269001/src/core/SkBitmap.cpp
> File src/core/SkBitmap.cpp (right):
> 
>
https://codereview.chromium.org/23021015/diff/269001/src/core/SkBitmap.cpp#ne...
> src/core/SkBitmap.cpp:1614: this->setIsOpaque(buffer.readBool());
> On 2013/10/21 21:22:37, Stephen White wrote:
> > Where is this bool written? I don't see it in flatten().
> 
> This line was deleted in a recent CL, and this one failed to merge that
> deletion. This will break unflattening bitmaps. I don't think this will affect
> SkPicture, though, or our picture tests, since they skip unflatten. Good
catch!
> 
>
https://codereview.chromium.org/23021015/diff/269001/src/core/SkOrderedWriteB...
> File src/core/SkOrderedWriteBuffer.cpp (right):
> 
>
https://codereview.chromium.org/23021015/diff/269001/src/core/SkOrderedWriteB...
> src/core/SkOrderedWriteBuffer.cpp:274: this->writeString("");
> On 2013/10/21 21:22:37, Stephen White wrote:
> > Will this change the serialization of SkPictures? If so, we'll need to bump
> the
> > version number and regenerate them. (I don't know how this is done or who's
on
> > the hook, I just wanted to raise it in case someone more clueful than me
> needed
> > to know.)
> 
> I'm guessing this is fine - I think isValidating should return false for
> SkPictures.

Yes, isValidating is currently unused in this cl, and, in the short term, it
will only be used within SkFlattenableSerialization.cpp for the filters
serialization.

[sugoi1, 2013-10-22 12:12:53]

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

https://codereview.chromium.org/23021015/diff/269001/include/core/SkFlattenab...
include/core/SkFlattenableBuffers.h:137: template <typename T> T*
readFlattenableT();
On 2013/10/21 20:54:48, reed1 wrote:
> don't really care, but why is this guy private?

We don't need it in any place except within the readFoo() functions, so there
was no reason to expose it in the public API, which is why I thought it would be
appropriate to make it private.

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

https://codereview.chromium.org/23021015/diff/269001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1614: this->setIsOpaque(buffer.readBool());
On 2013/10/21 21:22:37, Stephen White wrote:
> Where is this bool written? I don't see it in flatten().

Good catch on the bad merge. Line deleted.

[reed1, 2013-10-23 16:13:28]

lgtm

[commit-bot: I haz the power, 2013-10-23 16:37:08]

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

[commit-bot: I haz the power, 2013-10-23 17:06:36]

Change committed as 11922