Fix stack-use-after-scope in png_codec.

Fix stack-use-after-scope in png_codec.

PngWriteStructDestroyer accesses info_ptr, so
info_ptr must have a wider lifetime scope. Otherwise,
the behavior of the problem is undefined.

The issue is found by AddressSanitizer with use-after-scope check enabled.

BUG=649897
Committed: https://crrev.com/23478bc8eb2dcf7df4678359e8fb164a5317313f
Cr-Commit-Position: refs/heads/master@{#438954}

[krasin1, 2016-12-14 19:37:43]

krasin@chromium.org changed reviewers:
+ dcheng@chromium.org

[krasin1, 2016-12-14 19:37:48]

The CQ bit was checked by krasin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 19:38:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2016-12-14 20:11:29]

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

[Nico, 2016-12-14 20:11:29]

Nice!

https://codereview.chromium.org/2576823002/diff/1/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

https://codereview.chromium.org/2576823002/diff/1/ui/gfx/codec/png_codec.cc#n...
ui/gfx/codec/png_codec.cc:715: // info_ptr should have wider lifetime scope than
destroyer that access it
nit: i'd add a comma at the end of this line to remove an ambiguous parse from
this comment ;-)

Nice find, but PngWriteStructDestroyer is almost impossible to use correctly now
that we know this bug. Maybe its ctor should take a png_info** and
SetInfStruct() should be removed. Then it's impossible to use incorrectly. Does
that make sense?

[krasin1, 2016-12-14 20:56:59]

https://codereview.chromium.org/2576823002/diff/1/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

https://codereview.chromium.org/2576823002/diff/1/ui/gfx/codec/png_codec.cc#n...
ui/gfx/codec/png_codec.cc:715: // info_ptr should have wider lifetime scope than
destroyer that access it
On 2016/12/14 20:11:29, Nico wrote:
> nit: i'd add a comma at the end of this line to remove an ambiguous parse from
> this comment ;-)
Done.

> 
> Nice find, but PngWriteStructDestroyer is almost impossible to use correctly
now
> that we know this bug. Maybe its ctor should take a png_info** and
> SetInfStruct() should be removed. Then it's impossible to use incorrectly.
Does that make sense?

There are a number of similar classes around (other uses are correct). I agree
it's easy to make a mistake, and a better API might help.

Just doing a constructor won't help, because it would require ugly code to
handle the case when creating the first struct (png_ptr) succeeds, but handling
the second struct fails.

I feel that we can probably rely on asan + use-after-scope check to find cases.
At least, I don't have a good alternative suggestion for now.

[Nico, 2016-12-14 22:24:32]

On 2016/12/14 20:56:59, krasin1 wrote:
> https://codereview.chromium.org/2576823002/diff/1/ui/gfx/codec/png_codec.cc
> File ui/gfx/codec/png_codec.cc (right):
> 
>
https://codereview.chromium.org/2576823002/diff/1/ui/gfx/codec/png_codec.cc#n...
> ui/gfx/codec/png_codec.cc:715: // info_ptr should have wider lifetime scope
than
> destroyer that access it
> On 2016/12/14 20:11:29, Nico wrote:
> > nit: i'd add a comma at the end of this line to remove an ambiguous parse
from
> > this comment ;-)
> Done.
> 
> > 
> > Nice find, but PngWriteStructDestroyer is almost impossible to use correctly
> now
> > that we know this bug. Maybe its ctor should take a png_info** and
> > SetInfStruct() should be removed. Then it's impossible to use incorrectly.
> Does that make sense?
> 
> There are a number of similar classes around (other uses are correct). I agree
> it's easy to make a mistake, and a better API might help.
> 
> Just doing a constructor won't help, because it would require ugly code to
> handle the case when creating the first struct (png_ptr) succeeds, but
handling
> the second struct fails.

If you make png_info** a pointer to pointer, set *ptr to nullptr, and then check
if *ptr is set before freeing in the dtor, that isn't a problem, is it?

> 
> I feel that we can probably rely on asan + use-after-scope check to find
cases.
> At least, I don't have a good alternative suggestion for now.

[dcheng, 2016-12-14 22:32:47]

