Fixed bad bitmap size crashes

Fixed bad bitmap size crashes

There were 2 issues :
1 ) If the size of an SkBitmap's underlying SkPixelRef's alocated memory is too small to fit the bitmap, then the deserialization will now check this and set an error appropriately.
2 ) If a device fails to allocate its pixels, the device will be deleted and NULL will be returned to avoid attempting to draw on a bad device.

BUG=

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

[Stephen White, 2013-11-27 21:52:49]

LGTM for src/effects. Leaving the rest for Mike.

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

https://codereview.chromium.org/92793002/diff/1/include/core/SkPixelRef.h#new...
include/core/SkPixelRef.h:261: virtual size_t getSize() const;
Nit: maybe getSizeInBytes(), if that's what this is?

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

As this will be redundant when my rowbytes/info CL lands...

1. Are there real crashes in the current code?
2. Do we have unittests for this?
3. Can this wait for my rowbytes/info CL?

[sugoi, 2013-11-27 22:05:28]

On 2013/11/27 21:56:01, reed1 wrote:
> As this will be redundant when my rowbytes/info CL lands...
> 
> 1. Are there real crashes in the current code?
The are real crashes, using the fuzzer with bitflips in the serialized stream.

> 2. Do we have unittests for this?
I can add some.

> 3. Can this wait for my rowbytes/info CL?
Yes, as long as it also does make sure that the allocated memory inside
SkPixelRef isn't too small for the rowbytes/info.

[reed1, 2013-11-27 22:09:45]

On 2013/11/27 22:05:28, sugoi wrote:
> On 2013/11/27 21:56:01, reed1 wrote:
> > As this will be redundant when my rowbytes/info CL lands...
> > 
> > 1. Are there real crashes in the current code?
> The are real crashes, using the fuzzer with bitflips in the serialized stream.
> 
> > 2. Do we have unittests for this?
> I can add some.
> 
> > 3. Can this wait for my rowbytes/info CL?
> Yes, as long as it also does make sure that the allocated memory inside
> SkPixelRef isn't too small for the rowbytes/info.

Sure. I think we will be able to keep getsize(), it will just be non-virtual,
accessing the virtual data about SkImageInfo and rowbytes (which will be
intrinsic to the pixelref).

[Stephen White, 2013-11-27 22:22:13]

On 2013/11/27 21:56:01, reed1 wrote:
> As this will be redundant when my rowbytes/info CL lands...
> 
> 1. Are there real crashes in the current code?
> 2. Do we have unittests for this?

+1 for tests (I forgot to mention).

> 3. Can this wait for my rowbytes/info CL?

Just to add an idea of timing, we'd like to get these changes in for the next
branch point, which is coming up quickly. Contact me off-rietveld if that
doesn't fit in with your plans.

[sugoi, 2013-11-28 18:22:40]

Added tests

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

https://codereview.chromium.org/92793002/diff/20001/src/core/SkBitmap.cpp#new...
src/core/SkBitmap.cpp:1572: 
Simplified this a bit by using getSafeSize()

[sugoi, 2013-11-29 20:32:42]

I added more "randomness" to the fuzzer and fixed most of the issues found by
that change.

[Stephen White, 2013-12-02 15:33:23]

effects/ still LGTM.

https://codereview.chromium.org/92793002/diff/40001/src/effects/SkMagnifierIm...
File src/effects/SkMagnifierImageFilter.cpp (right):

