New issue
Advanced search Search tips

Issue 792108 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

Null-dereference READ in blink::LocalFrame::GetDocument

Project Member Reported by ClusterFuzz, Dec 5 2017

Issue description

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

Fuzzer: inferno_twister_c
Job Type: linux_asan_content_shell_drt
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000030
Crash State:
  blink::LocalFrame::GetDocument
  blink::Worklet::Worklet
  blink::AudioWorklet::AudioWorklet
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_content_shell_drt&range=521077:521078

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

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Project Member

Comment 1 by ClusterFuzz, Dec 5 2017

Components: Blink>Internals Blink>Workers
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Project Member

Comment 2 by ClusterFuzz, Dec 5 2017

Labels: Test-Predator-Auto-Owner
Owner: hongchan@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/249e75ed36a08aed24a4c6f81aca47b384bd2ad8 (Move audioWorklet under BaseAudioContext).

If this is incorrect, please remove the owner and apply the Test-Predator-Wrong-CLs label.
Components: Blink>WebAudio
Cc: nhiroki@chromium.org
nhiroki@ Does Worklet behave okay when the frame is gone? 

Stack trace:
---
#0 0xfa3961d in blink::LocalFrame::GetDocument() const third_party/WebKit/Source/core/frame/LocalFrame.cpp:433
 #1 0x11863995 in blink::Worklet::Worklet(blink::LocalFrame*) third_party/WebKit/Source/core/workers/Worklet.cpp:36:39
 #2 0x1333f20f in blink::AudioWorklet::AudioWorklet(blink::BaseAudioContext*) third_party/WebKit/Source/modules/webaudio/AudioWorklet.cpp:26:7
#3 0x1333ed96 in blink::AudioWorklet::Create(blink::BaseAudioContext*) third_party/WebKit/Source/modules/webaudio/AudioWorklet.cpp:21:13
#4 0x1328ec6f in blink::BaseAudioContext::Initialize() third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:154:22
#5 0x13334949 in AudioContext third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:112:3
#6 0x13334949 in blink::AudioContext::Create(blink::Document&, blink::AudioContextOptions const&, blink::ExceptionState&) third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:63
---

Repro case:
---
var iframe = document.createElementNS("http://www.w3.org/1999/xhtml", "iframe");
document.body.appendChild(iframe);
var frameWin = iframe.contentWindow;
new frameWin.AudioContext();
document.body.removeChild(iframe);
new frameWin.AudioContext();          <== (A)
---

I believe at (A) might crash with "Null Dereference" because Worklet accepts the "local frame" as an argument.

Also this suggests the frame can be null at any moment.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.h?gsn=Document&l=498

What's our expectation on Worklet when the frame is null?
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 6 2017

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

commit 7f87a643cf9d5de271f2b6adeb7e30d6a5f8eb5d
Author: Hongchan Choi <hongchan@chromium.org>
Date: Wed Dec 06 02:22:29 2017

Block null Frame object for Worklet instantiation

Worklet requires a valid Frame object, but GetFrame() from the window
can be nullptr. This CL blocks out such case.

Bug:  792108 
Change-Id: Ie3fe64c707447956fcfeac687d4df9f5bf7b26d1
Reviewed-on: https://chromium-review.googlesource.com/809299
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Hongchan Choi <hongchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521956}
[modify] https://crrev.com/7f87a643cf9d5de271f2b6adeb7e30d6a5f8eb5d/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp

Status: Fixed (was: Assigned)
Fixed, but waiting for CF to verify.
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 6 2017

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

commit a3acb2d50c09040f45c2f2efe8f624cb63e3bf2c
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Wed Dec 06 07:07:50 2017

Worklet: Remove dependency on LocalFrame from Worklet

This is a clean-up and doesn't change behavior. Worklets care about the lifetime
of Document not LocalFrame. To make it clearer, this CL removes dependency on
LocalFrame from Worklet.

