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

Issue 863823 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 22
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

CRAS: crash hfp_buf_queued-9c163718

Project Member Reported by hychao@chromium.org, Jul 16

Issue description

0x08671812	(cras -cras_audio_format.h:123 )	hfp_buf_queued
0x086713dd	(cras -cras_hfp_iodev.c:63 )	delay_frames
0x08685f05	(cras -cras_iodev.h:563 )	dev_io_playback_fetch
0x086862bb	(cras -dev_io.c:726 )	dev_io_run
0x086785bb	(cras -audio_thread.c:889 )	audio_io_thread
0xb4e9054f	(libpthread-2.23.so -pthread_create.c:335 )	start_thread
0xb4c1813f	(libc-2.23.so + 0x0009813f )	clone
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/adhd/+/23ed583f6315a0b18cac04ba7eee4fd18f10513a

commit 23ed583f6315a0b18cac04ba7eee4fd18f10513a
Author: Hsin-Yu Chao <hychao@chromium.org>
Date: Tue Jul 17 09:26:23 2018

CRAS: bt_device - Fix multi thread accessing iodev at the same time

For some cases we used timer to schedule a callback for BT profilde
switching, and that accesses iodev from main thread.
It is possible during the 500ms period before timer callback is
triggered, this iodev got enabled for some reason so audio thread
started reading or playing data to it.
That is dangerous because the update_active_node member function of
cras_bt_io.c could modify the active_node pointer while audio thread
is reading it. Fixing this bug by using additional check to void
timer callback calls update_active_node.
This bug may relate to some BT crashes we've been seeing for a long
time.

BUG= chromium:863823 
TEST=None

Change-Id: I094b1fc4ec61f2bd1a9056c3f80788f7d6e09adf
Reviewed-on: https://chromium-review.googlesource.com/1138039
Commit-Ready: Hsinyu Chao <hychao@chromium.org>
Tested-by: Hsinyu Chao <hychao@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/23ed583f6315a0b18cac04ba7eee4fd18f10513a/cras/src/server/cras_bt_device.c

Project Member

Comment 2 by bugdroid1@chromium.org, Jul 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/adhd/+/5e1e6981125b6d61190e5a5e64cd49272184936a

commit 5e1e6981125b6d61190e5a5e64cd49272184936a
Author: Hsin-Yu Chao <hychao@chromium.org>
Date: Thu Jul 19 09:52:12 2018

CRAS: iodev_list - Fix potential multi thread access

Audio thread starts using iodev for data I/O after it
enters 'enabled' state. In that case we should NOT call
update_active_node from main thread to modify the pointer
content.
This is especially dangerous for BT iodev because calling
update_active_node could change the active profile.
Add check to all places in cras_iodev_list potentially have
this problem.

BUG= chromium:863823 
TEST=None

Change-Id: I0f5e5684742e4710763c8ffb65b137d1f0cc2ea3
Reviewed-on: https://chromium-review.googlesource.com/1140093
Commit-Ready: Hsinyu Chao <hychao@chromium.org>
Tested-by: Hsinyu Chao <hychao@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/5e1e6981125b6d61190e5a5e64cd49272184936a/cras/src/server/cras_iodev_list.c

Cc: bhthompson@chromium.org
Labels: M-68 Merge-Request-68
Status: Started (was: Untriaged)
This is the top most crash we're seeing now, requesting R68 merge for the two fixes in #2 and #3.
Project Member

Comment 4 by sheriffbot@chromium.org, Jul 20

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: We are only 3 days from stable.
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
Labels: -Hotlist-Merge-Review -Merge-Review-68 Merge-Approved-68
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 22

Labels: merge-merged-release-R68-10718.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/adhd/+/623698d34e8c30fa67df08e01212fac22f9b0bad

commit 623698d34e8c30fa67df08e01212fac22f9b0bad
Author: Hsin-Yu Chao <hychao@chromium.org>
Date: Sun Jul 22 16:01:06 2018

CRAS: bt_device - Fix multi thread accessing iodev at the same time

For some cases we used timer to schedule a callback for BT profilde
switching, and that accesses iodev from main thread.
It is possible during the 500ms period before timer callback is
triggered, this iodev got enabled for some reason so audio thread
started reading or playing data to it.
That is dangerous because the update_active_node member function of
cras_bt_io.c could modify the active_node pointer while audio thread
is reading it. Fixing this bug by using additional check to void
timer callback calls update_active_node.
This bug may relate to some BT crashes we've been seeing for a long
time.

BUG= chromium:863823 
TEST=None

Change-Id: I094b1fc4ec61f2bd1a9056c3f80788f7d6e09adf
Reviewed-on: https://chromium-review.googlesource.com/1138039
Commit-Ready: Hsinyu Chao <hychao@chromium.org>
Tested-by: Hsinyu Chao <hychao@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
(cherry picked from commit 23ed583f6315a0b18cac04ba7eee4fd18f10513a)
Reviewed-on: https://chromium-review.googlesource.com/1146343
Reviewed-by: Hsinyu Chao <hychao@chromium.org>
Commit-Queue: Hsinyu Chao <hychao@chromium.org>

