Checking structure sizes before reading them from memory to avoid overflowing the buffer's stream.

Checking structure sizes before reading them from memory to avoid overflowing the buffer's stream.

BUG=
R=reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12114

Committed: https://code.google.com/p/skia/source/detail?r=12119

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

[sugoi1, 2013-10-24 19:51:38]

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

https://codereview.chromium.org/41253002/diff/1/src/core/SkPathRef.cpp#newcod...
src/core/SkPathRef.cpp:158: //       the buffer's peek forward (while making
sure we can read these values)
This isn't completed yet. I need some suggestions. I need to figure out a way to
read verbCount, pointCount, conicCount without moving the buffer's peek forward.
I'd like to do this in a clean way, but I see no way of doing this with
SkFlattenableReadBuffer right now.

Maybe an "unflatten" function could be added by SkPath, SkPathRef, SkRegion
instead of trying to precompute their size before reading these objects. Any
idea, opinion ?

[sugoi1, 2013-10-24 20:41:57]

On 2013/10/24 19:51:38, sugoi1 wrote:
> https://codereview.chromium.org/41253002/diff/1/src/core/SkPathRef.cpp
> File src/core/SkPathRef.cpp (right):
> 
>
https://codereview.chromium.org/41253002/diff/1/src/core/SkPathRef.cpp#newcod...
> src/core/SkPathRef.cpp:158: //       the buffer's peek forward (while making
> sure we can read these values)
> This isn't completed yet. I need some suggestions. I need to figure out a way
to
> read verbCount, pointCount, conicCount without moving the buffer's peek
forward.
> I'd like to do this in a clean way, but I see no way of doing this with
> SkFlattenableReadBuffer right now.
> 
> Maybe an "unflatten" function could be added by SkPath, SkPathRef, SkRegion
> instead of trying to precompute their size before reading these objects. Any
> idea, opinion ?

Update : I tried to unflatten an SkPathRef object using an
SkFlattenableReadBuffer. There's an unfortunate issue that SkPathRef's verbs are
uint8_t and the number of verbs isn't necessarily a multiple of 4, so
SkPathRef's read/write operations are currently not word aligned, which can't
work with SkFlattenableReadBuffer, so I guess I'm stuck with the current attempt
at figuring out the entire size before reading...

[reed1, 2013-10-24 20:51:19]

Not a fan of teaching our (very) core classes about readbuffers. Why this
change?

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

https://codereview.chromium.org/41253002/diff/1/include/core/SkMatrix.h#newco...
include/core/SkMatrix.h:562: uint32_t sizeInMemory() const;
Doesn't writeToMemory(NULL) return this same value?

https://codereview.chromium.org/41253002/diff/1/include/core/SkMatrix.h#newco...
include/core/SkMatrix.h:564: static uint32_t SizeToRead();
This need not always be the same value for all matrices (even if it happens to
be so today).

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

