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

Issue 892608 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

CRAS: crash at stream using APM processing

Project Member Reported by hychao@chromium.org, Oct 5

Issue description

Open three input streams, with different settings:

cras_test_client --capture_file /tmp/1 --block_size 480
cras_test_client --capture_file /tmp/2 --block_size 256
cras_test_client --capture_file /tmp/3 --block_size 256 --effects aec

By doing this, and with some luck, we can see CRAS crash in a short while.

There are two problems causing crash. Valgrind reports:

First one:
==28517== Process terminating with default action of signal 11 (SIGSEGV)
==28517==  Access not within mapped region at address 0x8126000
==28517==    at 0x14B3F8: cras_mix_add_scale_stride_s16_le (cras_mix_ops.c:196)
==28517==    by 0x14B3F8: mix_add_scale_stride (cras_mix_ops.c:832)
==28517==    by 0x12740A: cras_audio_area_copy (cras_audio_area.c:51)
==28517==    by 0x13CFB3: dev_stream_capture (dev_stream.c:402)
==28517==    by 0x13B67F: capture_to_streams (dev_io.c:440)
==28517==    by 0x13B67F: dev_io_capture (dev_io.c:766)
==28517==    by 0x13BF21: dev_io_run (dev_io.c:869)
==28517==    by 0x125F4D: audio_io_thread (audio_thread.c:898)
==28517==    by 0x4A2E2B7: start_thread (pthread_create.c:333)
==28517==    by 0x5505FAC: clone (clone.S:109)


Second:

