New fix for issue 4111672: control block flags
The first fix (commit 913af0b4) is problematic because it makes threads
in mediaserver process block on the cblk mutex. This is not permitted
as it can cause audio to skip or worse have a malicious application
prevent all audio playback by keeping the mutex locked.
The fix consists in using atomic operations when modifying the control
block flags.
Also fixed audio_track_cblk_t::framesReady() so that it doesn't block
when called from AudioFlinger (only applies when a loop is active).
Change-Id: Ibf0abb562ced3e9f64118afdd5036854bb959428
diff --git a/include/private/media/AudioTrackShared.h b/include/private/media/AudioTrackShared.h
index 4610135..1827c3e 100644
--- a/include/private/media/AudioTrackShared.h
+++ b/include/private/media/AudioTrackShared.h
@@ -83,13 +83,12 @@
uint8_t frameSize;
uint8_t channelCount;
- uint16_t flags;
-
uint16_t bufferTimeoutMs; // Maximum cumulated timeout before restarting audioflinger
- uint16_t waitTimeMs; // Cumulated wait time
+ uint16_t waitTimeMs; // Cumulated wait time
uint16_t sendLevel;
- uint16_t reserved;
+ volatile int32_t flags;
+
// Cache line boundary (32 bytes)
audio_track_cblk_t();
uint32_t stepUser(uint32_t frameCount);
@@ -98,6 +97,7 @@
uint32_t framesAvailable();
uint32_t framesAvailable_l();
uint32_t framesReady();
+ bool tryLock();
};
diff --git a/media/libmedia/AudioRecord.cpp b/media/libmedia/AudioRecord.cpp
index cee1c75..5d74a0a 100644
--- a/media/libmedia/AudioRecord.cpp
+++ b/media/libmedia/AudioRecord.cpp
@@ -35,6 +35,7 @@
#include <binder/Parcel.h>
#include <binder/IPCThreadState.h>
#include <utils/Timers.h>
+#include <utils/Atomic.h>
#define LIKELY( exp ) (__builtin_expect( (exp) != 0, true ))
#define UNLIKELY( exp ) (__builtin_expect( (exp) != 0, false ))
@@ -299,7 +300,7 @@
ret = mAudioRecord->start();
cblk->lock.lock();
if (ret == DEAD_OBJECT) {
- cblk->flags |= CBLK_INVALID_MSK;
+ android_atomic_or(CBLK_INVALID_ON, &cblk->flags);
}
}
if (cblk->flags & CBLK_INVALID_MSK) {
@@ -467,7 +468,7 @@
mCblkMemory = cblk;
mCblk = static_cast<audio_track_cblk_t*>(cblk->pointer());
mCblk->buffers = (char*)mCblk + sizeof(audio_track_cblk_t);
- mCblk->flags &= ~CBLK_DIRECTION_MSK;
+ android_atomic_and(~CBLK_DIRECTION_MSK, &mCblk->flags);
mCblk->bufferTimeoutMs = MAX_RUN_TIMEOUT_MS;
mCblk->waitTimeMs = 0;
return NO_ERROR;
@@ -522,7 +523,7 @@
result = mAudioRecord->start();
cblk->lock.lock();
if (result == DEAD_OBJECT) {
- cblk->flags |= CBLK_INVALID_MSK;
+ android_atomic_or(CBLK_INVALID_ON, &cblk->flags);
create_new_record:
result = AudioRecord::restoreRecord_l(cblk);
}
@@ -722,12 +723,8 @@
// Manage overrun callback
if (mActive && (cblk->framesAvailable() == 0)) {
LOGV("Overrun user: %x, server: %x, flags %04x", cblk->user, cblk->server, cblk->flags);
- AutoMutex _l(cblk->lock);
- if ((cblk->flags & CBLK_UNDERRUN_MSK) == CBLK_UNDERRUN_OFF) {
- cblk->flags |= CBLK_UNDERRUN_ON;
- cblk->lock.unlock();
+ if (!(android_atomic_or(CBLK_UNDERRUN_ON, &cblk->flags) & CBLK_UNDERRUN_MSK)) {
mCbf(EVENT_OVERRUN, mUserData, 0);
- cblk->lock.lock();
}
}
@@ -746,10 +743,8 @@
{
status_t result;
- if (!(cblk->flags & CBLK_RESTORING_MSK)) {
+ if (!(android_atomic_or(CBLK_RESTORING_ON, &cblk->flags) & CBLK_RESTORING_MSK)) {
LOGW("dead IAudioRecord, creating a new one");
-
- cblk->flags |= CBLK_RESTORING_ON;
// signal old cblk condition so that other threads waiting for available buffers stop
// waiting now
cblk->cv.broadcast();
@@ -768,10 +763,8 @@
}
// signal old cblk condition for other threads waiting for restore completion
- cblk->lock.lock();
- cblk->flags |= CBLK_RESTORED_MSK;
+ android_atomic_or(CBLK_RESTORED_ON, &cblk->flags);
cblk->cv.broadcast();
- cblk->lock.unlock();
} else {
if (!(cblk->flags & CBLK_RESTORED_MSK)) {
LOGW("dead IAudioRecord, waiting for a new one to be created");
diff --git a/media/libmedia/AudioTrack.cpp b/media/libmedia/AudioTrack.cpp
index fb2ee0f..66e11d2 100644
--- a/media/libmedia/AudioTrack.cpp
+++ b/media/libmedia/AudioTrack.cpp
@@ -35,6 +35,7 @@
#include <binder/Parcel.h>
#include <binder/IPCThreadState.h>
#include <utils/Timers.h>
+#include <utils/Atomic.h>
#define LIKELY( exp ) (__builtin_expect( (exp) != 0, true ))
#define UNLIKELY( exp ) (__builtin_expect( (exp) != 0, false ))
@@ -332,7 +333,7 @@
cblk->lock.lock();
cblk->bufferTimeoutMs = MAX_STARTUP_TIMEOUT_MS;
cblk->waitTimeMs = 0;
- cblk->flags &= ~CBLK_DISABLED_ON;
+ android_atomic_and(~CBLK_DISABLED_ON, &cblk->flags);
if (t != 0) {
t->run("AudioTrackThread", THREAD_PRIORITY_AUDIO_CLIENT);
} else {
@@ -345,7 +346,7 @@
status = mAudioTrack->start();
cblk->lock.lock();
if (status == DEAD_OBJECT) {
- cblk->flags |= CBLK_INVALID_MSK;
+ android_atomic_or(CBLK_INVALID_ON, &cblk->flags);
}
}
if (cblk->flags & CBLK_INVALID_MSK) {
@@ -636,7 +637,7 @@
if (position > mCblk->user) return BAD_VALUE;
mCblk->server = position;
- mCblk->flags |= CBLK_FORCEREADY_ON;
+ android_atomic_or(CBLK_FORCEREADY_ON, &mCblk->flags);
return NO_ERROR;
}
@@ -793,7 +794,7 @@
mCblkMemory.clear();
mCblkMemory = cblk;
mCblk = static_cast<audio_track_cblk_t*>(cblk->pointer());
- mCblk->flags |= CBLK_DIRECTION_OUT;
+ android_atomic_or(CBLK_DIRECTION_OUT, &mCblk->flags);
if (sharedBuffer == 0) {
mCblk->buffers = (char*)mCblk + sizeof(audio_track_cblk_t);
} else {
@@ -873,7 +874,7 @@
result = mAudioTrack->start();
cblk->lock.lock();
if (result == DEAD_OBJECT) {
- cblk->flags |= CBLK_INVALID_MSK;
+ android_atomic_or(CBLK_INVALID_ON, &cblk->flags);
create_new_track:
result = restoreTrack_l(cblk, false);
}
@@ -900,14 +901,9 @@
// restart track if it was disabled by audioflinger due to previous underrun
if (mActive && (cblk->flags & CBLK_DISABLED_MSK)) {
- AutoMutex _l(cblk->lock);
- if (mActive && (cblk->flags & CBLK_DISABLED_MSK)) {
- cblk->flags &= ~CBLK_DISABLED_ON;
- cblk->lock.unlock();
- LOGW("obtainBuffer() track %p disabled, restarting", this);
- mAudioTrack->start();
- cblk->lock.lock();
- }
+ android_atomic_and(~CBLK_DISABLED_ON, &cblk->flags);
+ LOGW("obtainBuffer() track %p disabled, restarting", this);
+ mAudioTrack->start();
}
cblk->waitTimeMs = 0;
@@ -1026,17 +1022,13 @@
mLock.unlock();
// Manage underrun callback
- if (mActive && (cblk->framesReady() == 0)) {
+ if (mActive && (cblk->framesAvailable() == cblk->frameCount)) {
LOGV("Underrun user: %x, server: %x, flags %04x", cblk->user, cblk->server, cblk->flags);
- AutoMutex _l(cblk->lock);
- if ((cblk->flags & CBLK_UNDERRUN_MSK) == CBLK_UNDERRUN_OFF) {
- cblk->flags |= CBLK_UNDERRUN_ON;
- cblk->lock.unlock();
+ if (!(android_atomic_or(CBLK_UNDERRUN_ON, &cblk->flags) & CBLK_UNDERRUN_MSK)) {
mCbf(EVENT_UNDERRUN, mUserData, 0);
if (cblk->server == cblk->frameCount) {
mCbf(EVENT_BUFFER_END, mUserData, 0);
}
- cblk->lock.lock();
if (mSharedBuffer != 0) return false;
}
}
@@ -1150,12 +1142,10 @@
{
status_t result;
- if (!(cblk->flags & CBLK_RESTORING_MSK)) {
+ if (!(android_atomic_or(CBLK_RESTORING_ON, &cblk->flags) & CBLK_RESTORING_MSK)) {
LOGW("dead IAudioTrack, creating a new one from %s",
fromStart ? "start()" : "obtainBuffer()");
- cblk->flags |= CBLK_RESTORING_ON;
-
// signal old cblk condition so that other threads waiting for available buffers stop
// waiting now
cblk->cv.broadcast();
@@ -1198,10 +1188,8 @@
}
// signal old cblk condition for other threads waiting for restore completion
- cblk->lock.lock();
- cblk->flags |= CBLK_RESTORED_MSK;
+ android_atomic_or(CBLK_RESTORED_ON, &cblk->flags);
cblk->cv.broadcast();
- cblk->lock.unlock();
} else {
if (!(cblk->flags & CBLK_RESTORED_MSK)) {
LOGW("dead IAudioTrack, waiting for a new one");
@@ -1275,11 +1263,12 @@
// =========================================================================
+
audio_track_cblk_t::audio_track_cblk_t()
: lock(Mutex::SHARED), cv(Condition::SHARED), user(0), server(0),
userBase(0), serverBase(0), buffers(0), frameCount(0),
loopStart(UINT_MAX), loopEnd(UINT_MAX), loopCount(0), volumeLR(0),
- flags(0), sendLevel(0)
+ sendLevel(0), flags(0)
{
}
@@ -1307,10 +1296,7 @@
// Clear flow control error condition as new data has been written/read to/from buffer.
if (flags & CBLK_UNDERRUN_MSK) {
- AutoMutex _l(lock);
- if (flags & CBLK_UNDERRUN_MSK) {
- flags &= ~CBLK_UNDERRUN_MSK;
- }
+ android_atomic_and(~CBLK_UNDERRUN_MSK, &flags);
}
return u;
@@ -1318,18 +1304,8 @@
bool audio_track_cblk_t::stepServer(uint32_t frameCount)
{
- // the code below simulates lock-with-timeout
- // we MUST do this to protect the AudioFlinger server
- // as this lock is shared with the client.
- status_t err;
-
- err = lock.tryLock();
- if (err == -EBUSY) { // just wait a bit
- usleep(1000);
- err = lock.tryLock();
- }
- if (err != NO_ERROR) {
- // probably, the client just died.
+ if (!tryLock()) {
+ LOGW("stepServer() could not lock cblk");
return false;
}
@@ -1406,18 +1382,42 @@
if (u < loopEnd) {
return u - s;
} else {
- Mutex::Autolock _l(lock);
- if (loopCount >= 0) {
- return (loopEnd - loopStart)*loopCount + u - s;
- } else {
- return UINT_MAX;
+ // do not block on mutex shared with client on AudioFlinger side
+ if (!tryLock()) {
+ LOGW("framesReady() could not lock cblk");
+ return 0;
}
+ uint32_t frames = UINT_MAX;
+ if (loopCount >= 0) {
+ frames = (loopEnd - loopStart)*loopCount + u - s;
+ }
+ lock.unlock();
+ return frames;
}
} else {
return s - u;
}
}
+bool audio_track_cblk_t::tryLock()
+{
+ // the code below simulates lock-with-timeout
+ // we MUST do this to protect the AudioFlinger server
+ // as this lock is shared with the client.
+ status_t err;
+
+ err = lock.tryLock();
+ if (err == -EBUSY) { // just wait a bit
+ usleep(1000);
+ err = lock.tryLock();
+ }
+ if (err != NO_ERROR) {
+ // probably, the client just died.
+ return false;
+ }
+ return true;
+}
+
// -------------------------------------------------------------------------
}; // namespace android
diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp
index 2702242..e27a10f 100644
--- a/services/audioflinger/AudioFlinger.cpp
+++ b/services/audioflinger/AudioFlinger.cpp
@@ -31,6 +31,7 @@
#include <binder/IPCThreadState.h>
#include <utils/String16.h>
#include <utils/threads.h>
+#include <utils/Atomic.h>
#include <cutils/properties.h>
@@ -1738,10 +1739,7 @@
LOGV("BUFFER TIMEOUT: remove(%d) from active list on thread %p", track->name(), this);
tracksToRemove->add(track);
// indicate to client process that the track was disabled because of underrun
- {
- AutoMutex _l(cblk->lock);
- cblk->flags |= CBLK_DISABLED_ON;
- }
+ android_atomic_or(CBLK_DISABLED_ON, &cblk->flags);
} else if (mixerStatus != MIXER_TRACKS_READY) {
mixerStatus = MIXER_TRACKS_ENABLED;
}
@@ -1790,8 +1788,7 @@
for (size_t i = 0; i < size; i++) {
sp<Track> t = mTracks[i];
if (t->type() == streamType) {
- AutoMutex _lcblk(t->mCblk->lock);
- t->mCblk->flags |= CBLK_INVALID_ON;
+ android_atomic_or(CBLK_INVALID_ON, &t->mCblk->flags);
t->mCblk->cv.signal();
}
}
@@ -2950,9 +2947,8 @@
if (mCblk->framesReady() >= mCblk->frameCount ||
(mCblk->flags & CBLK_FORCEREADY_MSK)) {
- AutoMutex _l(mCblk->lock);
mFillingUpStatus = FS_FILLED;
- mCblk->flags &= ~CBLK_FORCEREADY_MSK;
+ android_atomic_and(~CBLK_FORCEREADY_MSK, &mCblk->flags);
return true;
}
return false;
@@ -3066,26 +3062,25 @@
// STOPPED state
mState = STOPPED;
- // NOTE: reset() will reset cblk->user and cblk->server with
- // the risk that at the same time, the AudioMixer is trying to read
- // data. In this case, getNextBuffer() would return a NULL pointer
- // as audio buffer => the AudioMixer code MUST always test that pointer
- // returned by getNextBuffer() is not NULL!
- reset();
+ // do not reset the track if it is still in the process of being stopped or paused.
+ // this will be done by prepareTracks_l() when the track is stopped.
+ PlaybackThread *playbackThread = (PlaybackThread *)thread.get();
+ if (playbackThread->mActiveTracks.indexOf(this) < 0) {
+ reset();
+ }
}
}
void AudioFlinger::PlaybackThread::Track::reset()
{
- AutoMutex _l(mCblk->lock);
// Do not reset twice to avoid discarding data written just after a flush and before
// the audioflinger thread detects the track is stopped.
if (!mResetDone) {
TrackBase::reset();
// Force underrun condition to avoid false underrun callback until first data is
// written to buffer
- mCblk->flags |= CBLK_UNDERRUN_ON;
- mCblk->flags &= ~CBLK_FORCEREADY_MSK;
+ android_atomic_and(~CBLK_FORCEREADY_MSK, &mCblk->flags);
+ android_atomic_or(CBLK_UNDERRUN_ON, &mCblk->flags);
mFillingUpStatus = FS_FILLING;
mResetDone = true;
}
@@ -3211,13 +3206,10 @@
if (thread != 0) {
RecordThread *recordThread = (RecordThread *)thread.get();
recordThread->stop(this);
- {
- AutoMutex _l(mCblk->lock);
- TrackBase::reset();
- // Force overerrun condition to avoid false overrun callback until first data is
- // read from buffer
- mCblk->flags |= CBLK_UNDERRUN_ON;
- }
+ TrackBase::reset();
+ // Force overerrun condition to avoid false overrun callback until first data is
+ // read from buffer
+ android_atomic_or(CBLK_UNDERRUN_ON, &mCblk->flags);
}
}