Fix a memory leak when decoding corrupted indexed PNGs.

Fix a memory leak when decoding corrupted indexed PNGs.
Commit to branch refs/heads/png-leak

BUG=skia:3457
Committed: https://skia.googlesource.com/skia/+/561a1ca9559a1ea7589ab93350124284fcef3315

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

[David Lattimore, 2015-02-23 22:15:45]

dml@google.com changed reviewers:
+ scroggo@google.com

[scroggo, 2015-03-05 22:56:48]

LGTM

Thanks for uploading the patch, and sorry for the delay!

Reason for the delay:
I didn't receive an email because you apparently did not click "Publish+Mail
Comments".  When you edit the issue and enter a reviewer, it unfortunately does
not send them an email :(

https://codereview.chromium.org/951663002/diff/1/tests/InvalidIndexedPngTest.cpp
File tests/InvalidIndexedPngTest.cpp (right):

https://codereview.chromium.org/951663002/diff/1/tests/InvalidIndexedPngTest....
tests/InvalidIndexedPngTest.cpp:2: * Copyright 2011 Google Inc.
2015*

https://codereview.chromium.org/951663002/diff/1/tests/InvalidIndexedPngTest....
tests/InvalidIndexedPngTest.cpp:14: unsigned char a_png[] = {
nit: we typically label globals as gSomething.

[David Lattimore, 2015-03-05 23:21:06]

Sorry about now clicking publish+mail - I see that is mentioned in the
instructions. Guess I should have read them more carefully. Thanks for noticing
the CL.

I was having trouble building for some reason, so I synced which resulted in it
showing changes to gyp/tests.gypi relative to my last patch.

https://codereview.chromium.org/951663002/diff/1/tests/InvalidIndexedPngTest.cpp
File tests/InvalidIndexedPngTest.cpp (right):

https://codereview.chromium.org/951663002/diff/1/tests/InvalidIndexedPngTest....
tests/InvalidIndexedPngTest.cpp:2: * Copyright 2011 Google Inc.
On 2015/03/05 22:56:48, scroggo wrote:
> 2015*

Done.

https://codereview.chromium.org/951663002/diff/1/tests/InvalidIndexedPngTest....
tests/InvalidIndexedPngTest.cpp:14: unsigned char a_png[] = {
On 2015/03/05 22:56:48, scroggo wrote:
> nit: we typically label globals as gSomething.

Done.

[scroggo, 2015-03-05 23:26:10]

On 2015/03/05 23:21:06, David Lattimore wrote:
> Sorry about now clicking publish+mail - I see that is mentioned in the
> instructions. Guess I should have read them more carefully. Thanks for
noticing
> the CL.

No worries! I knew the problem because it's happened plenty before :)

LGTM

[David Lattimore, 2015-03-06 00:12:50]

After syncing, the test was failing. It seems that indexed PNGs with a size-0
palette are no longer considered valid (fair enough). So I created a new PNG
with a palette size of 2 and put that in. I confirmed that the buffer overread
still happened without my fix.

[David Lattimore, 2015-03-06 00:13:57]

Sorry, that comment was on the wrong CL. Please disregard.

[scroggo, 2015-03-06 19:48:46]

On 2015/03/06 00:13:57, David Lattimore wrote:
> Sorry, that comment was on the wrong CL. Please disregard.

Sounds good. I'll go ahead and click the 'Commit' check box, and then it will
merge in automatically.

[scroggo, 2015-03-06 19:48:54]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-03-06 19:49:32]

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

[scroggo, 2015-03-06 19:49:35]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-03-06 19:49:41]

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

[commit-bot: I haz the power, 2015-03-07 00:25:31]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/561a1ca9559a1ea7589ab93350124284fcef3315

[reed2, 2015-03-07 14:05:11]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/980203005/ by reed@chromium.org.

The reason for reverting is: speculative revert due to many failures in tree.

[David Lattimore, 2015-03-09 01:44:27]

Sorry about the breakage. Does commit-bot only runs the tests on Linux before
submitting?

It seems that some platforms don't have PNG decoding. Even more interesting is
that on these platforms, decoding a PNG always reports success (tested on a
Mac). I've uploaded a new patch set that disables the test for platforms that
don't have PNG decoding.

[scroggo, 2015-03-11 13:29:06]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-03-11 13:29:07]

lgtm

On 2015/03/09 01:44:27, David Lattimore wrote:
> Sorry about the breakage. Does commit-bot only runs the tests on Linux before
> submitting?

It happens :) IIRC, commit-bot only tests compilation, and does not attempt to
run any tests before committing.

> 
> It seems that some platforms don't have PNG decoding.

That's correct. SkImageDecoder is primarily used by Android. The other platform
versions are basically there to help us in our testing. For simplicity, we did
not use libpng on those platforms. That said, with our next version of decoding,
SkCodec, we are using the same version everywhere.

> Even more interesting is
> that on these platforms, decoding a PNG always reports success (tested on a
> Mac).

That's a bug. Just the sort of thing that happens when we have different
versions on different platforms, and focus on one platform :(

> I've uploaded a new patch set that disables the test for platforms that
> don't have PNG decoding.

That seems like the best fix, given that we are replacing SkImageDecoder.

I've reopened the issue, and marked for commit (though it won't actually be
committed until the tree goes back to green - currently it's set to "Caution").

https://codereview.chromium.org/951663002/diff/40001/tests/InvalidIndexedPngT...
File tests/InvalidIndexedPngTest.cpp (right):

https://codereview.chromium.org/951663002/diff/40001/tests/InvalidIndexedPngT...
tests/InvalidIndexedPngTest.cpp:8: // The PNG decoder is not available on all
platforms.
Nit: There is a PNG decoder on all platforms, it just isn't always using libpng.
On Windows, see SkImageDecoder_WIC.cpp, and on Mac, see SkImageDecoder_CG.cpp
(both are in ports).

[commit-bot: I haz the power, 2015-03-11 13:29:22]

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

[commit-bot: I haz the power, 2015-03-11 18:12:58]

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