On 2016/12/14 22:24:32, Nico wrote:
> On 2016/12/14 20:56:59, krasin1 wrote:
> > https://codereview.chromium.org/2576823002/diff/1/ui/gfx/codec/png_codec.cc
> > File ui/gfx/codec/png_codec.cc (right):
> > 
> >
>
https://codereview.chromium.org/2576823002/diff/1/ui/gfx/codec/png_codec.cc#n...
> > ui/gfx/codec/png_codec.cc:715: // info_ptr should have wider lifetime scope
> than
> > destroyer that access it
> > On 2016/12/14 20:11:29, Nico wrote:
> > > nit: i'd add a comma at the end of this line to remove an ambiguous parse
> from
> > > this comment ;-)
> > Done.
> > 
> > > 
> > > Nice find, but PngWriteStructDestroyer is almost impossible to use
correctly
> > now
> > > that we know this bug. Maybe its ctor should take a png_info** and
> > > SetInfStruct() should be removed. Then it's impossible to use incorrectly.
> > Does that make sense?
> > 
> > There are a number of similar classes around (other uses are correct). I
agree
> > it's easy to make a mistake, and a better API might help.
> > 
> > Just doing a constructor won't help, because it would require ugly code to
> > handle the case when creating the first struct (png_ptr) succeeds, but
> handling
> > the second struct fails.
> 
> If you make png_info** a pointer to pointer, set *ptr to nullptr, and then
check
> if *ptr is set before freeing in the dtor, that isn't a problem, is it?
> 

I feel like I'm missing something here: we fail fast if *ptr is null, so that
shouldn't happen? Is there other code that would be susceptible to using this
incorrectly?

