From e5002cedc4908c8d3ef8454d5b2fd658c9f4a973 Mon Sep 17 00:00:00 2001 From: Phil Burk Date: Thu, 12 May 2022 15:34:55 -0700 Subject: [PATCH 1/2] Add API to set sleep time before close. Add: AudioStream::setDelayBeforeCloseMillis(delay) This can be used to avoid use-after-free bugs cause by callbacks running when close is called. Fixes #1500 --- include/oboe/AudioStream.h | 39 ++++++++++++++++++++++ src/aaudio/AudioStreamAAudio.cpp | 9 ++--- src/aaudio/AudioStreamAAudio.h | 9 +++-- src/common/AudioStream.cpp | 9 +++++ src/opensles/AudioInputStreamOpenSLES.cpp | 14 +------- src/opensles/AudioOutputStreamOpenSLES.cpp | 17 +++------- src/opensles/AudioStreamOpenSLES.cpp | 24 +++++++++++++ src/opensles/AudioStreamOpenSLES.h | 18 ++++++---- 8 files changed, 98 insertions(+), 41 deletions(-) diff --git a/include/oboe/AudioStream.h b/include/oboe/AudioStream.h index 9b27e9a03..18fabea73 100644 --- a/include/oboe/AudioStream.h +++ b/include/oboe/AudioStream.h @@ -449,6 +449,29 @@ class AudioStream : public AudioStreamBase { return mErrorCallbackResult; } + + int32_t getDelayBeforeCloseMillis() const { + return mDelayBeforeCloseMillis; + } + + /** + * Set the time to sleep before closing the internal stream. + * + * Sometimes a callback can occur shortly after a stream has been stopped and + * even after a close! If the stream has been closed then the callback + * might access memory that has been freed, which could cause a crash. + * This seems to be more likely in Android P or earlier. + * But it can also occur in later versions. By sleeping, we give time for + * the callback threads to finish. + * + * Note that this only has an effect when OboeGlobals::areWorkaroundsEnabled() is true. + * + * @param delayBeforeCloseMillis time to sleep before close. + */ + void setDelayBeforeCloseMillis(int32_t delayBeforeCloseMillis) { + mDelayBeforeCloseMillis = delayBeforeCloseMillis; + } + protected: /** @@ -510,6 +533,17 @@ class AudioStream : public AudioStreamBase { mDataCallbackEnabled = enabled; } + void calculateDefaultDelayBeforeCloseMillis(); + + /** + * Try to avoid a race condition when closing. + */ + void sleepBeforeClose() { + if (mDelayBeforeCloseMillis > 0) { + usleep(mDelayBeforeCloseMillis * 1000); + } + } + /* * Set a weak_ptr to this stream from the shared_ptr so that we can * later use a shared_ptr in the error callback. @@ -553,6 +587,11 @@ class AudioStream : public AudioStreamBase { */ int32_t mFramesPerBurst = kUnspecified; + // Time to sleep in order to prevent a race condition with a callback after a close(). + // Two milliseconds may be enough but 10 msec is even safer. + static constexpr int kMinDelayBeforeCloseMillis = 10; + int32_t mDelayBeforeCloseMillis = kMinDelayBeforeCloseMillis; + private: // Log the scheduler if it changes. diff --git a/src/aaudio/AudioStreamAAudio.cpp b/src/aaudio/AudioStreamAAudio.cpp index 200d05b02..abc86944c 100644 --- a/src/aaudio/AudioStreamAAudio.cpp +++ b/src/aaudio/AudioStreamAAudio.cpp @@ -328,6 +328,8 @@ Result AudioStreamAAudio::open() { static_cast(mFormat), static_cast(mSampleRate), static_cast(mBufferCapacityInFrames)); + calculateDefaultDelayBeforeCloseMillis(); + error2: mLibLoader->builder_delete(aaudioBuilder); LOGD("AudioStreamAAudio.open: AAudioStream_Open() returned %s", @@ -355,12 +357,7 @@ Result AudioStreamAAudio::close() { // Make sure we are really stopped. Do it under mLock // so another thread cannot call requestStart() right before the close. requestStop_l(stream); - // Sometimes a callback can occur shortly after a stream has been stopped and - // even after a close! If the stream has been closed then the callback - // can access memory that has been freed. That causes a crash. - // This seems to be more likely in Android P or earlier. - // But it can also occur in later versions. - usleep(kDelayBeforeCloseMillis * 1000); + sleepBeforeClose(); } return static_cast(mLibLoader->stream_close(stream)); } else { diff --git a/src/aaudio/AudioStreamAAudio.h b/src/aaudio/AudioStreamAAudio.h index e71efe85c..2517199a2 100644 --- a/src/aaudio/AudioStreamAAudio.h +++ b/src/aaudio/AudioStreamAAudio.h @@ -117,9 +117,12 @@ class AudioStreamAAudio : public AudioStream { */ void launchStopThread(); - // Time to sleep in order to prevent a race condition with a callback after a close(). - // Two milliseconds may be enough but 10 msec is even safer. - static constexpr int kDelayBeforeCloseMillis = 10; +public: + int32_t getMDelayBeforeCloseMillis() const; + + void setDelayBeforeCloseMillis(int32_t mDelayBeforeCloseMillis); + +private: std::atomic mCallbackThreadEnabled; std::atomic mStopThreadAllowed{false}; diff --git a/src/common/AudioStream.cpp b/src/common/AudioStream.cpp index 66d071172..9065e2d76 100644 --- a/src/common/AudioStream.cpp +++ b/src/common/AudioStream.cpp @@ -196,4 +196,13 @@ ResultWithValue AudioStream::getTimestamp(clockid_t clockId) { } } +void AudioStream::calculateDefaultDelayBeforeCloseMillis() { + // Calculate delay time before close based on burst duration. + // Start with a burst duration then add 1 msec as a safety margin. + mDelayBeforeCloseMillis = std::max(kMinDelayBeforeCloseMillis, + 1 + ((mFramesPerBurst * 1000) / getSampleRate())); + LOGD("calculateDefaultDelayBeforeCloseMillis() default = %d", + static_cast(mDelayBeforeCloseMillis)); +} + } // namespace oboe diff --git a/src/opensles/AudioInputStreamOpenSLES.cpp b/src/opensles/AudioInputStreamOpenSLES.cpp index b790429bc..6a0df94e6 100644 --- a/src/opensles/AudioInputStreamOpenSLES.cpp +++ b/src/opensles/AudioInputStreamOpenSLES.cpp @@ -195,23 +195,11 @@ Result AudioInputStreamOpenSLES::open() { goto error; } - result = AudioStreamOpenSLES::registerBufferQueueCallback(); + result = finishCommonOpen(configItf); if (SL_RESULT_SUCCESS != result) { goto error; } - result = updateStreamParameters(configItf); - if (SL_RESULT_SUCCESS != result) { - goto error; - } - - oboeResult = configureBufferSizes(mSampleRate); - if (Result::OK != oboeResult) { - goto error; - } - - allocateFifo(); - setState(StreamState::Open); return Result::OK; diff --git a/src/opensles/AudioOutputStreamOpenSLES.cpp b/src/opensles/AudioOutputStreamOpenSLES.cpp index 2e201a7e5..e567b8af9 100644 --- a/src/opensles/AudioOutputStreamOpenSLES.cpp +++ b/src/opensles/AudioOutputStreamOpenSLES.cpp @@ -214,23 +214,11 @@ Result AudioOutputStreamOpenSLES::open() { goto error; } - result = AudioStreamOpenSLES::registerBufferQueueCallback(); + result = finishCommonOpen(configItf); if (SL_RESULT_SUCCESS != result) { goto error; } - result = updateStreamParameters(configItf); - if (SL_RESULT_SUCCESS != result) { - goto error; - } - - oboeResult = configureBufferSizes(mSampleRate); - if (Result::OK != oboeResult) { - goto error; - } - - allocateFifo(); - setState(StreamState::Open); return Result::OK; @@ -252,6 +240,9 @@ Result AudioOutputStreamOpenSLES::close() { result = Result::ErrorClosed; } else { (void) requestPause_l(); + if (OboeGlobals::areWorkaroundsEnabled()) { + sleepBeforeClose(); + } // invalidate any interfaces mPlayInterface = nullptr; result = AudioStreamOpenSLES::close_l(); diff --git a/src/opensles/AudioStreamOpenSLES.cpp b/src/opensles/AudioStreamOpenSLES.cpp index 3b8d26c3e..83cb910d8 100644 --- a/src/opensles/AudioStreamOpenSLES.cpp +++ b/src/opensles/AudioStreamOpenSLES.cpp @@ -99,6 +99,30 @@ Result AudioStreamOpenSLES::open() { return Result::OK; } + +SLresult AudioStreamOpenSLES::finishCommonOpen(SLAndroidConfigurationItf configItf) { + SLresult result = registerBufferQueueCallback(); + if (SL_RESULT_SUCCESS != result) { + return result; + } + + result = updateStreamParameters(configItf); + if (SL_RESULT_SUCCESS != result) { + return result; + } + + Result oboeResult = configureBufferSizes(mSampleRate); + if (Result::OK != oboeResult) { + return (SLresult) oboeResult; + } + + allocateFifo(); + + calculateDefaultDelayBeforeCloseMillis(); + + return SL_RESULT_SUCCESS; +} + Result AudioStreamOpenSLES::configureBufferSizes(int32_t sampleRate) { LOGD("AudioStreamOpenSLES:%s(%d) initial mFramesPerBurst = %d, mFramesPerCallback = %d", __func__, sampleRate, mFramesPerBurst, mFramesPerCallback); diff --git a/src/opensles/AudioStreamOpenSLES.h b/src/opensles/AudioStreamOpenSLES.h index b00b62873..82281428d 100644 --- a/src/opensles/AudioStreamOpenSLES.h +++ b/src/opensles/AudioStreamOpenSLES.h @@ -78,6 +78,14 @@ class AudioStreamOpenSLES : public AudioStreamBuffered { protected: + /** + * Finish setting up the stream. Common for INPUT and OUTPUT. + * + * @param configItf + * @return SL_RESULT_SUCCESS if OK. + */ + SLresult finishCommonOpen(SLAndroidConfigurationItf configItf); + // This must be called under mLock. Result close_l(); @@ -88,21 +96,15 @@ class AudioStreamOpenSLES : public AudioStreamBuffered { static SLuint32 getDefaultByteOrder(); - SLresult registerBufferQueueCallback(); - int32_t getBufferDepth(SLAndroidSimpleBufferQueueItf bq); SLresult enqueueCallbackBuffer(SLAndroidSimpleBufferQueueItf bq); SLresult configurePerformanceMode(SLAndroidConfigurationItf configItf); - SLresult updateStreamParameters(SLAndroidConfigurationItf configItf); - PerformanceMode convertPerformanceMode(SLuint32 openslMode) const; SLuint32 convertPerformanceMode(PerformanceMode oboeMode) const; - Result configureBufferSizes(int32_t sampleRate); - void logUnsupportedAttributes(); /** @@ -123,6 +125,10 @@ class AudioStreamOpenSLES : public AudioStreamBuffered { MonotonicCounter mPositionMillis; // for tracking OpenSL ES service position private: + SLresult registerBufferQueueCallback(); + SLresult updateStreamParameters(SLAndroidConfigurationItf configItf); + Result configureBufferSizes(int32_t sampleRate); + std::unique_ptr mCallbackBuffer[kBufferQueueLength]; int mCallbackBufferIndex = 0; std::atomic mState{StreamState::Uninitialized}; From 01fdb61d70c16a1200fe97f06f6511b38c25e7fd Mon Sep 17 00:00:00 2001 From: Phil Burk Date: Fri, 5 Aug 2022 13:15:09 -0700 Subject: [PATCH 2/2] Add test for delay before close. Bump version to 1.6.3 --- include/oboe/AudioStream.h | 4 ++ include/oboe/Version.h | 2 +- src/opensles/AudioInputStreamOpenSLES.cpp | 3 ++ tests/testStreamClosedMethods.cpp | 63 +++++++++++++++++++++++ 4 files changed, 71 insertions(+), 1 deletion(-) diff --git a/include/oboe/AudioStream.h b/include/oboe/AudioStream.h index 18fabea73..c349d6286 100644 --- a/include/oboe/AudioStream.h +++ b/include/oboe/AudioStream.h @@ -533,6 +533,10 @@ class AudioStream : public AudioStreamBase { mDataCallbackEnabled = enabled; } + /** + * This should only be called as a stream is being opened. + * Otherwise we might override setDelayBeforeCloseMillis(). + */ void calculateDefaultDelayBeforeCloseMillis(); /** diff --git a/include/oboe/Version.h b/include/oboe/Version.h index 95b5046ab..446e75f2b 100644 --- a/include/oboe/Version.h +++ b/include/oboe/Version.h @@ -37,7 +37,7 @@ #define OBOE_VERSION_MINOR 6 // Type: 16-bit unsigned int. Min value: 0 Max value: 65535. See below for description. -#define OBOE_VERSION_PATCH 2 +#define OBOE_VERSION_PATCH 3 #define OBOE_STRINGIFY(x) #x #define OBOE_TOSTRING(x) OBOE_STRINGIFY(x) diff --git a/src/opensles/AudioInputStreamOpenSLES.cpp b/src/opensles/AudioInputStreamOpenSLES.cpp index 6a0df94e6..f4c4b9976 100644 --- a/src/opensles/AudioInputStreamOpenSLES.cpp +++ b/src/opensles/AudioInputStreamOpenSLES.cpp @@ -216,6 +216,9 @@ Result AudioInputStreamOpenSLES::close() { result = Result::ErrorClosed; } else { (void) requestStop_l(); + if (OboeGlobals::areWorkaroundsEnabled()) { + sleepBeforeClose(); + } // invalidate any interfaces mRecordInterface = nullptr; result = AudioStreamOpenSLES::close_l(); diff --git a/tests/testStreamClosedMethods.cpp b/tests/testStreamClosedMethods.cpp index b4865901a..a75e8aeab 100644 --- a/tests/testStreamClosedMethods.cpp +++ b/tests/testStreamClosedMethods.cpp @@ -50,6 +50,43 @@ class StreamClosedReturnValues : public ::testing::Test { return (s == StreamState::Closed); } + static int64_t getNanoseconds() { + struct timespec time; + int result = clock_gettime(CLOCK_MONOTONIC, &time); + if (result < 0) { + return result; + } + return (time.tv_sec * (int64_t)1e9) + time.tv_nsec; + } + + int32_t mElapsedTimeMillis = 0; // used for passing back a value from a test function. + // ASSERT_* requires a void return type. + void measureCloseTime(int32_t delayMillis) { + ASSERT_TRUE(openStream()); + mStream->setDelayBeforeCloseMillis(delayMillis); + ASSERT_EQ(delayMillis, mStream->getDelayBeforeCloseMillis()); + // Measure time it takes to close. + int64_t startTimeMillis = getNanoseconds() / 1e6; + ASSERT_TRUE(closeStream()); + int64_t stopTimeMillis = getNanoseconds() / 1e6; + int32_t elapsedTimeMillis = (int32_t)(stopTimeMillis - startTimeMillis); + ASSERT_GE(elapsedTimeMillis, delayMillis); + mElapsedTimeMillis = elapsedTimeMillis; + } + + void testDelayBeforeClose() { + const int32_t delayMillis = 100; + measureCloseTime(0); + int32_t elapsedTimeMillis1 = mElapsedTimeMillis; + // Do it again with a longer sleep using setDelayBeforeCloseMillis. + // The increase in elapsed time should match the added delay. + measureCloseTime(delayMillis); + int32_t elapsedTimeMillis2 = mElapsedTimeMillis; + int32_t extraElapsedTime = elapsedTimeMillis2 - elapsedTimeMillis1; + // Expect the additional elapsed time to be close to the added delay. + ASSERT_LE(abs(extraElapsedTime - delayMillis), delayMillis / 5); + } + AudioStreamBuilder mBuilder; AudioStream *mStream = nullptr; @@ -336,3 +373,29 @@ TEST_F(StreamClosedReturnValues, WriteReturnsClosed){ auto r = mStream->write(buffer, 1, 0); ASSERT_EQ(r.error(), Result::ErrorClosed); } + +TEST_F(StreamClosedReturnValues, DelayBeforeCloseInput){ + if (AudioStreamBuilder::isAAudioRecommended()) { + mBuilder.setDirection(Direction::Input); + testDelayBeforeClose(); + } +} + +TEST_F(StreamClosedReturnValues, DelayBeforeCloseOutput){ + if (AudioStreamBuilder::isAAudioRecommended()) { + mBuilder.setDirection(Direction::Output); + testDelayBeforeClose(); + } +} + +TEST_F(StreamClosedReturnValues, DelayBeforeCloseInputOpenSL){ + mBuilder.setAudioApi(AudioApi::OpenSLES); + mBuilder.setDirection(Direction::Input); + testDelayBeforeClose(); +} + +TEST_F(StreamClosedReturnValues, DelayBeforeCloseOutputOpenSL){ + mBuilder.setAudioApi(AudioApi::OpenSLES); + mBuilder.setDirection(Direction::Output); + testDelayBeforeClose(); +}