Remove size check from SkCodec

Remove size check from SkCodec

This check is useless because it is vulnerable to integer overflow.

Also, I don't think this is the right way to solve the problem
of "too large" images.  For example, many image specs allow images
larger than this, so we should too.

BUG=skia:4667
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1580113002

Committed: https://skia.googlesource.com/skia/+/f44631b133909bbf98c13e53ec11bf8cd87a2dc4

[msarett, 2016-01-12 19:59:49]

Description was changed from

==========
Remove size check from SkCodec

This check is useless because it is vulernable to integer overflow.

Also, I don't think this is the right way to solve the problem
of "too large" images.  For example, many image specs allow images
larger than this, so we should too.

I will follow up with a new approach to this problem.

BUG=skia:4667
==========

to

==========
Remove size check from SkCodec

This check is useless because it is vulernable to integer overflow.

Also, I don't think this is the right way to solve the problem
of "too large" images.  For example, many image specs allow images
larger than this, so we should too.

I will follow up with a new approach to this problem.

BUG=skia:4667
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-01-12 20:01:36]

msarett@google.com changed reviewers:
+ mtklein@google.com, scroggo@google.com

[scroggo, 2016-01-12 22:24:20]

> Remove size check from SkCodec
>
> This check is useless because it is vulernable to integer overflow.

nit: vulnerable

> Also, I don't think this is the right way to solve the problem
> of "too large" images.  For example, many image specs allow images
> larger than this, so we should too.
>
> I will follow up with a new approach to this problem.

What is the follow up approach?

https://codereview.chromium.org/1580113002/diff/20001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1580113002/diff/20001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:93: return codec.detach();
This seems a little strange until the SkRawCodec code goes in (i.e. we could
just return proc.NewFromStream. OTOH, some people prefer a single return
statement...)

[msarett, 2016-01-12 22:31:40]

Description was changed from

==========
Remove size check from SkCodec

This check is useless because it is vulernable to integer overflow.

Also, I don't think this is the right way to solve the problem
of "too large" images.  For example, many image specs allow images
larger than this, so we should too.

I will follow up with a new approach to this problem.

BUG=skia:4667
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Remove size check from SkCodec

This check is useless because it is vulernable to integer overflow.

Also, I don't think this is the right way to solve the problem
of "too large" images.  For example, many image specs allow images
larger than this, so we should too.

BUG=skia:4667
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-01-13 16:28:38]

Description was changed from

==========
Remove size check from SkCodec

This check is useless because it is vulernable to integer overflow.

Also, I don't think this is the right way to solve the problem
of "too large" images.  For example, many image specs allow images
larger than this, so we should too.

BUG=skia:4667
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Remove size check from SkCodec

This check is useless because it is vulnerable to integer overflow.

Also, I don't think this is the right way to solve the problem
of "too large" images.  For example, many image specs allow images
larger than this, so we should too.

BUG=skia:4667
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-01-13 16:34:34]

Patchset #3 (id:40001) has been deleted

[msarett, 2016-01-13 16:38:58]

> > I will follow up with a new approach to this problem.

> What is the follow up approach?

FWIW, I've removed this line from the commit message.

I *think* the follow-up approach might be to not do any
size checking (and rely on the client to check).  The
client is the one allocating memory for the decode anyway.
So no follow-up at all :).

However, there are still several opened bugs about this,
and I don't feel completely sure that we shouldn't do
something.
https://buganizer.corp.google.com/u/0/issues/26505891
https://bugs.chromium.org/p/skia/issues/detail?id=3617

You could argue that we should wait until these are
resolved?  But I'd like to go ahead and fix the dumb
overflow bug - and I prefer this fix for now.

https://codereview.chromium.org/1580113002/diff/20001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1580113002/diff/20001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:93: return codec.detach();
On 2016/01/12 22:24:20, scroggo wrote:
> This seems a little strange until the SkRawCodec code goes in (i.e. we could
> just return proc.NewFromStream. OTOH, some people prefer a single return
> statement...)

You're right.  I've fixed this.

[scroggo, 2016-01-13 18:35:43]

lgtm

On 2016/01/13 16:38:58, msarett wrote:
> > > I will follow up with a new approach to this problem.
> 
> > What is the follow up approach?
> 
> FWIW, I've removed this line from the commit message.
> 
> I *think* the follow-up approach might be to not do any
> size checking (and rely on the client to check).  The
> client is the one allocating memory for the decode anyway.
> So no follow-up at all :).
> 
> However, there are still several opened bugs about this,
> and I don't feel completely sure that we shouldn't do
> something.
> https://buganizer.corp.google.com/u/0/issues/26505891

I suspect this is a problem in the RAW code.

> https://bugs.chromium.org/p/skia/issues/detail?id=3617

IIUC, this may run out of memory to allocate the output bitmap, but a client can
make that decision for themselves when they create the SkCodec and check its
dimensions. They could also use scanline decoding/sampling if they don't want
the full size. I think it's fine to just remove the check.

https://codereview.chromium.org/1580113002/diff/20001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1580113002/diff/20001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:93: return codec.detach();
On 2016/01/13 16:38:58, msarett wrote:
> On 2016/01/12 22:24:20, scroggo wrote:
> > This seems a little strange until the SkRawCodec code goes in (i.e. we could
> > just return proc.NewFromStream. OTOH, some people prefer a single return
> > statement...)
> 
> You're right.  I've fixed this.

It probably would've been fine to leave it. This change will make them have to
undo it, but this is more logical for now. I'm fine either way.

[msarett, 2016-01-13 18:37:24]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-01-13 18:37:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1580113002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1580113002/60001

[commit-bot: I haz the power, 2016-01-13 18:54:22]

Description was changed from

==========
Remove size check from SkCodec

This check is useless because it is vulnerable to integer overflow.

Also, I don't think this is the right way to solve the problem
of "too large" images.  For example, many image specs allow images
larger than this, so we should too.

BUG=skia:4667
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Remove size check from SkCodec

This check is useless because it is vulnerable to integer overflow.

Also, I don't think this is the right way to solve the problem
of "too large" images.  For example, many image specs allow images
larger than this, so we should too.

BUG=skia:4667
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/f44631b133909bbf98c13e53ec11bf8cd87a2dc4
==========

[commit-bot: I haz the power, 2016-01-13 18:54:23]

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