From 305996c854ee76becf0fad73505f0fe9abde4c5d Mon Sep 17 00:00:00 2001 From: Glenn Kasten Date: Mon, 27 Jan 2020 08:03:37 -0800 Subject: [PATCH] Fix race condition in AudioTrack::releaseBuffer() Ignore releaseBuffer() if it is for a different IAudioTrack than was used for obtainBuffer(). Bug: 136268149 Test: at bug comments #32 and #45 Change-Id: I2e1955c60479edcba6e7d66c98b9faef088b65d9 --- media/libaudioclient/AudioTrack.cpp | 10 ++++++++-- media/libaudioclient/include/media/AudioTrack.h | 8 ++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/media/libaudioclient/AudioTrack.cpp b/media/libaudioclient/AudioTrack.cpp index eda94268d0..d5690fb639 100644 --- a/media/libaudioclient/AudioTrack.cpp +++ b/media/libaudioclient/AudioTrack.cpp @@ -1777,7 +1777,6 @@ status_t AudioTrack::obtainBuffer(Buffer* audioBuffer, const struct timespec *re { // previous and new IAudioTrack sequence numbers are used to detect track re-creation uint32_t oldSequence = 0; - uint32_t newSequence; Proxy::Buffer buffer; status_t status = NO_ERROR; @@ -1794,7 +1793,7 @@ status_t AudioTrack::obtainBuffer(Buffer* audioBuffer, const struct timespec *re { // start of lock scope AutoMutex lock(mLock); - newSequence = mSequence; + uint32_t newSequence = mSequence; // did previous obtainBuffer() fail due to media server death or voluntary invalidation? if (status == DEAD_OBJECT) { // re-create track, unless someone else has already done so @@ -1841,6 +1840,7 @@ status_t AudioTrack::obtainBuffer(Buffer* audioBuffer, const struct timespec *re audioBuffer->frameCount = buffer.mFrameCount; audioBuffer->size = buffer.mFrameCount * mFrameSize; audioBuffer->raw = buffer.mRaw; + audioBuffer->sequence = oldSequence; if (nonContig != NULL) { *nonContig = buffer.mNonContig; } @@ -1864,6 +1864,12 @@ void AudioTrack::releaseBuffer(const Buffer* audioBuffer) buffer.mRaw = audioBuffer->raw; AutoMutex lock(mLock); + if (audioBuffer->sequence != mSequence) { + // This Buffer came from a different IAudioTrack instance, so ignore the releaseBuffer + ALOGD("%s is no-op due to IAudioTrack sequence mismatch %u != %u", + __func__, audioBuffer->sequence, mSequence); + return; + } mReleased += stepCount; mInUnderrun = false; mProxy->releaseBuffer(&buffer); diff --git a/media/libaudioclient/include/media/AudioTrack.h b/media/libaudioclient/include/media/AudioTrack.h index 192d87f965..95cad0a059 100644 --- a/media/libaudioclient/include/media/AudioTrack.h +++ b/media/libaudioclient/include/media/AudioTrack.h @@ -107,6 +107,11 @@ public: int16_t* i16; // signed 16-bit int8_t* i8; // unsigned 8-bit, offset by 0x80 }; // input to obtainBuffer(): unused, output: pointer to buffer + + uint32_t sequence; // IAudioTrack instance sequence number, as of obtainBuffer(). + // It is set by obtainBuffer() and confirmed by releaseBuffer(). + // Not "user-serviceable". + // TODO Consider sp instead, or in addition to this. }; /* As a convenience, if a callback is supplied, a handler thread @@ -692,14 +697,17 @@ public: * frameCount number of [empty slots for] frames requested * size ignored * raw ignored + * sequence ignored * After error return: * frameCount 0 * size 0 * raw undefined + * sequence undefined * After successful return: * frameCount actual number of [empty slots for] frames available, <= number requested * size actual number of bytes available * raw pointer to the buffer + * sequence IAudioTrack instance sequence number, as of obtainBuffer() */ status_t obtainBuffer(Buffer* audioBuffer, int32_t waitCount, size_t *nonContig = NULL);