New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 854816 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Security


Participants' hotlists:
Audio-Service


Sign in to add a comment

Heap-use-after-free in media::AudioManagerWin::InitializeOnAudioThread

Project Member Reported by ClusterFuzz, Jun 20 2018

Issue description

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

Fuzzer: inferno_canvas_wrecker
Job Type: windows_asan_chrome_no_sandbox
Platform Id: windows

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x127940285240
Crash State:
  media::AudioManagerWin::InitializeOnAudioThread
  base::debug::TaskAnnotator::RunTask
  base::MessageLoop::RunTask
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

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

Issue filed automatically.

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

Note: This crash might not be reproducible with the provided testcase. That said, for the past 14 days we've been seeing this crash frequently. If you are unable to reproduce this, please try a speculative fix based on the crash stacktrace in the report. The fix can be verified by looking at the crash statistics in the report, a day after the fix is deployed. We will auto-close the bug if the crash is not seen for 14 days.
 
Project Member

Comment 1 by ClusterFuzz, Jun 20 2018

Components: Internals>Core
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.

Comment 2 by och...@chromium.org, Jun 21 2018

Owner: grunell@chromium.org
Status: Assigned (was: Untriaged)
grunell, do you think you could take a look at this one, or find someone more suitable to?

We don't have a reproducible testcase, but this occurs very frequently on ClusterFuzz. Please see if you can see anything obvious from the stacktrace, thanks!
Cc: olka@chromium.org
Olga, is this maybe related to the audio service/process? Perhaps there is an existing bug that happen to manifest itself in that environment. Is it possible that the manager has been deleted before the task is run on the IO thread?
Cc: maxmorin@chromium.org
+Max since Olga is ooo today. See previous comment.
Cc: -maxmorin@chromium.org grunell@chromium.org
Components: -Internals>Core Internals>Media>Audio
Labels: Security_Impact-Beta Pri-1
Owner: maxmorin@chromium.org
Status: Started (was: Assigned)
Ooh, snap. Using Unretained to post the InitializeOnAudioThread used to be safe, since ShutdownOnAudioThread was also posted at some later time, and we blocked waiting for that. However, we avoid posting ShutdownOnAudioThread if we are already on the audio thread (we just call it directly), so there is no assurance that Initialize finished before that when running out of process (where the main thread is the thread that creates/shuts down the audio manager, and also the audio thread). I guess we should just avoid posting InitializeOnAudioThread if we are already on the audio thread. I'll take care of it.
But Mac has the same issue, and here we still want to post the initialization task when running on the main thread in the browser, but not in the service, so simply doing "if (GetTaskRunner()->BelongsToCurrentThread())" won't do. I'll think about it a bit.
Project Member

Comment 7 by sheriffbot@chromium.org, Jun 21 2018

Labels: M-68 Target-68
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 25 2018

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

commit 75c63f8b8f1f19874867ac05a60c4ebe89496e1a
Author: Max Morin <maxmorin@chromium.org>
Date: Mon Jun 25 14:11:08 2018

Fix InitializeOnAudioThread race.

It may race with ShutdownOnAudioThread when running the audio service
out of process.

Bug:  854816 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I2ccd4254f4c4f0f70ffb2180c3a586e35ce1baed
Reviewed-on: https://chromium-review.googlesource.com/1109683
Reviewed-by: Henrik Grunell <grunell@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570029}
[modify] https://crrev.com/75c63f8b8f1f19874867ac05a60c4ebe89496e1a/media/audio/mac/audio_manager_mac.cc
[modify] https://crrev.com/75c63f8b8f1f19874867ac05a60c4ebe89496e1a/media/audio/mac/audio_manager_mac.h
[modify] https://crrev.com/75c63f8b8f1f19874867ac05a60c4ebe89496e1a/media/audio/win/audio_manager_win.cc

Cc: maxmorin@chromium.org
Labels: Merge-Request-68
Owner: olka@chromium.org
Requesting merge to M68. Should be a safe merge, and the bug could have security implications.

Olga: Could you merge it?
Project Member

Comment 10 by sheriffbot@chromium.org, Jul 2

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

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

Comment 11 by sheriffbot@chromium.org, Jul 2

Status: Fixed (was: Started)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

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

Comment 13 by bugdroid1@chromium.org, Jul 3

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5428410d921237525cc40d3022a1589855d418b6

commit 5428410d921237525cc40d3022a1589855d418b6
Author: Max Morin <maxmorin@chromium.org>
Date: Tue Jul 03 12:50:43 2018

Fix InitializeOnAudioThread race.

It may race with ShutdownOnAudioThread when running the audio service
out of process.

Bug:  854816 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I2ccd4254f4c4f0f70ffb2180c3a586e35ce1baed
Reviewed-on: https://chromium-review.googlesource.com/1109683
Reviewed-by: Henrik Grunell <grunell@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#570029}(cherry picked from commit 75c63f8b8f1f19874867ac05a60c4ebe89496e1a)
Reviewed-on: https://chromium-review.googlesource.com/1124479
Reviewed-by: Olga Sharonova <olka@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#585}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/5428410d921237525cc40d3022a1589855d418b6/media/audio/mac/audio_manager_mac.cc
[modify] https://crrev.com/5428410d921237525cc40d3022a1589855d418b6/media/audio/mac/audio_manager_mac.h
[modify] https://crrev.com/5428410d921237525cc40d3022a1589855d418b6/media/audio/win/audio_manager_win.cc

Owner: maxmorin@chromium.org
Project Member

Comment 15 by sheriffbot@chromium.org, Jul 3

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 16 by sheriffbot@chromium.org, Oct 9

Labels: -Restrict-View-SecurityNotify allpublic
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

Sign in to add a comment