[modify] https://crrev.com/623698d34e8c30fa67df08e01212fac22f9b0bad/cras/src/server/cras_bt_device.c

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/adhd/+/6c9d16c07e61e1c66fd6505321553bee20e362b7

commit 6c9d16c07e61e1c66fd6505321553bee20e362b7
Author: Hsin-Yu Chao <hychao@chromium.org>
Date: Sun Jul 22 16:04:08 2018

CRAS: iodev_list - Fix potential multi thread access

Audio thread starts using iodev for data I/O after it
enters 'enabled' state. In that case we should NOT call
update_active_node from main thread to modify the pointer
content.
This is especially dangerous for BT iodev because calling
update_active_node could change the active profile.
Add check to all places in cras_iodev_list potentially have
this problem.

BUG= chromium:863823 
TEST=None

Change-Id: I0f5e5684742e4710763c8ffb65b137d1f0cc2ea3
Reviewed-on: https://chromium-review.googlesource.com/1140093
Commit-Ready: Hsinyu Chao <hychao@chromium.org>
Tested-by: Hsinyu Chao <hychao@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
(cherry picked from commit 5e1e6981125b6d61190e5a5e64cd49272184936a)
Reviewed-on: https://chromium-review.googlesource.com/1146344
Reviewed-by: Hsinyu Chao <hychao@chromium.org>
Commit-Queue: Hsinyu Chao <hychao@chromium.org>

[modify] https://crrev.com/6c9d16c07e61e1c66fd6505321553bee20e362b7/cras/src/server/cras_iodev_list.c

Labels: -Merge-Approved-68 Merge-Merged
Status: Fixed (was: Started)
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/adhd/+/14327702091ee73622c2d54c70d8135233428465

commit 14327702091ee73622c2d54c70d8135233428465
Author: Hsin-Yu Chao <hychao@chromium.org>
Date: Wed Oct 10 16:44:56 2018

CRAS: iodev_list - Check if pinned device is open already

cras_iodev can be opened and accessed by audio_thread
in two ways:
(1) UI enables an input/output audio node.
(2) An application creates a stream that pins to specific audio node.

This change adds a function to ask if the given device is already
in use by the audio_thread. It can be used to avoid unecessary
calls and possible multi thread access to iodev.

BUG= chromium:890242 ,  chromium:863823 
TEST=unittest

Change-Id: I24713e15f1039599abd3387fbc1bbe2c490efff1
Reviewed-on: https://chromium-review.googlesource.com/1257487
Commit-Ready: Hsinyu Chao <hychao@chromium.org>
Tested-by: Hsinyu Chao <hychao@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/14327702091ee73622c2d54c70d8135233428465/cras/src/server/audio_thread.h
[modify] https://crrev.com/14327702091ee73622c2d54c70d8135233428465/cras/src/server/cras_iodev_list.c
[modify] https://crrev.com/14327702091ee73622c2d54c70d8135233428465/cras/src/server/audio_thread.c
[modify] https://crrev.com/14327702091ee73622c2d54c70d8135233428465/cras/src/tests/iodev_list_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 12

Labels: merge-merged-release-R70-11021.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/adhd/+/83addda994d654f37a3b9bfea2a6a149442eb8ae

commit 83addda994d654f37a3b9bfea2a6a149442eb8ae
Author: Hsin-Yu Chao <hychao@chromium.org>
Date: Fri Oct 12 07:31:19 2018

CRAS: iodev_list - Check if pinned device is open already

cras_iodev can be opened and accessed by audio_thread
in two ways:
(1) UI enables an input/output audio node.
(2) An application creates a stream that pins to specific audio node.

This change adds a function to ask if the given device is already
in use by the audio_thread. It can be used to avoid unecessary
calls and possible multi thread access to iodev.

BUG= chromium:890242 ,  chromium:863823 
TEST=unittest

Change-Id: I24713e15f1039599abd3387fbc1bbe2c490efff1
Reviewed-on: https://chromium-review.googlesource.com/1257487
Commit-Ready: Hsinyu Chao <hychao@chromium.org>
Tested-by: Hsinyu Chao <hychao@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
(cherry picked from commit 14327702091ee73622c2d54c70d8135233428465)
Reviewed-on: https://chromium-review.googlesource.com/c/1278606
Reviewed-by: Hsinyu Chao <hychao@chromium.org>
Commit-Queue: Hsinyu Chao <hychao@chromium.org>

[modify] https://crrev.com/83addda994d654f37a3b9bfea2a6a149442eb8ae/cras/src/server/audio_thread.h
[modify] https://crrev.com/83addda994d654f37a3b9bfea2a6a149442eb8ae/cras/src/server/cras_iodev_list.c
[modify] https://crrev.com/83addda994d654f37a3b9bfea2a6a149442eb8ae/cras/src/server/audio_thread.c
[modify] https://crrev.com/83addda994d654f37a3b9bfea2a6a149442eb8ae/cras/src/tests/iodev_list_unittest.cc

Sign in to add a comment