This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

hiroshige@: This is a recent security regression showing up in SVGImage::ServiceAnimations, and you have a couple of related CLs in the regression range. It is a use-after-poison, possibly on a stale image_observer_. Can you please investigate this?

Reproduced also on Linux.

First bad commit: https://codereview.chromium.org/2897533002

The root cause for this is ImageResourceContent doesn't clear its |ImageResourceContent::image_|'s observer.

In most cases, this is not exposed, because ImageResourceContent is usually the only owner of |image_|, and thus |image_| and ImageResourceContent are destructed together, and we can't observe the stale |Image::observer_| pointer.

My CL (Comment #6) exposed this because it adds another RefPtr<Image> to |image_|.
However, as the root cause has existed since before, I suspect the use-after-poison bug also has existed before my CL.

Adding more people who knows Image/SVGImage/ImageResourceContent.

Related issue: some code locations seems to assume that the lifetimes of Image and ImageObserver=ImageResourceContent are coupled.
GetImageObserver() is assumed to be non-null, while it can be actaully null:
- BitmapImage::InternalAdvanceAnimation()
- BitmapImage::NotifyObserversOfAnimationAdvance()
- SVGImageChromeClient::AnimationTimerFired()
- SVGImage::AdvanceAnimationForTesting()
- SVGImage::ServiceAnimations()

Null pointer dereferences at SVGImage::ServiceAnimations():
- 60.0.3082.0 da567f6e80000000
- 60.0.3084.0 b614aaee80000000

Created CLs and started review:
[1] https://codereview.chromium.org/2905833003/
[2] https://codereview.chromium.org/2906583003/

While I couldn't find crashes related to this in M-59 or earlier, I suspect I should merge the fixes to M-59.
If we can
(1) Create a reference to Image/SVGImage from outside of ImageResourceContent,
(2) Garbage-collect ImageResourceContent (this can be done by moving <img> to
    another document), and then
(3) Use Image::GetImageObserver(),
then we can cause use-after-poison at Step (3) even before my CL.
(If we couldn't do (1), we can/should make Image managed by unique_ptr<> but I expect this isn't the case)
hajimehoshi@, do you have any idea for exposing this?

2 crashes in BitmapImage::NotifyObserversOfAnimationAdvance():
Query:
custom_data.ChromeCrashProto.magic_signature_1.name='blink::BitmapImage::notifyObserversOfAnimationAdvance'

~15 crashes in BitmapImage::InternalAdvanceAnimation():
Query:
custom_data.ChromeCrashProto.magic_signature_1.name='blink::BitmapImage::internalAdvanceAnimation' AND crash.address=0
Query:
custom_data.ChromeCrashProto.magic_signature_1.name='blink::BitmapImage::internalAdvanceAnimation' AND crash.address=28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e056249f434f9ccca8024246ebb879d225b9f55a

commit e056249f434f9ccca8024246ebb879d225b9f55a
Author: hiroshige <hiroshige@chromium.org>
Date: Fri May 26 20:43:39 2017

Make Image::image_observer_ WeakPersistent

|image_observer_| was made UntracedMember during Oilpanization in [1],
and this CL turns it WeakPersistent, to clear |image_observer_|
when the corresponding ImageResourceContent is garbage collected while
Image is still alive because the Image is referenced from other than
ImageResourceContent.

This CL also add null checks of GetImageObserver() to SVG-related code,
because GetImageObserver() can be null since before.

[1] https://codereview.chromium.org/1610883002

BUG= 726220 

Review-Url: https://codereview.chromium.org/2905833003
Cr-Commit-Position: refs/heads/master@{#475116}

[add] https://crrev.com/e056249f434f9ccca8024246ebb879d225b9f55a/third_party/WebKit/LayoutTests/svg/as-image/adopt-while-async-svg-load-is-in-progress-crash.html
[modify] https://crrev.com/e056249f434f9ccca8024246ebb879d225b9f55a/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
[modify] https://crrev.com/e056249f434f9ccca8024246ebb879d225b9f55a/third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
[modify] https://crrev.com/e056249f434f9ccca8024246ebb879d225b9f55a/third_party/WebKit/Source/platform/graphics/Image.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c33b530e841ad76be6e6c472711fda4e09c9d4d

commit 1c33b530e841ad76be6e6c472711fda4e09c9d4d
Author: hiroshige <hiroshige@chromium.org>
Date: Fri May 26 21:52:14 2017

Add null checks of GetImageObserver() in BitmapImage

Because GetImageObserver() can be null but some callsites have used it
without null checks.
This CL adds null checks to BitmapImage, while [1] adds to SVG-related
classes.

[1] https://codereview.chromium.org/2905833003/

BUG= 726220 

Review-Url: https://codereview.chromium.org/2906583003
Cr-Commit-Position: refs/heads/master@{#475147}

[modify] https://crrev.com/1c33b530e841ad76be6e6c472711fda4e09c9d4d/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp

ClusterFuzz has detected this issue as fixed in range 475115:475134.

Detailed report: https://clusterfuzz.com/testcase?key=5740035101163520

Fuzzer: inferno_twister
Job Type: mac_asan_chrome
Platform Id: mac

Crash Type: Use-after-poison READ 8
Crash Address: 0x7ec86a645120
Crash State:
  blink::SVGImage::ServiceAnimations
  blink::TimerBase::RunInternal
  base::debug::TaskAnnotator::RunTask
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=mac_asan_chrome&range=473589:473622
Fixed: https://clusterfuzz.com/revisions?job=mac_asan_chrome&range=475115:475134

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5740035101163520


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

Fixed. I'll request merge to M-60 (and M-59?) later.

ClusterFuzz testcase 5740035101163520 is verified as fixed, so closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Requesting merge to M-60 (CLs at Comment #15 and #16).

Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6c28cbc115d39a06d728f09c69bb7100f6aaec86

commit 6c28cbc115d39a06d728f09c69bb7100f6aaec86
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Tue May 30 08:18:38 2017

Make Image::image_observer_ WeakPersistent

|image_observer_| was made UntracedMember during Oilpanization in [1],
and this CL turns it WeakPersistent, to clear |image_observer_|
when the corresponding ImageResourceContent is garbage collected while
Image is still alive because the Image is referenced from other than
ImageResourceContent.

This CL also add null checks of GetImageObserver() to SVG-related code,
because GetImageObserver() can be null since before.

[1] https://codereview.chromium.org/1610883002

BUG= 726220 

Review-Url: https://codereview.chromium.org/2905833003
Cr-Original-Commit-Position: refs/heads/master@{#475116}
Review-Url: https://codereview.chromium.org/2908333002 .
Cr-Commit-Position: refs/branch-heads/3112@{#24}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[add] https://crrev.com/6c28cbc115d39a06d728f09c69bb7100f6aaec86/third_party/WebKit/LayoutTests/svg/as-image/adopt-while-async-svg-load-is-in-progress-crash.html
[modify] https://crrev.com/6c28cbc115d39a06d728f09c69bb7100f6aaec86/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
[modify] https://crrev.com/6c28cbc115d39a06d728f09c69bb7100f6aaec86/third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
[modify] https://crrev.com/6c28cbc115d39a06d728f09c69bb7100f6aaec86/third_party/WebKit/Source/platform/graphics/Image.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb2694f0cbf46f6ac6b467bf04a9373e4981c2ba

commit bb2694f0cbf46f6ac6b467bf04a9373e4981c2ba
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Tue May 30 09:34:28 2017

Add null checks of GetImageObserver() in BitmapImage

Because GetImageObserver() can be null but some callsites have used it
without null checks.
This CL adds null checks to BitmapImage, while [1] adds to SVG-related
classes.

[1] https://codereview.chromium.org/2905833003/

BUG= 726220 

Review-Url: https://codereview.chromium.org/2906583003
Cr-Original-Commit-Position: refs/heads/master@{#475147}
Review-Url: https://codereview.chromium.org/2914583003 .
Cr-Commit-Position: refs/branch-heads/3112@{#25}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/bb2694f0cbf46f6ac6b467bf04a9373e4981c2ba/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot