From 084077feb6b8c961adcbe77b2bd76601ca54e534 Mon Sep 17 00:00:00 2001 From: Phil Burk Date: Wed, 7 Apr 2021 06:30:50 +0000 Subject: aaudio: prevent deadlock when stop() calls disconnect() Move all calls to send the timestamp into the one timestamp thread. There was a clear code path that could lead to a deadlock. If the call to get the timestamp from the HAL returned an unexpected error code then it would call disconnect(). If that happened below the call to stop() then the deadlock would occur. The sequence of calls was AAudioServiceStreamBase::stop() which locked mLock, then called AAudioServiceStreamBase::stop_l(), which called AAudioServiceStreamBase:sendCurrentTimeStamp(), which called AAudioServiceStreamMMAP::getFreeRunningPosition(), which called disconnect(), which locked mLock AGAIN. It is not clear what would trigger the error return from the HAL but a routing change may be involved. The bug was discovered during stress tests and we do not have a clear repro case. Bug: 182852602 Bug: 153358911 Test: atest CtsNativeMediaAAudioTestCases Change-Id: I575f75ece9b459e7412bca293d7338babe76b3a7 Merged-In: I575f75ece9b459e7412bca293d7338babe76b3a7 (cherry picked from commit 45da1b7e3231bf3475cb9ca1a2243a27355c0466) (cherry picked from commit 9dd928e100d38c42f68c04c01f09fa8c8cb606d3) --- services/oboeservice/AAudioServiceStreamBase.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/services/oboeservice/AAudioServiceStreamBase.cpp b/services/oboeservice/AAudioServiceStreamBase.cpp index 663dae29cc..32cc7ca070 100644 --- a/services/oboeservice/AAudioServiceStreamBase.cpp +++ b/services/oboeservice/AAudioServiceStreamBase.cpp @@ -293,10 +293,6 @@ aaudio_result_t AAudioServiceStreamBase::pause_l() { .set(AMEDIAMETRICS_PROP_STATUS, (int32_t)result) .record(); }); - // Send it now because the timestamp gets rounded up when stopStream() is called below. - // Also we don't need the timestamps while we are shutting down. - sendCurrentTimestamp(); - result = stopTimestampThread(); if (result != AAUDIO_OK) { disconnect_l(); @@ -342,9 +338,6 @@ aaudio_result_t AAudioServiceStreamBase::stop_l() { setState(AAUDIO_STREAM_STATE_STOPPING); - // Send it now because the timestamp gets rounded up when stopStream() is called below. - // Also we don't need the timestamps while we are shutting down. - sendCurrentTimestamp(); // warning - this calls a virtual function result = stopTimestampThread(); if (result != AAUDIO_OK) { disconnect_l(); @@ -410,10 +403,11 @@ void AAudioServiceStreamBase::run() { timestampScheduler.start(AudioClock::getNanoseconds()); int64_t nextTime = timestampScheduler.nextAbsoluteTime(); int32_t loopCount = 0; + aaudio_result_t result = AAUDIO_OK; while(mThreadEnabled.load()) { loopCount++; if (AudioClock::getNanoseconds() >= nextTime) { - aaudio_result_t result = sendCurrentTimestamp(); + result = sendCurrentTimestamp(); if (result != AAUDIO_OK) { ALOGE("%s() timestamp thread got result = %d", __func__, result); break; @@ -425,6 +419,11 @@ void AAudioServiceStreamBase::run() { AudioClock::sleepUntilNanoTime(nextTime); } } + // This was moved from the calls in stop_l() and pause_l(), which could cause a deadlock + // if it resulted in a call to disconnect. + if (result == AAUDIO_OK) { + (void) sendCurrentTimestamp(); + } ALOGD("%s() %s exiting after %d loops <<<<<<<<<<<<<< TIMESTAMPS", __func__, getTypeText(), loopCount); } -- cgit v1.2.3 From cc2165840d524bb9553f9d73d1904633d20100a2 Mon Sep 17 00:00:00 2001 From: Phil Burk Date: Thu, 15 Apr 2021 23:40:25 +0000 Subject: aaudio: unlock when joining the timestamp thread This will prevent a deadlock in case the timestamp thread tries to acquire the same lock. Bug: 182852602 Bug: 153358911 Test: plug and unplug headphones while playing Change-Id: I625d191906c7e280f3a223f476716ef17b9098ea Merged-In: I625d191906c7e280f3a223f476716ef17b9098ea (cherry picked from commit 5f6fda778bf35be4cd67363ca0fe40cf710364c3) --- services/oboeservice/AAudioServiceStreamBase.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/services/oboeservice/AAudioServiceStreamBase.cpp b/services/oboeservice/AAudioServiceStreamBase.cpp index 32cc7ca070..30deb5df9c 100644 --- a/services/oboeservice/AAudioServiceStreamBase.cpp +++ b/services/oboeservice/AAudioServiceStreamBase.cpp @@ -338,7 +338,12 @@ aaudio_result_t AAudioServiceStreamBase::stop_l() { setState(AAUDIO_STREAM_STATE_STOPPING); + // Temporarily unlock because we are joining the timestamp thread and it may try + // to acquire mLock. + mLock.unlock(); result = stopTimestampThread(); + mLock.lock(); + if (result != AAUDIO_OK) { disconnect_l(); return result; -- cgit v1.2.3