(I think the suggestion to make this a constructor parameter seems reasonable,
but I'm not entirely sure why we need the extra null check)

> > 
> > I feel that we can probably rely on asan + use-after-scope check to find
> cases.
> > At least, I don't have a good alternative suggestion for now.

https://codereview.chromium.org/2576823002/diff/20001/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

https://codereview.chromium.org/2576823002/diff/20001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:718: PngWriteStructDestroyer destroyer(&png_ptr);
Can we move this line down to after return false? Then we can combine
declaration + initialization of |info_ptr| (and we may as well just make
&info_ptr an argument to the ctor).

[krasin1, 2016-12-14 23:24:48]

The CQ bit was checked by krasin@chromium.org to run a CQ dry run

[krasin1, 2016-12-14 23:24:55]

The CQ bit was unchecked by krasin@chromium.org

[krasin1, 2016-12-14 23:45:04]

https://codereview.chromium.org/2576823002/diff/20001/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

https://codereview.chromium.org/2576823002/diff/20001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:718: PngWriteStructDestroyer destroyer(&png_ptr);
Daniel, Nico,

I have made a simplification that can be applied to other types of Png*Destroyer
classes too (and I am fine to do the upgrade in this CL, it you feel it's worth
it).

Basically, instead of making this delicate dance inline, I have added two fields
into PngWriteStructDestroyer (and also renamed it to PngWriteStructInfo, as it
now holds png_ptr and info_ptr), and we use the fields instead of local
variables.

Do you feel that's less fragile?

[Nico, 2016-12-14 23:47:52]

i like this, lgtm

https://codereview.chromium.org/2576823002/diff/40001/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

https://codereview.chromium.org/2576823002/diff/40001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:334: explicit PngWriteStructInfo() : png_str_(0),
info_ptr_(0) {
(minor nit: "str" means "string" for me, not struct, so I actually like ps_ a
bit more - png_struct_ is probably even better)

[dcheng, 2016-12-14 23:48:53]

LGTM as well.

https://codereview.chromium.org/2576823002/diff/40001/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

https://codereview.chromium.org/2576823002/diff/40001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:334: explicit PngWriteStructInfo() : png_str_(0),
info_ptr_(0) {
Minor nit: no explicit, and use nullptr

[krasin1, 2016-12-14 23:51:18]

On 2016/12/14 23:47:52, Nico wrote:
> i like this, lgtm
> 
>
https://codereview.chromium.org/2576823002/diff/40001/ui/gfx/codec/png_codec.cc
> File ui/gfx/codec/png_codec.cc (right):
> 
>
https://codereview.chromium.org/2576823002/diff/40001/ui/gfx/codec/png_codec....
> ui/gfx/codec/png_codec.cc:334: explicit PngWriteStructInfo() : png_str_(0),
> info_ptr_(0) {
> (minor nit: "str" means "string" for me, not struct, so I actually like ps_ a
> bit more - png_struct_ is probably even better)

Renamed to png_ptr_. This file already uses png_ptr as the argument name in a
few places, so it's now more consistent.

[krasin1, 2016-12-14 23:53:47]

Shall I also convert other Png*Destroyer classes to the new scheme? I feel like
doing this, as it will decrease the chance I ever come and fix another bug like
this.

https://codereview.chromium.org/2576823002/diff/40001/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

https://codereview.chromium.org/2576823002/diff/40001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:334: explicit PngWriteStructInfo() : png_str_(0),
info_ptr_(0) {
On 2016/12/14 23:48:52, dcheng wrote:
> Minor nit: no explicit, and use nullptr

Done.

[Nico, 2016-12-14 23:54:43]

lgtm++

[krasin1, 2016-12-15 00:40:23]

At this time, I satisfied with the change. One interesting insight that this
code has been copied 3 times. It's here, and also there:
https://cs.chromium.org/chromium/src/tools/imagediff/image_diff_png.cc?q=Png....
https://cs.chromium.org/chromium/src/third_party/pdfium/samples/image_diff_pn...

They don't have a PngWriteStructDestroyer and the bug in the question, so I
won't probably come after them.

[krasin1, 2016-12-15 00:40:37]

The CQ bit was checked by krasin@chromium.org

[krasin1, 2016-12-15 00:40:37]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2576823002/#ps160001
(title: "BuildPNGStruct -> PngReadStructInfo::Build")

[commit-bot: I haz the power, 2016-12-15 00:41:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 00:48:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 00:48:50]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[krasin1, 2016-12-15 01:01:08]

The CQ bit was checked by krasin@chromium.org

[commit-bot: I haz the power, 2016-12-15 01:02:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 01:09:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 01:09:19]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[krasin1, 2016-12-15 01:33:53]

The CQ bit was checked by krasin@chromium.org

[commit-bot: I haz the power, 2016-12-15 01:34:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 01:41:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 01:41:17]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[krasin1, 2016-12-15 01:42:04]

The CQ bit was checked by krasin@chromium.org

[commit-bot: I haz the power, 2016-12-15 01:42:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 01:46:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 01:46:51]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[krasin1, 2016-12-15 01:51:10]

The CQ bit was checked by krasin@chromium.org

[commit-bot: I haz the power, 2016-12-15 01:51:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 01:56:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 01:56:26]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[krasin1, 2016-12-15 01:58:12]

On 2016/12/15 01:56:26, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
>   win_clang on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

Something is wrong with the trybot. Possibly, it's the remainder from
https://crbug.com/674330. I will try again tomorrow.

[krasin1, 2016-12-15 19:34:39]

The CQ bit was checked by krasin@chromium.org

[commit-bot: I haz the power, 2016-12-15 19:35:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 21:13:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 21:13:09]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[krasin1, 2016-12-15 22:58:42]

The CQ bit was checked by krasin@chromium.org

[commit-bot: I haz the power, 2016-12-15 22:59:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 23:39:39]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481842722670140,
"parent_rev": "007ed41c5c205ff21f60911b4d31effa0e810a6b", "commit_rev":
"c56392b5d154d59b10ca6566f0e5dff1a5f9e61b"}

[commit-bot: I haz the power, 2016-12-15 23:40:29]

Description was changed from

==========
Fix stack-use-after-scope in png_codec.

PngWriteStructDestroyer accesses info_ptr, so
info_ptr must have a wider lifetime scope. Otherwise,
the behavior of the problem is undefined.

The issue is found by AddressSanitizer with use-after-scope check enabled.

BUG=649897
==========

to

==========
Fix stack-use-after-scope in png_codec.

PngWriteStructDestroyer accesses info_ptr, so
info_ptr must have a wider lifetime scope. Otherwise,
the behavior of the problem is undefined.

The issue is found by AddressSanitizer with use-after-scope check enabled.

BUG=649897

Review-Url: https://codereview.chromium.org/2576823002
==========

[commit-bot: I haz the power, 2016-12-15 23:40:33]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-15 23:42:47]

Description was changed from

==========
Fix stack-use-after-scope in png_codec.

PngWriteStructDestroyer accesses info_ptr, so
info_ptr must have a wider lifetime scope. Otherwise,
the behavior of the problem is undefined.

The issue is found by AddressSanitizer with use-after-scope check enabled.

BUG=649897

Review-Url: https://codereview.chromium.org/2576823002
==========

to

==========
Fix stack-use-after-scope in png_codec.

PngWriteStructDestroyer accesses info_ptr, so
info_ptr must have a wider lifetime scope. Otherwise,
the behavior of the problem is undefined.

The issue is found by AddressSanitizer with use-after-scope check enabled.

BUG=649897

Committed: https://crrev.com/23478bc8eb2dcf7df4678359e8fb164a5317313f
Cr-Commit-Position: refs/heads/master@{#438954}
==========

[commit-bot: I haz the power, 2016-12-15 23:42:48]

Patchset 9 (id:??) landed as
https://crrev.com/23478bc8eb2dcf7df4678359e8fb164a5317313f
Cr-Commit-Position: refs/heads/master@{#438954}