Commit 440e8d7f authored by Hongchan Choi's avatar Hongchan Choi Committed by Commit Bot

Fix data race in MediaStreamAudioSourceHandler

Fixes a potential data race of |source_number_of_channels_|. In the
worst scenario, the rendering may fail with a crash when the output
channel count does not match.

- SetFormat() is peeking/writing the value. (mostly from main thread)
- Process() is reading the value. (rendering thread)

Bug: 1108477
Change-Id: I4b64eab94b66e1998a456915d4eaa391791dc44f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2316404
Commit-Queue: Hongchan Choi <hongchan@chromium.org>
Reviewed-by: default avatarRaymond Toy <rtoy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792035}
parent a277eacb
...@@ -40,8 +40,7 @@ MediaStreamAudioSourceHandler::MediaStreamAudioSourceHandler( ...@@ -40,8 +40,7 @@ MediaStreamAudioSourceHandler::MediaStreamAudioSourceHandler(
: AudioHandler(kNodeTypeMediaStreamAudioSource, : AudioHandler(kNodeTypeMediaStreamAudioSource,
node, node,
node.context()->sampleRate()), node.context()->sampleRate()),
audio_source_provider_(std::move(audio_source_provider)), audio_source_provider_(std::move(audio_source_provider)) {
source_number_of_channels_(0) {
// Default to stereo. This could change depending on the format of the // Default to stereo. This could change depending on the format of the
// MediaStream's audio track. // MediaStream's audio track.
AddOutput(2); AddOutput(2);
...@@ -63,6 +62,7 @@ MediaStreamAudioSourceHandler::~MediaStreamAudioSourceHandler() { ...@@ -63,6 +62,7 @@ MediaStreamAudioSourceHandler::~MediaStreamAudioSourceHandler() {
void MediaStreamAudioSourceHandler::SetFormat(uint32_t number_of_channels, void MediaStreamAudioSourceHandler::SetFormat(uint32_t number_of_channels,
float source_sample_rate) { float source_sample_rate) {
MutexLocker locker(process_lock_);
if (number_of_channels != source_number_of_channels_ || if (number_of_channels != source_number_of_channels_ ||
source_sample_rate != Context()->sampleRate()) { source_sample_rate != Context()->sampleRate()) {
// The sample-rate must be equal to the context's sample-rate. // The sample-rate must be equal to the context's sample-rate.
...@@ -76,9 +76,6 @@ void MediaStreamAudioSourceHandler::SetFormat(uint32_t number_of_channels, ...@@ -76,9 +76,6 @@ void MediaStreamAudioSourceHandler::SetFormat(uint32_t number_of_channels,
return; return;
} }
// Synchronize with process().
MutexLocker locker(process_lock_);
source_number_of_channels_ = number_of_channels; source_number_of_channels_ = number_of_channels;
{ {
...@@ -102,16 +99,15 @@ void MediaStreamAudioSourceHandler::Process(uint32_t number_of_frames) { ...@@ -102,16 +99,15 @@ void MediaStreamAudioSourceHandler::Process(uint32_t number_of_frames) {
return; return;
} }
if (source_number_of_channels_ != output_bus->NumberOfChannels()) {
output_bus->Zero();
return;
}
// Use a tryLock() to avoid contention in the real-time audio thread. // Use a tryLock() to avoid contention in the real-time audio thread.
// If we fail to acquire the lock then the MediaStream must be in the middle // If we fail to acquire the lock then the MediaStream must be in the middle
// of a format change, so we output silence in this case. // of a format change, so we output silence in this case.
MutexTryLocker try_locker(process_lock_); MutexTryLocker try_locker(process_lock_);
if (try_locker.Locked()) { if (try_locker.Locked()) {
if (source_number_of_channels_ != output_bus->NumberOfChannels()) {
output_bus->Zero();
return;
}
GetAudioSourceProvider()->ProvideInput(output_bus, number_of_frames); GetAudioSourceProvider()->ProvideInput(output_bus, number_of_frames);
} else { } else {
// We failed to acquire the lock. // We failed to acquire the lock.
......
...@@ -72,9 +72,10 @@ class MediaStreamAudioSourceHandler final : public AudioHandler { ...@@ -72,9 +72,10 @@ class MediaStreamAudioSourceHandler final : public AudioHandler {
std::unique_ptr<AudioSourceProvider> audio_source_provider_; std::unique_ptr<AudioSourceProvider> audio_source_provider_;
// Protects |source_number_of_channels_|.
Mutex process_lock_; Mutex process_lock_;
unsigned source_number_of_channels_; unsigned source_number_of_channels_ = 0;
}; };
class MediaStreamAudioSourceNode final class MediaStreamAudioSourceNode final
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment