Verify size_t overflow

Verify size_t overflow

In 32 bits, it's possible that multiplying 2 32b values might overflow a size_t, which could be 32b unsigned in that context, so I added a check for size_t overflow.

BUG=445831
Committed: https://skia.googlesource.com/skia/+/bd0d9da981289504c9dcd8547eaad52b1f4a52cb

[sugoi1, 2015-01-06 22:05:10]

sugoi@chromium.org changed reviewers:
+ reed@google.com

[reed1, 2015-01-06 22:29:35]

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

https://codereview.chromium.org/836733005/diff/1/src/core/SkBitmap.cpp#newcod...
src/core/SkBitmap.cpp:1208: if (!buffer->validate((snugSize <= ramSize) &&
((ramSize / ramRB) == height))) {
perhaps we can explicitly use int64_t. We have handy macros to promote 32bit
args to make the multiply correct.

e.g.

int64_t snugSize = sk_64_mul(snugRB, height);
int64_t ramSize = sk_64_mul(ramRB, height);
...

When you explicitly need the value in 32bits, you can use

if (sk_64_isS32(ramSize))
   ...

[sugoi1, 2015-01-07 16:09:04]

Wrote a new version using sk_64_mul

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

https://codereview.chromium.org/836733005/diff/20001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1205: const int height = SkMax32(info.height(), 0);
height can apparently be negative and valid, which just means "empty", so I have
to prevent negative numbers here to avoid the rest of the math being wrong since
it will use int64_t to multiply, which is a signed integer type

https://codereview.chromium.org/836733005/diff/20001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1213: SkAutoDataUnref
data(SkData::NewUninitialized(static_cast<size_t>(ramSize)));
This cast and the following one should do nothing on 64b arch, but should
prevent warnings when compiling on 32b arch.

[reed1, 2015-01-07 16:19:35]

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

https://codereview.chromium.org/836733005/diff/20001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1213: SkAutoDataUnref
data(SkData::NewUninitialized(static_cast<size_t>(ramSize)));
On 2015/01/07 16:09:04, sugoi1 wrote:
> This cast and the following one should do nothing on 64b arch, but should
> prevent warnings when compiling on 32b arch.

SkToSizeT(ramSize) -- this guy checks for overflow in debug builds, and performs
the cast.

[sugoi1, 2015-01-07 16:28:35]

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

https://codereview.chromium.org/836733005/diff/20001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1213: SkAutoDataUnref
data(SkData::NewUninitialized(static_cast<size_t>(ramSize)));
On 2015/01/07 16:19:35, reed1 wrote:
> On 2015/01/07 16:09:04, sugoi1 wrote:
> > This cast and the following one should do nothing on 64b arch, but should
> > prevent warnings when compiling on 32b arch.
> 
> SkToSizeT(ramSize) -- this guy checks for overflow in debug builds, and
performs
> the cast.

Done.

[reed1, 2015-01-07 16:34:25]

lgtm

[sugoi1, 2015-01-07 16:37:19]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2015-01-07 16:38:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/836733005/40001

[commit-bot: I haz the power, 2015-01-07 16:47:47]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/bd0d9da981289504c9dcd8547eaad52b1f4a52cb

[Stephen White, 2015-01-07 19:22:28]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2015-01-07 19:22:29]

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

https://codereview.chromium.org/836733005/diff/40001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1209: if (!buffer->validate((snugSize <= ramSize) &&
(ramSize <= max_size_t))) {
static const uint64_t max_size_t = (size_t)(-1);
if (!buffer->validate((snugSize <= ramSize) && (ramSize <= max_size_t))) {

Nit: I think this could be slightly shorter as:

if (!buffer->validate((snugSize <= ramSize) && (ramSize == (size_t) ramSize))) {

I leave it up to you guys to say if it's clearer or not.