Chromium Code Reviews
Help | Chromium Project | Sign in
(13)

Issue 2576823002: Fix stack-use-after-scope in png_codec. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 2 weeks ago by krasin1
Modified:
4 months, 2 weeks ago
CC:
chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Add comma #

Total comments: 2

Patch Set 3 : Move ownership into PngWriteStructInfo. #

Total comments: 3

Patch Set 4 : png_str_ -> png_ptr_ #

Patch Set 5 : 0 -> nullptr #

Patch Set 6 : no explicit #

Patch Set 7 : 80 chars #

Patch Set 8 : PngReadStructInfo #

Patch Set 9 : BuildPNGStruct -> PngReadStructInfo::Build #

Unified diffs Side-by-side diffs Delta from patch set Stats (+61 lines, -68 lines) Patch
M ui/gfx/codec/png_codec.cc View 1 2 3 4 5 6 7 8 6 chunks +61 lines, -68 lines 0 comments Download
Trybot results:  chromium_presubmit   android_n5x_swarming_rel   android_n5x_swarming_rel   win_clang   win_chromium_x64_rel_ng   win_chromium_compile_dbg_ng   win_chromium_rel_ng   chromium_presubmit   android_n5x_swarming_rel   win_chromium_x64_rel_ng   chromium_presubmit   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   win_chromium_compile_dbg_ng   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   chromium_presubmit   win_chromium_compile_dbg_ng   win_clang   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   chromium_presubmit   linux_android_rel_ng   win_chromium_rel_ng   win_clang   win_chromium_rel_ng   win_chromium_x64_rel_ng   chromium_presubmit   win_chromium_compile_dbg_ng   win_chromium_rel_ng   mac_chromium_rel_ng   mac_chromium_compile_dbg_ng   ios-simulator-xcode-clang   ios-simulator   ios-device-xcode-clang   ios-device   linux_chromium_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_asan_rel_ng   chromium_presubmit   chromeos_daisy_chromium_compile_only_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   linux_android_rel_ng   cast_shell_android   android_cronet   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe 
Commit queue not available (can’t edit this change).

Messages

Total messages: 51 (23 generated)
krasin1
4 months, 2 weeks ago (2016-12-14 19:37:44 UTC) #2
Nico
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#newcode715 ui/gfx/codec/png_codec.cc:715: // info_ptr should have wider lifetime scope than ...
4 months, 2 weeks ago (2016-12-14 20:11:29 UTC) #6
krasin1
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#newcode715 ui/gfx/codec/png_codec.cc:715: // info_ptr should have wider lifetime scope than destroyer ...
4 months, 2 weeks ago (2016-12-14 20:56:59 UTC) #7
Nico
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#newcode715 > ...
4 months, 2 weeks ago (2016-12-14 22:24:32 UTC) #8
dcheng (OOO through May 2)
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 > ...
4 months, 2 weeks ago (2016-12-14 22:32:47 UTC) #9
krasin1
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.cc#newcode718 ui/gfx/codec/png_codec.cc:718: PngWriteStructDestroyer destroyer(&png_ptr); Daniel, Nico, I have made a simplification ...
4 months, 2 weeks ago (2016-12-14 23:45:04 UTC) #12
Nico
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.cc#newcode334 ui/gfx/codec/png_codec.cc:334: explicit PngWriteStructInfo() : png_str_(0), info_ptr_(0) ...
4 months, 2 weeks ago (2016-12-14 23:47:52 UTC) #13
dcheng (OOO through May 2)
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.cc#newcode334 ui/gfx/codec/png_codec.cc:334: explicit PngWriteStructInfo() : png_str_(0), info_ptr_(0) { ...
4 months, 2 weeks ago (2016-12-14 23:48:53 UTC) #14
krasin1
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 ...
4 months, 2 weeks ago (2016-12-14 23:51:18 UTC) #15
krasin1
Shall I also convert other Png*Destroyer classes to the new scheme? I feel like doing ...
4 months, 2 weeks ago (2016-12-14 23:53:47 UTC) #16
Nico
lgtm++
4 months, 2 weeks ago (2016-12-14 23:54:43 UTC) #17
krasin1
At this time, I satisfied with the change. One interesting insight that this code has ...
4 months, 2 weeks ago (2016-12-15 00:40:23 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2576823002/160001
4 months, 2 weeks ago (2016-12-15 00:41:09 UTC) #21
commit-bot: I haz the power
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_compile_dbg_ng/builds/315146) win_clang on master.tryserver.chromium.win (JOB_FAILED, ...
4 months, 2 weeks ago (2016-12-15 00:48:50 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2576823002/160001
4 months, 2 weeks ago (2016-12-15 01:02:32 UTC) #25
commit-bot: I haz the power
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_compile_dbg_ng/builds/315170) win_clang on master.tryserver.chromium.win (JOB_FAILED, ...
4 months, 2 weeks ago (2016-12-15 01:09:19 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2576823002/160001
4 months, 2 weeks ago (2016-12-15 01:34:13 UTC) #29
commit-bot: I haz the power
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_rel_ng/builds/335003)
4 months, 2 weeks ago (2016-12-15 01:41:17 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2576823002/160001
4 months, 2 weeks ago (2016-12-15 01:42:18 UTC) #33
commit-bot: I haz the power
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_rel_ng/builds/335016)
4 months, 2 weeks ago (2016-12-15 01:46:51 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2576823002/160001
4 months, 2 weeks ago (2016-12-15 01:51:19 UTC) #37
commit-bot: I haz the power
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_ng/builds/350409) win_clang on master.tryserver.chromium.win (JOB_FAILED, ...
4 months, 2 weeks ago (2016-12-15 01:56:26 UTC) #39
krasin1
On 2016/12/15 01:56:26, commit-bot: I haz the power wrote: > Try jobs failed on following ...
4 months, 2 weeks ago (2016-12-15 01:58:12 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2576823002/160001
4 months, 2 weeks ago (2016-12-15 19:35:21 UTC) #42
commit-bot: I haz the power
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_swarming_rel/builds/86972)
4 months, 2 weeks ago (2016-12-15 21:13:09 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2576823002/160001
4 months, 2 weeks ago (2016-12-15 22:59:25 UTC) #46
commit-bot: I haz the power
Committed patchset #9 (id:160001)
4 months, 2 weeks ago (2016-12-15 23:40:33 UTC) #49
commit-bot: I haz the power
4 months, 2 weeks ago (2016-12-15 23:42:48 UTC) #51
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/23478bc8eb2dcf7df4678359e8fb164a5317313f
Cr-Commit-Position: refs/heads/master@{#438954}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46