==29341== Invalid read of size 2
==29341==    at 0x403311E: memcpy@@GLIBC_2.14 (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==29341==    by 0x13E640: input_data_get_for_stream (input_data.c:107)
==29341==    by 0x13B661: capture_to_streams (dev_io.c:436)
==29341==    by 0x13B661: dev_io_capture (dev_io.c:766)
==29341==    by 0x13BF21: dev_io_run (dev_io.c:869)
==29341==    by 0x125F4D: audio_io_thread (audio_thread.c:898)
==29341==    by 0x4A2E2B7: start_thread (pthread_create.c:333)
==29341==    by 0x5505FAC: clone (clone.S:109)
==29341==  Address 0x7c952b4 is 0 bytes after a block of size 32,788 alloc'd
==29341==    at 0x402FB71: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==29341==    by 0x13E489: float_buffer_create (float_buffer.h:40)
==29341==    by 0x13E489: input_data_configure (input_data.c:51)
==29341==    by 0x12F83B: add_ext_dsp_module_to_pipeline (cras_iodev.c:525)
==29341==    by 0x130449: cras_iodev_open (cras_iodev.c:949)
==29341==    by 0x131D21: init_device (cras_iodev_list.c:494)
==29341==    by 0x131D21: stream_added_cb (cras_iodev_list.c:764)
==29341==    by 0x13F11B: stream_list_add (stream_list.c:88)
==29341==    by 0x137233: handle_client_stream_connect (cras_rclient.c:74)
==29341==    by 0x136A22: cras_rclient_message_from_client (cras_rclient.c:0)
==29341==    by 0x13694D: cras_rclient_buffer_from_client (cras_rclient.c:421)
==29341==    by 0x110C4A: handle_message_from_client (cras_server.c:135)
==29341==    by 0x110C4A: cras_server_run (cras_server.c:605)
==29341==    by 0x11011D: main (cras.c:141)

 
Status: Started (was: Assigned)
Turns out ARC++ recording is affected by this, because of recent uprev.

Because of in cheets2/audio input stream is created by calling cras_client_unified_params_create()
And this API doesn't initialize 'effects' member.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 10

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

commit 78e781a0764b327f8af7748bd2649bc845c1b8c4
Author: Hsin-Yu Chao <hychao@google.com>
Date: Wed Oct 10 04:14:00 2018

CRAS: float_buffer - Fix readable count

Fix a bug in float_buffer when passing non-zero offset. Without
the fix, the result readable frames count could be incorrectly
larger and cause crash.

BUG= chromium:892608 
TEST=unittest

Change-Id: I2d5f1724a032bc3264e4c178bf2b4e488686cde4
Reviewed-on: https://chromium-review.googlesource.com/1265215
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Hsinyu Chao <hychao@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[add] https://crrev.com/78e781a0764b327f8af7748bd2649bc845c1b8c4/cras/src/tests/float_buffer_unittest.cc
[modify] https://crrev.com/78e781a0764b327f8af7748bd2649bc845c1b8c4/cras/src/Makefile.am
[modify] https://crrev.com/78e781a0764b327f8af7748bd2649bc845c1b8c4/cras/src/server/float_buffer.h

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 10

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

commit cd5c53f1468104c55e1f8de22ef766f01616a794
Author: Hsin-Yu Chao <hychao@google.com>
Date: Wed Oct 10 04:14:00 2018

CRAS: input_data - Fix larger offset case

As the background explained in commit 01cb78342, when multiple
input streams are connected with different buffer sizes and processing
options, there could be a case in capture_to_streams() that stream
offset is larger than the buffer size got from input iodev.

This commit fixes the crash caused by above scenario plus the
assumption in cras_audio_area_copy() that stream offset not larger
than buffer size.

BUG= chromium:892608 
TEST=Start three input streams
cras_test_client --capture_file /tmp/1 --block_size 480
cras_test_client --capture_file /tmp/2 --block_size 256
cras_test_client --capture_file /tmp/3 --block_size 256 --effects aec
and verify CRAS doesn't crash.

Change-Id: I869a19d88a32ee764127766b432a4b17011d7933
Reviewed-on: https://chromium-review.googlesource.com/1265216
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Hsinyu Chao <hychao@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/cd5c53f1468104c55e1f8de22ef766f01616a794/cras/src/server/input_data.c

Cc: geohsu@chromium.org
Labels: Merge-Request-70
Crash fixes has been verified on Tot. Requesting merge to M70.
Project Member

Comment 5 by sheriffbot@chromium.org, Oct 10

Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: We are only 5 days from stable.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

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

Comment 7 by bugdroid1@chromium.org, Oct 11

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

commit b62dac334c455bbce25d08c798a53c7bb77f7862
Author: Hsin-Yu Chao <hychao@google.com>
Date: Thu Oct 11 02:41:03 2018

CRAS: float_buffer - Fix readable count

Fix a bug in float_buffer when passing non-zero offset. Without
the fix, the result readable frames count could be incorrectly
larger and cause crash.

BUG= chromium:892608 
TEST=unittest

Change-Id: I2d5f1724a032bc3264e4c178bf2b4e488686cde4
Reviewed-on: https://chromium-review.googlesource.com/1265215
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Hsinyu Chao <hychao@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
(cherry picked from commit 78e781a0764b327f8af7748bd2649bc845c1b8c4)
Reviewed-on: https://chromium-review.googlesource.com/c/1275246
Reviewed-by: Hsinyu Chao <hychao@chromium.org>
Commit-Queue: Hsinyu Chao <hychao@chromium.org>

[add] https://crrev.com/b62dac334c455bbce25d08c798a53c7bb77f7862/cras/src/tests/float_buffer_unittest.cc
[modify] https://crrev.com/b62dac334c455bbce25d08c798a53c7bb77f7862/cras/src/Makefile.am
[modify] https://crrev.com/b62dac334c455bbce25d08c798a53c7bb77f7862/cras/src/server/float_buffer.h

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 11

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

commit e32cee3708209cdc91d6d9b5a025f1bf2eab1c8d
Author: Hsin-Yu Chao <hychao@google.com>
Date: Thu Oct 11 02:44:50 2018

CRAS: input_data - Fix larger offset case

As the background explained in commit 01cb78342, when multiple
input streams are connected with different buffer sizes and processing
options, there could be a case in capture_to_streams() that stream
offset is larger than the buffer size got from input iodev.

This commit fixes the crash caused by above scenario plus the
assumption in cras_audio_area_copy() that stream offset not larger
than buffer size.

BUG= chromium:892608 
TEST=Start three input streams
cras_test_client --capture_file /tmp/1 --block_size 480
cras_test_client --capture_file /tmp/2 --block_size 256
cras_test_client --capture_file /tmp/3 --block_size 256 --effects aec
and verify CRAS doesn't crash.

Change-Id: I869a19d88a32ee764127766b432a4b17011d7933
Reviewed-on: https://chromium-review.googlesource.com/1265216
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Hsinyu Chao <hychao@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
(cherry picked from commit cd5c53f1468104c55e1f8de22ef766f01616a794)
Reviewed-on: https://chromium-review.googlesource.com/c/1275505
Reviewed-by: Hsinyu Chao <hychao@chromium.org>
Commit-Queue: Hsinyu Chao <hychao@chromium.org>

[modify] https://crrev.com/e32cee3708209cdc91d6d9b5a025f1bf2eab1c8d/cras/src/server/input_data.c

Labels: -Merge-Approved-70
Status: Fixed (was: Started)

Sign in to add a comment