https://codereview.chromium.org/92793002/diff/40001/src/effects/SkMagnifierIm...
src/effects/SkMagnifierImageFilter.cpp:288: if ((src.config() !=
SkBitmap::kARGB_8888_Config) ||
Should we be checking for the other things we validate on above? I.e., fInset is
finite, fSrcRect is valid, etc, so that the new-filter case is consistent with
the deserialized case? Or is that foolish consistency?

[sugoi1, 2013-12-02 15:42:56]

https://codereview.chromium.org/92793002/diff/40001/src/effects/SkMagnifierIm...
File src/effects/SkMagnifierImageFilter.cpp (right):

https://codereview.chromium.org/92793002/diff/40001/src/effects/SkMagnifierIm...
src/effects/SkMagnifierImageFilter.cpp:288: if ((src.config() !=
SkBitmap::kARGB_8888_Config) ||
On 2013/12/02 15:33:24, Stephen White wrote:
> Should we be checking for the other things we validate on above? I.e., fInset
is
> finite, fSrcRect is valid, etc, so that the new-filter case is consistent with
> the deserialized case? Or is that foolish consistency?

Hmmm... it might be overkill to do this at rendering time. If we want to be
consistent with the deserialized case, then maybe we could add checks at
construction time, but then this would apply to essentially all filters.

[sugoi, 2013-12-03 16:24:22]

I extracted a subset of this cl into https://codereview.chromium.org/102213002/.
I'll update this one once the other cl has landed.

[reed1, 2013-12-03 19:02:50]

There are now other CLs that may block the rowbytes CL, so lets see if we can
land this guy (with minimal api impact on the baseclass). Not the end of the
world if we circle back later and undo some parts if they become obsoleted.

https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h...
include/core/SkPixelRef.h:260: // default impl returns 0.
Lets change the dox to document what this does/returns.
Is it the safe-size or the "normal" size? Is it useful/well-defined w/o
rowbytes?

[sugoi, 2013-12-03 19:07:22]

https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h...
include/core/SkPixelRef.h:260: // default impl returns 0.
On 2013/12/03 19:02:50, reed1 wrote:
> Lets change the dox to document what this does/returns.
> Is it the safe-size or the "normal" size? Is it useful/well-defined w/o
rowbytes?

I'll add the dox. It should return the allocated size in memory of the current
SkPixelRef's internal data, such that offset + safe-size should never overflow
that amount of allocated memory (If I understand things correctly).

[sugoi, 2013-12-03 19:18:06]

https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h...
include/core/SkPixelRef.h:260: // default impl returns 0.
On 2013/12/03 19:02:50, reed1 wrote:
> Lets change the dox to document what this does/returns.
> Is it the safe-size or the "normal" size? Is it useful/well-defined w/o
> rowbytes?

Done.

[reed1, 2013-12-03 19:29:08]

On 2013/12/03 19:07:22, sugoi wrote:
> https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h
> File include/core/SkPixelRef.h (right):
> 
>
https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h...
> include/core/SkPixelRef.h:260: // default impl returns 0.
> On 2013/12/03 19:02:50, reed1 wrote:
> > Lets change the dox to document what this does/returns.
> > Is it the safe-size or the "normal" size? Is it useful/well-defined w/o
> rowbytes?
> 
> I'll add the dox. It should return the allocated size in memory of the current
> SkPixelRef's internal data, such that offset + safe-size should never overflow
> that amount of allocated memory (If I understand things correctly).

What if a pixelref itself was subsetting another pixelref. Now the size and
safesize could be two different values. What does getSize() need to return? Is
returning rowBytes + height logically the same?

[sugoi, 2013-12-03 19:36:58]

On 2013/12/03 19:29:08, reed1 wrote:
> On 2013/12/03 19:07:22, sugoi wrote:
> >
https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h
> > File include/core/SkPixelRef.h (right):
> > 
> >
>
https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h...
> > include/core/SkPixelRef.h:260: // default impl returns 0.
> > On 2013/12/03 19:02:50, reed1 wrote:
> > > Lets change the dox to document what this does/returns.
> > > Is it the safe-size or the "normal" size? Is it useful/well-defined w/o
> > rowbytes?
> > 
> > I'll add the dox. It should return the allocated size in memory of the
current
> > SkPixelRef's internal data, such that offset + safe-size should never
overflow
> > that amount of allocated memory (If I understand things correctly).
> 
> What if a pixelref itself was subsetting another pixelref. Now the size and
> safesize could be two different values. What does getSize() need to return? Is
> returning rowBytes + height logically the same?

AFAIK, if a pixelref is subsetting another pixelref, it will use the offset
(SkBitmap::fPixelRefOffset) to set its starting point and will set rowBytes
accordingly to skip the proper amount of bytes between lines read. Looking at
SkBitmap's "safe size" :
SkBitmap::getSafeSize() { return (fHeight ? ((fHeight - 1) * fRowBytes) +
ComputeRowBytes(this->config(), fWidth): 0); }
the safe size does include fRowBytes and is not limited to the amount of memory
used by the subset, so this should work, I think.

[reed1, 2013-12-03 19:55:38]

On 2013/12/03 19:36:58, sugoi wrote:
> On 2013/12/03 19:29:08, reed1 wrote:
> > On 2013/12/03 19:07:22, sugoi wrote:
> > >
> https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h
> > > File include/core/SkPixelRef.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h...
> > > include/core/SkPixelRef.h:260: // default impl returns 0.
> > > On 2013/12/03 19:02:50, reed1 wrote:
> > > > Lets change the dox to document what this does/returns.
> > > > Is it the safe-size or the "normal" size? Is it useful/well-defined w/o
> > > rowbytes?
> > > 
> > > I'll add the dox. It should return the allocated size in memory of the
> current
> > > SkPixelRef's internal data, such that offset + safe-size should never
> overflow
> > > that amount of allocated memory (If I understand things correctly).
> > 
> > What if a pixelref itself was subsetting another pixelref. Now the size and
> > safesize could be two different values. What does getSize() need to return?
Is
> > returning rowBytes + height logically the same?
> 
> AFAIK, if a pixelref is subsetting another pixelref, it will use the offset
> (SkBitmap::fPixelRefOffset) to set its starting point and will set rowBytes
> accordingly to skip the proper amount of bytes between lines read. Looking at
> SkBitmap's "safe size" :
> SkBitmap::getSafeSize() { return (fHeight ? ((fHeight - 1) * fRowBytes) +
> ComputeRowBytes(this->config(), fWidth): 0); }
> the safe size does include fRowBytes and is not limited to the amount of
memory
> used by the subset, so this should work, I think.

Let me try to rephrase my question:

Does you getSize() need to return

height * rowBytes

or

(height - 1) * rowBytes + width * pixelSize

?

[sugoi, 2013-12-03 20:26:43]

On 2013/12/03 19:55:38, reed1 wrote:
> On 2013/12/03 19:36:58, sugoi wrote:
> > On 2013/12/03 19:29:08, reed1 wrote:
> > > On 2013/12/03 19:07:22, sugoi wrote:
> > > >
> >
https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h
> > > > File include/core/SkPixelRef.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h...
> > > > include/core/SkPixelRef.h:260: // default impl returns 0.
> > > > On 2013/12/03 19:02:50, reed1 wrote:
> > > > > Lets change the dox to document what this does/returns.
> > > > > Is it the safe-size or the "normal" size? Is it useful/well-defined
w/o
> > > > rowbytes?
> > > > 
> > > > I'll add the dox. It should return the allocated size in memory of the
> > current
> > > > SkPixelRef's internal data, such that offset + safe-size should never
> > overflow
> > > > that amount of allocated memory (If I understand things correctly).
> > > 
> > > What if a pixelref itself was subsetting another pixelref. Now the size
and
> > > safesize could be two different values. What does getSize() need to
return?
> Is
> > > returning rowBytes + height logically the same?
> > 
> > AFAIK, if a pixelref is subsetting another pixelref, it will use the offset
> > (SkBitmap::fPixelRefOffset) to set its starting point and will set rowBytes
> > accordingly to skip the proper amount of bytes between lines read. Looking
at
> > SkBitmap's "safe size" :
> > SkBitmap::getSafeSize() { return (fHeight ? ((fHeight - 1) * fRowBytes) +
> > ComputeRowBytes(this->config(), fWidth): 0); }
> > the safe size does include fRowBytes and is not limited to the amount of
> memory
> > used by the subset, so this should work, I think.
> 
> Let me try to rephrase my question:
> 
> Does you getSize() need to return
> 
> height * rowBytes
> 
> or
> 
> (height - 1) * rowBytes + width * pixelSize
> 
> ?

I guess it's closer to "(height - 1) * rowBytes + width * pixelSize", but
getSize() can't really be expressed in those terms. It should return the real
amount of memory allocated so that we can make sure that "(height - 1) *
rowBytes + width * pixelSize" fits within it. I'm expecting a real memory
allocation size from getSize().

[reed1, 2013-12-03 20:47:01]

On 2013/12/03 20:26:43, sugoi wrote:
> On 2013/12/03 19:55:38, reed1 wrote:
> > On 2013/12/03 19:36:58, sugoi wrote:
> > > On 2013/12/03 19:29:08, reed1 wrote:
> > > > On 2013/12/03 19:07:22, sugoi wrote:
> > > > >
> > >
> https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h
> > > > > File include/core/SkPixelRef.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h...
> > > > > include/core/SkPixelRef.h:260: // default impl returns 0.
> > > > > On 2013/12/03 19:02:50, reed1 wrote:
> > > > > > Lets change the dox to document what this does/returns.
> > > > > > Is it the safe-size or the "normal" size? Is it useful/well-defined
> w/o
> > > > > rowbytes?
> > > > > 
> > > > > I'll add the dox. It should return the allocated size in memory of the
> > > current
> > > > > SkPixelRef's internal data, such that offset + safe-size should never
> > > overflow
> > > > > that amount of allocated memory (If I understand things correctly).
> > > > 
> > > > What if a pixelref itself was subsetting another pixelref. Now the size
> and
> > > > safesize could be two different values. What does getSize() need to
> return?
> > Is
> > > > returning rowBytes + height logically the same?
> > > 
> > > AFAIK, if a pixelref is subsetting another pixelref, it will use the
offset
> > > (SkBitmap::fPixelRefOffset) to set its starting point and will set
rowBytes
> > > accordingly to skip the proper amount of bytes between lines read. Looking
> at
> > > SkBitmap's "safe size" :
> > > SkBitmap::getSafeSize() { return (fHeight ? ((fHeight - 1) * fRowBytes) +
> > > ComputeRowBytes(this->config(), fWidth): 0); }
> > > the safe size does include fRowBytes and is not limited to the amount of
> > memory
> > > used by the subset, so this should work, I think.
> > 
> > Let me try to rephrase my question:
> > 
> > Does you getSize() need to return
> > 
> > height * rowBytes
> > 
> > or
> > 
> > (height - 1) * rowBytes + width * pixelSize
> > 
> > ?
> 
> I guess it's closer to "(height - 1) * rowBytes + width * pixelSize", but
> getSize() can't really be expressed in those terms. It should return the real
> amount of memory allocated so that we can make sure that "(height - 1) *
> rowBytes + width * pixelSize" fits within it. I'm expecting a real memory
> allocation size from getSize().

Why? What is more correct about the real memory size (which the pixelref might
not know), than the amount it is allowed to reference?

[sugoi, 2013-12-03 21:11:39]

On 2013/12/03 20:47:01, reed1 wrote:
> On 2013/12/03 20:26:43, sugoi wrote:
> > On 2013/12/03 19:55:38, reed1 wrote:
> > > On 2013/12/03 19:36:58, sugoi wrote:
> > > > On 2013/12/03 19:29:08, reed1 wrote:
> > > > > On 2013/12/03 19:07:22, sugoi wrote:
> > > > > >
> > > >
> >
https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h
> > > > > > File include/core/SkPixelRef.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h...
> > > > > > include/core/SkPixelRef.h:260: // default impl returns 0.
> > > > > > On 2013/12/03 19:02:50, reed1 wrote:
> > > > > > > Lets change the dox to document what this does/returns.
> > > > > > > Is it the safe-size or the "normal" size? Is it
useful/well-defined
> > w/o
> > > > > > rowbytes?
> > > > > > 
> > > > > > I'll add the dox. It should return the allocated size in memory of
the
> > > > current
> > > > > > SkPixelRef's internal data, such that offset + safe-size should
never
> > > > overflow
> > > > > > that amount of allocated memory (If I understand things correctly).
> > > > > 
> > > > > What if a pixelref itself was subsetting another pixelref. Now the
size
> > and
> > > > > safesize could be two different values. What does getSize() need to
> > return?
> > > Is
> > > > > returning rowBytes + height logically the same?
> > > > 
> > > > AFAIK, if a pixelref is subsetting another pixelref, it will use the
> offset
> > > > (SkBitmap::fPixelRefOffset) to set its starting point and will set
> rowBytes
> > > > accordingly to skip the proper amount of bytes between lines read.
Looking
> > at
> > > > SkBitmap's "safe size" :
> > > > SkBitmap::getSafeSize() { return (fHeight ? ((fHeight - 1) * fRowBytes)
+
> > > > ComputeRowBytes(this->config(), fWidth): 0); }
> > > > the safe size does include fRowBytes and is not limited to the amount of
> > > memory
> > > > used by the subset, so this should work, I think.
> > > 
> > > Let me try to rephrase my question:
> > > 
> > > Does you getSize() need to return
> > > 
> > > height * rowBytes
> > > 
> > > or
> > > 
> > > (height - 1) * rowBytes + width * pixelSize
> > > 
> > > ?
> > 
> > I guess it's closer to "(height - 1) * rowBytes + width * pixelSize", but
> > getSize() can't really be expressed in those terms. It should return the
real
> > amount of memory allocated so that we can make sure that "(height - 1) *
> > rowBytes + width * pixelSize" fits within it. I'm expecting a real memory
> > allocation size from getSize().
> 
> Why? What is more correct about the real memory size (which the pixelref might
> not know), than the amount it is allowed to reference?

There's nothing wrong with using the amount that is allowed to reference, as
long as it is guaranteed that the deserialization process cannot, in any way,
produce a pixelref that has less "real" memory allocated than the amount it
shows as "available" through its API. Currently (before your changes), there's
nothing making sure that these amounts (allocated VS available) are always the
same.

[reed1, 2013-12-03 22:15:19]

On 2013/12/03 21:11:39, sugoi wrote:
> On 2013/12/03 20:47:01, reed1 wrote:
> > On 2013/12/03 20:26:43, sugoi wrote:
> > > On 2013/12/03 19:55:38, reed1 wrote:
> > > > On 2013/12/03 19:36:58, sugoi wrote:
> > > > > On 2013/12/03 19:29:08, reed1 wrote:
> > > > > > On 2013/12/03 19:07:22, sugoi wrote:
> > > > > > >
> > > > >
> > >
> https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h
> > > > > > > File include/core/SkPixelRef.h (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h...
> > > > > > > include/core/SkPixelRef.h:260: // default impl returns 0.
> > > > > > > On 2013/12/03 19:02:50, reed1 wrote:
> > > > > > > > Lets change the dox to document what this does/returns.
> > > > > > > > Is it the safe-size or the "normal" size? Is it
> useful/well-defined
> > > w/o
> > > > > > > rowbytes?
> > > > > > > 
> > > > > > > I'll add the dox. It should return the allocated size in memory of
> the
> > > > > current
> > > > > > > SkPixelRef's internal data, such that offset + safe-size should
> never
> > > > > overflow
> > > > > > > that amount of allocated memory (If I understand things
correctly).
> > > > > > 
> > > > > > What if a pixelref itself was subsetting another pixelref. Now the
> size
> > > and
> > > > > > safesize could be two different values. What does getSize() need to
> > > return?
> > > > Is
> > > > > > returning rowBytes + height logically the same?
> > > > > 
> > > > > AFAIK, if a pixelref is subsetting another pixelref, it will use the
> > offset
> > > > > (SkBitmap::fPixelRefOffset) to set its starting point and will set
> > rowBytes
> > > > > accordingly to skip the proper amount of bytes between lines read.
> Looking
> > > at
> > > > > SkBitmap's "safe size" :
> > > > > SkBitmap::getSafeSize() { return (fHeight ? ((fHeight - 1) *
fRowBytes)
> +
> > > > > ComputeRowBytes(this->config(), fWidth): 0); }
> > > > > the safe size does include fRowBytes and is not limited to the amount
of
> > > > memory
> > > > > used by the subset, so this should work, I think.
> > > > 
> > > > Let me try to rephrase my question:
> > > > 
> > > > Does you getSize() need to return
> > > > 
> > > > height * rowBytes
> > > > 
> > > > or
> > > > 
> > > > (height - 1) * rowBytes + width * pixelSize
> > > > 
> > > > ?
> > > 
> > > I guess it's closer to "(height - 1) * rowBytes + width * pixelSize", but
> > > getSize() can't really be expressed in those terms. It should return the
> real
> > > amount of memory allocated so that we can make sure that "(height - 1) *
> > > rowBytes + width * pixelSize" fits within it. I'm expecting a real memory
> > > allocation size from getSize().
> > 
> > Why? What is more correct about the real memory size (which the pixelref
might
> > not know), than the amount it is allowed to reference?
> 
> There's nothing wrong with using the amount that is allowed to reference, as
> long as it is guaranteed that the deserialization process cannot, in any way,
> produce a pixelref that has less "real" memory allocated than the amount it
> shows as "available" through its API. Currently (before your changes), there's
> nothing making sure that these amounts (allocated VS available) are always the
> same.

I'm lost. Lets have a VC.

[sugoi, 2013-12-04 04:52:52]

On 2013/12/03 22:15:19, reed1 wrote:
> On 2013/12/03 21:11:39, sugoi wrote:
> > On 2013/12/03 20:47:01, reed1 wrote:
> > > On 2013/12/03 20:26:43, sugoi wrote:
> > > > On 2013/12/03 19:55:38, reed1 wrote:
> > > > > On 2013/12/03 19:36:58, sugoi wrote:
> > > > > > On 2013/12/03 19:29:08, reed1 wrote:
> > > > > > > On 2013/12/03 19:07:22, sugoi wrote:
> > > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h
> > > > > > > > File include/core/SkPixelRef.h (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/92793002/diff/40001/include/core/SkPixelRef.h...
> > > > > > > > include/core/SkPixelRef.h:260: // default impl returns 0.
> > > > > > > > On 2013/12/03 19:02:50, reed1 wrote:
> > > > > > > > > Lets change the dox to document what this does/returns.
> > > > > > > > > Is it the safe-size or the "normal" size? Is it
> > useful/well-defined
> > > > w/o
> > > > > > > > rowbytes?
> > > > > > > > 
> > > > > > > > I'll add the dox. It should return the allocated size in memory
of
> > the
> > > > > > current
> > > > > > > > SkPixelRef's internal data, such that offset + safe-size should
> > never
> > > > > > overflow
> > > > > > > > that amount of allocated memory (If I understand things
> correctly).
> > > > > > > 
> > > > > > > What if a pixelref itself was subsetting another pixelref. Now the
> > size
> > > > and
> > > > > > > safesize could be two different values. What does getSize() need
to
> > > > return?
> > > > > Is
> > > > > > > returning rowBytes + height logically the same?
> > > > > > 
> > > > > > AFAIK, if a pixelref is subsetting another pixelref, it will use the
> > > offset
> > > > > > (SkBitmap::fPixelRefOffset) to set its starting point and will set
> > > rowBytes
> > > > > > accordingly to skip the proper amount of bytes between lines read.
> > Looking
> > > > at
> > > > > > SkBitmap's "safe size" :
> > > > > > SkBitmap::getSafeSize() { return (fHeight ? ((fHeight - 1) *
> fRowBytes)
> > +
> > > > > > ComputeRowBytes(this->config(), fWidth): 0); }
> > > > > > the safe size does include fRowBytes and is not limited to the
amount
> of
> > > > > memory
> > > > > > used by the subset, so this should work, I think.
> > > > > 
> > > > > Let me try to rephrase my question:
> > > > > 
> > > > > Does you getSize() need to return
> > > > > 
> > > > > height * rowBytes
> > > > > 
> > > > > or
> > > > > 
> > > > > (height - 1) * rowBytes + width * pixelSize
> > > > > 
> > > > > ?
> > > > 
> > > > I guess it's closer to "(height - 1) * rowBytes + width * pixelSize",
but
> > > > getSize() can't really be expressed in those terms. It should return the
> > real
> > > > amount of memory allocated so that we can make sure that "(height - 1) *
> > > > rowBytes + width * pixelSize" fits within it. I'm expecting a real
memory
> > > > allocation size from getSize().
> > > 
> > > Why? What is more correct about the real memory size (which the pixelref
> might
> > > not know), than the amount it is allowed to reference?
> > 
> > There's nothing wrong with using the amount that is allowed to reference, as
> > long as it is guaranteed that the deserialization process cannot, in any
way,
> > produce a pixelref that has less "real" memory allocated than the amount it
> > shows as "available" through its API. Currently (before your changes),
there's
> > nothing making sure that these amounts (allocated VS available) are always
the
> > same.
> 
> I'm lost. Lets have a VC.

Ok. But let me try one more time, because this is simple and I must be
explaining it wrong.
When we deserialize an SkBitmap, we deserialize its parameters (width, height,
rowBytes, etc) and we also deserialize an SkPixelRef object, which is a member
of SkBitmap. Right now, there nothing preventing SkBitmap's parameters and
SkPixelRef's data from being completely out of sync. For example, an SkBitmap
could have a width of 256, a height of 256, and 4 bytes per pixel (and let's
assume 1024 bytes per row), which means that this SkBimap requires 256k of
memory (assuming it's in RAM and not a texture) and nothing prevents the
SkPixelRef member here to only contain 128k of data (assume a compromised read
buffer stream here), which would mean that, at rendering time, half the pixels
would be read from out-of-bounds memory accesses. Note that I can assume that
SkPixelRef's data is in RAM while deserializing, because only SkMallocPixelRef
and SkDataPixelRef are serializable, so we don't serialize other classes derived
from SkPixelRef.

So, the general idea is : I need something, anything, that fully guarantees that
the parameters read (width, height, rowBytes, etc) cannot in any way cause
out-of-bounds memory accesses, or, in other words, that there is enough memory
allocated in the SkPixelRef object to contain all the pixels of its associated
SkBitmap object. Whether this is done through SkPixelRef::getSize() or through
any other means does not matter to me, as long as there's a way to know for sure
that a compromised read buffer stream cannot cause out-of-bounds memory accesses
later on, during rendering, for example.

[sugoi, 2013-12-04 16:20:31]

https://codereview.chromium.org/92793002/diff/100001/include/core/SkMallocPix...
File include/core/SkMallocPixelRef.h (right):

https://codereview.chromium.org/92793002/diff/100001/include/core/SkMallocPix...
include/core/SkMallocPixelRef.h:40: virtual size_t getAllocatedSizeInBytes()
const SK_OVERRIDE { return fSize; }
I moved and renamed this function and it's still compiling, so I'm fairly
certain that the original one was unused (I double checked in chromium code
search, and there's no call site for this SkMallocPixelRef::getSize).

[reed1, 2013-12-04 16:36:49]

lgtm

[commit-bot: I haz the power, 2013-12-04 16:39:42]

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

[commit-bot: I haz the power, 2013-12-04 17:06:58]

Change committed as 12484