Bug:  792307 ,  792108 
Change-Id: I24268c2f59efa70f26a0024bb02ee8a35d01ba7f
Reviewed-on: https://chromium-review.googlesource.com/810405
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522033}
[modify] https://crrev.com/a3acb2d50c09040f45c2f2efe8f624cb63e3bf2c/third_party/WebKit/Source/core/workers/Worklet.cpp
[modify] https://crrev.com/a3acb2d50c09040f45c2f2efe8f624cb63e3bf2c/third_party/WebKit/Source/core/workers/Worklet.h
[modify] https://crrev.com/a3acb2d50c09040f45c2f2efe8f624cb63e3bf2c/third_party/WebKit/Source/modules/animationworklet/AnimationWorklet.cpp
[modify] https://crrev.com/a3acb2d50c09040f45c2f2efe8f624cb63e3bf2c/third_party/WebKit/Source/modules/animationworklet/AnimationWorklet.h
[modify] https://crrev.com/a3acb2d50c09040f45c2f2efe8f624cb63e3bf2c/third_party/WebKit/Source/modules/animationworklet/WindowAnimationWorklet.cpp
[modify] https://crrev.com/a3acb2d50c09040f45c2f2efe8f624cb63e3bf2c/third_party/WebKit/Source/modules/animationworklet/WindowAnimationWorklet.h
[modify] https://crrev.com/a3acb2d50c09040f45c2f2efe8f624cb63e3bf2c/third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp
[modify] https://crrev.com/a3acb2d50c09040f45c2f2efe8f624cb63e3bf2c/third_party/WebKit/Source/modules/webaudio/AudioWorklet.cpp
[modify] https://crrev.com/a3acb2d50c09040f45c2f2efe8f624cb63e3bf2c/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp

Project Member

Comment 8 by ClusterFuzz, Dec 6 2017

ClusterFuzz has detected this issue as fixed in range 521955:521956.

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

Fuzzer: inferno_twister_c
Job Type: linux_asan_content_shell_drt
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000030
Crash State:
  blink::LocalFrame::GetDocument
  blink::Worklet::Worklet
  blink::AudioWorklet::AudioWorklet
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_content_shell_drt&range=521077:521078
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_content_shell_drt&range=521955:521956

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

See https://github.com/google/clusterfuzz-tools for more information.

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

Comment 9 by ClusterFuzz, Dec 6 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Fixed)
ClusterFuzz testcase 6717031004241920 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Labels: Merge-Request-64
Requesting merge for M64; this is a follow up on the  issue 786542 .
Project Member

Comment 11 by sheriffbot@chromium.org, Dec 7 2017

Labels: -Merge-Request-64 Hotlist-Merge-Approved Merge-Approved-64
Your change meets the bar and is auto-approved for M64. Please go ahead and merge the CL to branch 3282 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

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

Comment 12 by bugdroid1@chromium.org, Dec 7 2017

Labels: -merge-approved-64 merge-merged-3282
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8532ad1a3dbf3c521c29ceeb43f51f1486601e4b

commit 8532ad1a3dbf3c521c29ceeb43f51f1486601e4b
Author: Hongchan Choi <hongchan@chromium.org>
Date: Thu Dec 07 21:21:42 2017

Block null Frame object for Worklet instantiation

Worklet requires a valid Frame object, but GetFrame() from the window
can be nullptr. This CL blocks out such case.

Bug:  792108 
Change-Id: Ie3fe64c707447956fcfeac687d4df9f5bf7b26d1
Reviewed-on: https://chromium-review.googlesource.com/809299
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Hongchan Choi <hongchan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#521956}(cherry picked from commit 7f87a643cf9d5de271f2b6adeb7e30d6a5f8eb5d)
Reviewed-on: https://chromium-review.googlesource.com/815474
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Cr-Commit-Position: refs/branch-heads/3282@{#77}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/8532ad1a3dbf3c521c29ceeb43f51f1486601e4b/third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp

Sign in to add a comment