https://codereview.chromium.org/41253002/diff/1/include/core/SkPath.h#newcode920
include/core/SkPath.h:920: static uint32_t SizeToRead(SkFlattenableReadBuffer&
buffer);
:( Do we really need to teach SkPath about readbuffers? What is wrong with the
current memory-based interface?

[sugoi1, 2013-10-25 11:49:37]

On 2013/10/24 20:51:19, reed1 wrote:
> Not a fan of teaching our (very) core classes about readbuffers. Why this
> change?
> 
> https://codereview.chromium.org/41253002/diff/1/include/core/SkMatrix.h
> File include/core/SkMatrix.h (right):
> 
>
https://codereview.chromium.org/41253002/diff/1/include/core/SkMatrix.h#newco...
> include/core/SkMatrix.h:562: uint32_t sizeInMemory() const;
> Doesn't writeToMemory(NULL) return this same value?
> 
>
https://codereview.chromium.org/41253002/diff/1/include/core/SkMatrix.h#newco...
> include/core/SkMatrix.h:564: static uint32_t SizeToRead();
> This need not always be the same value for all matrices (even if it happens to
> be so today).
> 
> https://codereview.chromium.org/41253002/diff/1/include/core/SkPath.h
> File include/core/SkPath.h (right):
> 
>
https://codereview.chromium.org/41253002/diff/1/include/core/SkPath.h#newcode920
> include/core/SkPath.h:920: static uint32_t SizeToRead(SkFlattenableReadBuffer&
> buffer);
> :( Do we really need to teach SkPath about readbuffers? What is wrong with the
> current memory-based interface?

Never mind, I think I figured it out (in my sleep). I'll just send the remaining
length of the stream as an (int) argument and make sure I don't read past that.

[sugoi1, 2013-10-25 12:31:12]

Ok, here's the new solution. I think it's cleaner and it makes more sense than
the previous solution. It's not completed, because I still need to add size
checks to SkRBuffer in my case, but it should give you the general idea of what
I'm trying to do.

[reed1, 2013-10-25 14:42:45]

General thought about this "readFromMemory" pattern.

Seems like we should *not* support the notion of calling it with NULL, and
pretending we can return something meaningful for the 'bytes I would have read'.
Sometimes a given class and impl can answer that (e.g. matrix today always
reads/writes the same number of bytes), but often (e.g. Region) we just can't
know, since the answer depends on the first N bytes that we inspect.

So, what is a well-defined protocol for this sort of API, that can work for
simple and complex types?

https://codereview.chromium.org/41253002/diff/100001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.chromium.org/41253002/diff/100001/include/core/SkMatrix.h#...
include/core/SkMatrix.h:563: // return the number of bytes read
// Please document what 'length' is used for.

[sugoi1, 2013-10-25 14:59:02]

https://codereview.chromium.org/41253002/diff/100001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.chromium.org/41253002/diff/100001/include/core/SkMatrix.h#...
include/core/SkMatrix.h:563: // return the number of bytes read
On 2013/10/25 14:42:46, reed1 wrote:
> // Please document what 'length' is used for.

Done.

[sugoi1, 2013-10-25 15:08:58]

On 2013/10/25 14:42:45, reed1 wrote:
> General thought about this "readFromMemory" pattern.
> 
> Seems like we should *not* support the notion of calling it with NULL, and
> pretending we can return something meaningful for the 'bytes I would have
read'.
> Sometimes a given class and impl can answer that (e.g. matrix today always
> reads/writes the same number of bytes), but often (e.g. Region) we just can't
> know, since the answer depends on the first N bytes that we inspect.
> 
> So, what is a well-defined protocol for this sort of API, that can work for
> simple and complex types?

Hmmm... I don't think we can do this easily with the current structure. Instead
of defining such an API, I think we should make non backward compatible changes
to these classes so that they are easier to read. For example, in SkRegion,
instead of reading a count AND an array of integers, this should be receiving
data read with readIntArray(). Same thing with SkPathRef::CreateFromBuffer(),
which is stored with a structure like this :
size of array 1, size of array 2, size of array 3, array 1, array 2, array 3
And to make matters worse, array 1 isn't necessarily word aligned, so array 2
and array 3 may also not be.
Also, this structure is not compatible with the read/write arrays from
SkFlattenableBuffers.
All this data could be read inside SkFlattenableReadBuffer and passed to a
SkRegion/SkPath constructor afterwards, which would prevent us from having to
use SkRBuffer within these classes.

[mtklein, 2013-10-25 16:07:47]

On 2013/10/25 15:08:58, sugoi1 wrote:
> On 2013/10/25 14:42:45, reed1 wrote:
> > General thought about this "readFromMemory" pattern.
> > 
> > Seems like we should *not* support the notion of calling it with NULL, and
> > pretending we can return something meaningful for the 'bytes I would have
> read'.
> > Sometimes a given class and impl can answer that (e.g. matrix today always
> > reads/writes the same number of bytes), but often (e.g. Region) we just
can't
> > know, since the answer depends on the first N bytes that we inspect.
> > 
> > So, what is a well-defined protocol for this sort of API, that can work for
> > simple and complex types?
> 
> Hmmm... I don't think we can do this easily with the current structure.
Instead
> of defining such an API, I think we should make non backward compatible
changes
> to these classes so that they are easier to read. For example, in SkRegion,
> instead of reading a count AND an array of integers, this should be receiving
> data read with readIntArray(). Same thing with SkPathRef::CreateFromBuffer(),
> which is stored with a structure like this :
> size of array 1, size of array 2, size of array 3, array 1, array 2, array 3
> And to make matters worse, array 1 isn't necessarily word aligned, so array 2
> and array 3 may also not be.
> Also, this structure is not compatible with the read/write arrays from
> SkFlattenableBuffers.
> All this data could be read inside SkFlattenableReadBuffer and passed to a
> SkRegion/SkPath constructor afterwards, which would prevent us from having to
> use SkRBuffer within these classes.

Is there anything to be won if we move the responsibility for reading these data
structures up a level?  I.e. can we remove all the readFromMemory methods
entirely and have, say, SkReader32 take care of them?  Does SkReader32 have a
safer perspective on how many bytes we can read?  Can we reconstruct these
things just as efficiently there?

[sugoi1, 2013-10-25 17:01:31]

On 2013/10/25 16:07:47, mtklein wrote:
> On 2013/10/25 15:08:58, sugoi1 wrote:
> > On 2013/10/25 14:42:45, reed1 wrote:
> > > General thought about this "readFromMemory" pattern.
> > > 
> > > Seems like we should *not* support the notion of calling it with NULL, and
> > > pretending we can return something meaningful for the 'bytes I would have
> > read'.
> > > Sometimes a given class and impl can answer that (e.g. matrix today always
> > > reads/writes the same number of bytes), but often (e.g. Region) we just
> can't
> > > know, since the answer depends on the first N bytes that we inspect.
> > > 
> > > So, what is a well-defined protocol for this sort of API, that can work
for
> > > simple and complex types?
> > 
> > Hmmm... I don't think we can do this easily with the current structure.
> Instead
> > of defining such an API, I think we should make non backward compatible
> changes
> > to these classes so that they are easier to read. For example, in SkRegion,
> > instead of reading a count AND an array of integers, this should be
receiving
> > data read with readIntArray(). Same thing with
SkPathRef::CreateFromBuffer(),
> > which is stored with a structure like this :
> > size of array 1, size of array 2, size of array 3, array 1, array 2, array 3
> > And to make matters worse, array 1 isn't necessarily word aligned, so array
2
> > and array 3 may also not be.
> > Also, this structure is not compatible with the read/write arrays from
> > SkFlattenableBuffers.
> > All this data could be read inside SkFlattenableReadBuffer and passed to a
> > SkRegion/SkPath constructor afterwards, which would prevent us from having
to
> > use SkRBuffer within these classes.
> 
> Is there anything to be won if we move the responsibility for reading these
data
> structures up a level?  I.e. can we remove all the readFromMemory methods
> entirely and have, say, SkReader32 take care of them?  Does SkReader32 have a
> safer perspective on how many bytes we can read?  Can we reconstruct these
> things just as efficiently there?

Yes, I think we should be able to remove all readFromMemory() functions if we
have a version of these within SkReader32, but the safer implementation would
have to be done within SkValidatingReadBuffer, not within SkReader32, just like
the other "safer" functions have a specific implementation within
SkValidatingReadBuffer.
The unaligned data issue is currently a problem for reading old setups, though,
since we have many asserts within SkReader32 that verify that we're word
aligned. It's not impossible to work around it, though, by reading chunks of
data and manually copying the memory, it's simply not as elegant as calling
SkFlattenableReadBuffer::read...Array().

[sugoi1, 2013-10-25 18:05:07]

On 2013/10/25 17:01:31, sugoi1 wrote:
> On 2013/10/25 16:07:47, mtklein wrote:
> > On 2013/10/25 15:08:58, sugoi1 wrote:
> > > On 2013/10/25 14:42:45, reed1 wrote:
> > > > General thought about this "readFromMemory" pattern.
> > > > 
> > > > Seems like we should *not* support the notion of calling it with NULL,
and
> > > > pretending we can return something meaningful for the 'bytes I would
have
> > > read'.
> > > > Sometimes a given class and impl can answer that (e.g. matrix today
always
> > > > reads/writes the same number of bytes), but often (e.g. Region) we just
> > can't
> > > > know, since the answer depends on the first N bytes that we inspect.
> > > > 
> > > > So, what is a well-defined protocol for this sort of API, that can work
> for
> > > > simple and complex types?
> > > 
> > > Hmmm... I don't think we can do this easily with the current structure.
> > Instead
> > > of defining such an API, I think we should make non backward compatible
> > changes
> > > to these classes so that they are easier to read. For example, in
SkRegion,
> > > instead of reading a count AND an array of integers, this should be
> receiving
> > > data read with readIntArray(). Same thing with
> SkPathRef::CreateFromBuffer(),
> > > which is stored with a structure like this :
> > > size of array 1, size of array 2, size of array 3, array 1, array 2, array
3
> > > And to make matters worse, array 1 isn't necessarily word aligned, so
array
> 2
> > > and array 3 may also not be.
> > > Also, this structure is not compatible with the read/write arrays from
> > > SkFlattenableBuffers.
> > > All this data could be read inside SkFlattenableReadBuffer and passed to a
> > > SkRegion/SkPath constructor afterwards, which would prevent us from having
> to
> > > use SkRBuffer within these classes.
> > 
> > Is there anything to be won if we move the responsibility for reading these
> data
> > structures up a level?  I.e. can we remove all the readFromMemory methods
> > entirely and have, say, SkReader32 take care of them?  Does SkReader32 have
a
> > safer perspective on how many bytes we can read?  Can we reconstruct these
> > things just as efficiently there?
> 
> Yes, I think we should be able to remove all readFromMemory() functions if we
> have a version of these within SkReader32, but the safer implementation would
> have to be done within SkValidatingReadBuffer, not within SkReader32, just
like
> the other "safer" functions have a specific implementation within
> SkValidatingReadBuffer.
> The unaligned data issue is currently a problem for reading old setups,
though,
> since we have many asserts within SkReader32 that verify that we're word
> aligned. It's not impossible to work around it, though, by reading chunks of
> data and manually copying the memory, it's simply not as elegant as calling
> SkFlattenableReadBuffer::read...Array().

I made an example with SkMatrix, using only SkReader32 and SkWriter32 to
read/write this object and removing SkMatrix::writeToMemory() /
SkMatrix::readFromMemory() (or any knowledge of "flattening" from SkMatrix). You
can look at the files modified between patch set 4 and 5 and tell me what you
think. I can attempt to do the same thing with SkPath and SkRegion.

[mtklein, 2013-10-25 18:11:49]

Yeah, do you think you could try SkPath?  SkMatrix is sort of too trivial to see
if it's going to generalize well.

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

https://codereview.chromium.org/41253002/diff/240001/include/core/SkReader32....
include/core/SkReader32.h:116: matrix->setAll(this->readScalar(),
this->readScalar(), this->readScalar(),
Um, I think we've got to do this outside the setAll.  Those this->readScalar()s
don't execute in any particular order.

https://codereview.chromium.org/41253002/diff/240001/include/core/SkWriter32.h
File include/core/SkWriter32.h (right):

https://codereview.chromium.org/41253002/diff/240001/include/core/SkWriter32....
include/core/SkWriter32.h:133: this->writeScalar(matrix[i]);
Were we just never writing the flags at all?

[sugoi1, 2013-10-25 18:27:20]

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

https://codereview.chromium.org/41253002/diff/240001/include/core/SkReader32....
include/core/SkReader32.h:116: matrix->setAll(this->readScalar(),
this->readScalar(), this->readScalar(),
On 2013/10/25 18:11:49, mtklein wrote:
> Um, I think we've got to do this outside the setAll.  Those
this->readScalar()s don't execute in any particular order.

Ah, I always thought the arguments were processed in order, will fix.

https://codereview.chromium.org/41253002/diff/240001/include/core/SkWriter32.h
File include/core/SkWriter32.h (right):

https://codereview.chromium.org/41253002/diff/240001/include/core/SkWriter32....
include/core/SkWriter32.h:133: this->writeScalar(matrix[i]);
On 2013/10/25 18:11:49, mtklein wrote:
> Were we just never writing the flags at all?

No, SkMatrix::writeToMemory() was only writing the SkScalar values.

[sugoi1, 2013-10-25 18:28:01]

On 2013/10/25 18:11:49, mtklein wrote:
> Yeah, do you think you could try SkPath?  SkMatrix is sort of too trivial to
see
> if it's going to generalize well.

SkPath and SkRegion are on their way...

[sugoi1, 2013-10-25 19:46:14]

Ok, so I did SkRegion first, because it seemed a bit simpler. Here's what I had
to do :

- Although SkRegion no longer has serialization code per se, it needs to be
friends with SkReader32 / SkWriter32 so that they can access private data
without exposing more of SkRegion in its public API.
- A few functions were added to SkRegion so that SkReader32 / SkWriter32 do not
need to depend on SkRegionPriv.h (they're all trivial "getter" functions)

[sugoi1, 2013-10-25 21:02:25]

Ok, so after all this, it seems that removing readFromMemory / writeToMemory
will be hard / ugly and, really, for now, all I need is patch 4. I suggest we go
back to it, since the amount of code necessary to do what is attempted in
patched 5 and 6 is very large and requires friend classes just to get access to
the data. It may be a good long term goal, but I don't think such a thing is
currently required.

[reed1, 2013-10-29 19:42:57]

https://codereview.chromium.org/41253002/diff/650001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.chromium.org/41253002/diff/650001/include/core/SkMatrix.h#...
include/core/SkMatrix.h:570: uint32_t readFromMemory(const void* buffer,
uint32_t length);
size_t ?

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

https://codereview.chromium.org/41253002/diff/650001/include/core/SkReader32....
include/core/SkReader32.h:112: (void)this->skip(size);
if readFromMemory returns 0 (e.g. truncated stream), shouldn't we skip available
instead of zero?

[sugoi1, 2013-10-29 20:16:46]

https://codereview.chromium.org/41253002/diff/650001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.chromium.org/41253002/diff/650001/include/core/SkMatrix.h#...
include/core/SkMatrix.h:570: uint32_t readFromMemory(const void* buffer,
uint32_t length);
On 2013/10/29 19:42:58, reed1 wrote:
> size_t ?

I assume this comment applies to all readFromMemory and writeToMemory functions.
Will do.

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

https://codereview.chromium.org/41253002/diff/650001/include/core/SkReader32....
include/core/SkReader32.h:112: (void)this->skip(size);
On 2013/10/29 19:42:58, reed1 wrote:
> if readFromMemory returns 0 (e.g. truncated stream), shouldn't we skip
available instead of zero?

Ok (I had to think about it a little). This would automatically cause an error
on the next read operation, either by asserting in debug or by reading
out-of-bounds in release (which is a bit more unpredictable), but it's probably
better overall than leaving peek() at its current position, since that would
increase the chance of trapping an error.

[Stephen White, 2013-10-29 20:40:53]

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

https://codereview.chromium.org/41253002/diff/650001/include/core/SkReader32....
include/core/SkReader32.h:112: (void)this->skip(size);
On 2013/10/29 20:16:47, sugoi1 wrote:
> On 2013/10/29 19:42:58, reed1 wrote:
> > if readFromMemory returns 0 (e.g. truncated stream), shouldn't we skip
> available instead of zero?
> 
> Ok (I had to think about it a little). This would automatically cause an error
> on the next read operation, either by asserting in debug or by reading
> out-of-bounds in release (which is a bit more unpredictable), but it's
probably
> better overall than leaving peek() at its current position, since that would
> increase the chance of trapping an error.

Reading out-of-bounds doesn't sound good. Is that something that only the
non-validating code will trigger?

BTW, is there a way we can make unit tests for all these cases? Perhaps by
serializing a valid stream, truncating it at specific points and attempting to
deserialize that?

[sugoi1, 2013-10-30 15:21:41]

Added tests, made changes from uint32_t to size_t and modified rrect, so that it
is the same as path/matrix/region.

[sugoi1, 2013-10-30 15:22:39]

On 2013/10/29 20:40:53, Stephen White wrote:
> Reading out-of-bounds doesn't sound good. Is that something that only the
> non-validating code will trigger?

Yes, the validating code does not read oob.

[reed1, 2013-10-30 15:37:07]

thanks, this is getting better each time. I still want to be sure the semantics
of read/writeFromMemory are very clear, and that reader exactly mirrors that.

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

https://codereview.chromium.org/41253002/diff/760001/include/core/SkReader32....
include/core/SkReader32.h:109: void readPath(SkPath* path) {
now it seems that all of these read-helpers must return a bool, or some other
way to indicate that we did or did not initialize the output parameter.

https://codereview.chromium.org/41253002/diff/760001/include/core/SkReader32....
include/core/SkReader32.h:112: (void)this->skip(size > 0 ? size :
this->available());
I think a quick comment in the code would help here:

// if readFromMemory fails (available was too small), it returns 0, in which
case we want to skip to the end

[sugoi1, 2013-10-30 18:07:03]

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

https://codereview.chromium.org/41253002/diff/760001/include/core/SkReader32....
include/core/SkReader32.h:109: void readPath(SkPath* path) {
On 2013/10/30 15:37:07, reed1 wrote:
> now it seems that all of these read-helpers must return a bool, or some other
> way to indicate that we did or did not initialize the output parameter.

Done.

https://codereview.chromium.org/41253002/diff/760001/include/core/SkReader32....
include/core/SkReader32.h:112: (void)this->skip(size > 0 ? size :
this->available());
On 2013/10/30 15:37:07, reed1 wrote:
> I think a quick comment in the code would help here:
> 
> // if readFromMemory fails (available was too small), it returns 0, in which
> case we want to skip to the end

Done.

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

https://codereview.chromium.org/41253002/diff/1060001/include/core/SkReader32...
include/core/SkReader32.h:109: void readPath(SkPath* path) {
These 4 functions had essentially the same code, so I unified them in a single
template function.

[reed1, 2013-10-30 18:10:20]

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

https://codereview.chromium.org/41253002/diff/1060001/include/core/SkReader32...
include/core/SkReader32.h:143: SkASSERT(SkAlign4(size) == size && success);
not sure we can keep the assert, if the call now returns a bool for
success/failure. Will make it very hard to write unit-tests...

https://codereview.chromium.org/41253002/diff/1060001/include/core/SkReader32...
include/core/SkReader32.h:145: (void)this->skip(success ? size :
this->available());
does skip() return something that might also indicate failure? If so, perhaps we
should return that, instead of success.?

[sugoi1, 2013-10-30 18:26:54]

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

https://codereview.chromium.org/41253002/diff/1060001/include/core/SkReader32...
include/core/SkReader32.h:143: SkASSERT(SkAlign4(size) == size && success);
On 2013/10/30 18:10:21, reed1 wrote:
> not sure we can keep the assert, if the call now returns a bool for
> success/failure. Will make it very hard to write unit-tests...

Done.

https://codereview.chromium.org/41253002/diff/1060001/include/core/SkReader32...
include/core/SkReader32.h:145: (void)this->skip(success ? size :
this->available());
On 2013/10/30 18:10:21, reed1 wrote:
> does skip() return something that might also indicate failure? If so, perhaps
we should return that, instead of success.?

Hmmm... skip() returns the address of the pointer at the beginning of the
skipped area, that's not really helpful. Also, skipping 0 is legal, so that
doesn't really help either.

[reed1, 2013-10-30 18:38:00]

On 2013/10/30 18:26:54, sugoi1 wrote:
>
https://codereview.chromium.org/41253002/diff/1060001/include/core/SkReader32.h
> File include/core/SkReader32.h (right):
> 
>
https://codereview.chromium.org/41253002/diff/1060001/include/core/SkReader32...
> include/core/SkReader32.h:143: SkASSERT(SkAlign4(size) == size && success);
> On 2013/10/30 18:10:21, reed1 wrote:
> > not sure we can keep the assert, if the call now returns a bool for
> > success/failure. Will make it very hard to write unit-tests...
> 
> Done.
> 
>
https://codereview.chromium.org/41253002/diff/1060001/include/core/SkReader32...
> include/core/SkReader32.h:145: (void)this->skip(success ? size :
> this->available());
> On 2013/10/30 18:10:21, reed1 wrote:
> > does skip() return something that might also indicate failure? If so,
perhaps
> we should return that, instead of success.?
> 
> Hmmm... skip() returns the address of the pointer at the beginning of the
> skipped area, that's not really helpful. Also, skipping 0 is legal, so that
> doesn't really help either.

If skip can fail (e.g. we ask for too much), we would still be returning
'success', which seems confusing.

[sugoi1, 2013-10-30 18:56:17]

On 2013/10/30 18:38:00, reed1 wrote:
> If skip can fail (e.g. we ask for too much), we would still be returning
'success', which seems confusing.

A generic call to skip() might fail, but here, if success is true, the size to
skip has to be <= available(), so, for this particular use case, if we were to
check if skip() succeeded, it would be a redundant check.

Also to note : I know we mentioned this, but none of this is used by the
validation code, all the code in SkReader32.h adds validation checks to
non-validating code. There's currently no code that verifies the output of
readObjectFromMemory(), so the modifications here were only made so that this
file compiles with the new signature for readFromMemory(). I don't mind adding
more logic here, I just want to make sure this doesn't burden the non validating
code too much.

[sugoi1, 2013-10-31 15:47:47]

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

https://codereview.chromium.org/41253002/diff/1120001/include/core/SkReader32...
include/core/SkReader32.h:142: bool success = (size > 0) && (size <=
this->available()) && (SkAlign4(size) == size);
I added a check here that size <= this->available(), to make sure that :
1 ) We can skip it
2 ) It returns an error if we can't

[sugoi1, 2013-11-01 18:15:41]

Merge tests together

[reed1, 2013-11-01 18:27:21]

looking good to me.

I think you correctly check that the returned size is a multiple of 4 in the
reader... but we don't document this in any of the classes the "implement"
readFromMemory/writeToMemory, nor do we test for it in our tests.

[sugoi1, 2013-11-01 18:52:35]

On 2013/11/01 18:27:21, reed1 wrote:
> looking good to me.
> 
> I think you correctly check that the returned size is a multiple of 4 in the
> reader... but we don't document this in any of the classes the "implement"
> readFromMemory/writeToMemory, nor do we test for it in our tests.

Ok, will do.

[sugoi1, 2013-11-04 13:15:58]

On 2013/11/01 18:52:35, sugoi1 wrote:
> On 2013/11/01 18:27:21, reed1 wrote:
> > looking good to me.
> > 
> > I think you correctly check that the returned size is a multiple of 4 in the
> > reader... but we don't document this in any of the classes the "implement"
> > readFromMemory/writeToMemory, nor do we test for it in our tests.
> 
> Ok, will do.

Done

[reed1, 2013-11-04 14:26:53]

thanks for the checks. lgtm but I think we could do a more concise job on the
similar-looking-tests.

https://codereview.chromium.org/41253002/diff/1230001/tests/SerializationTest...
File tests/SerializationTest.cpp (right):

https://codereview.chromium.org/41253002/diff/1230001/tests/SerializationTest...
tests/SerializationTest.cpp:13: // Test matrix serialization
Looks like a lot of similar code. Can any of this be
consolidate/templated/shared?

[sugoi1, 2013-11-04 15:26:13]

https://codereview.chromium.org/41253002/diff/1230001/tests/SerializationTest...
File tests/SerializationTest.cpp (right):

https://codereview.chromium.org/41253002/diff/1230001/tests/SerializationTest...
tests/SerializationTest.cpp:13: // Test matrix serialization
On 2013/11/04 14:26:54, reed1 wrote:
> Looks like a lot of similar code. Can any of this be
> consolidate/templated/shared?

Done.

[reed1, 2013-11-04 15:45:47]

danke
lgtm

[commit-bot: I haz the power, 2013-11-04 16:00:52]

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

[sugoi1, 2013-11-04 16:18:24]

Committed patchset #15 manually as r12114 (presubmit successful).

[sugoi1, 2013-11-04 18:41:36]

https://codereview.chromium.org/41253002/diff/1410001/tests/SerializationTest...
File tests/SerializationTest.cpp (right):

https://codereview.chromium.org/41253002/diff/1410001/tests/SerializationTest...
tests/SerializationTest.cpp:111: SkValidatingReadBuffer buffer(dataWritten,
bytesWritten - 4);
This was a silly error. I originally wrote "- 1" instead of "- 4", which
resulted in a non multiple of 4 size, which was causing the buffer to
immediately fail and, since we're expecting this test to fail, the test itself
didn't pick up that a failure here was unexpected.

[commit-bot: I haz the power, 2013-11-04 18:45:11]

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

[sugoi1, 2013-11-04 20:28:33]

Committed patchset #17 manually as r12119 (presubmit successful).

[commit-bot: I haz the power, 2013-11-05 15:21:11]

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

[commit-bot: I haz the power, 2013-11-05 15:47:03]

Change committed as 12130

[bungeman-skia, 2013-11-27 17:11:26]

https://codereview.chromium.org/41253002/diff/1570001/tests/SerializationTest...
File tests/SerializationTest.cpp (right):

https://codereview.chromium.org/41253002/diff/1570001/tests/SerializationTest...
tests/SerializationTest.cpp:177: TestAlignment(&rrect, reporter);
This test is causing Valgrind to complain about SkRRect's readFromMemory
branching on uninitialized values. SkRRect starts out uninitialized; it requires
setXXX to be called before using it. Please see
https://code.google.com/p/skia/source/detail?r=12419 which fixes this (by
ensuring that all of the data in these tests are initialized).