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

4 files changed, 73 insertions, 88 deletions

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 @@ struct audio_track_cblk_t
 
                 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 @@ struct audio_track_cblk_t
                 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 @@ status_t AudioRecord::start()
             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 @@ status_t AudioRecord::openRecord_l(
     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 @@ status_t AudioRecord::obtainBuffer(Buffer* audioBuffer, int32_t waitCount)
                     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 @@ bool AudioRecord::processAudioBuffer(const sp<ClientRecordThread>& thread)
     // 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 AudioRecord::restoreRecord_l(audio_track_cblk_t*& cblk)
 {
     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 @@ status_t AudioRecord::restoreRecord_l(audio_track_cblk_t*& cblk)
         }
 
         // 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 @@ void AudioTrack::start()
         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 @@ void AudioTrack::start()
             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 @@ status_t AudioTrack::setPosition(uint32_t position)
     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 @@ status_t AudioTrack::createTrack_l(
     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 @@ status_t AudioTrack::obtainBuffer(Buffer* audioBuffer, int32_t waitCount)
                         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 @@ create_new_track:
 
     // 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 @@ bool AudioTrack::processAudioBuffer(const sp<AudioTrackThread>& thread)
     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 AudioTrack::restoreTrack_l(audio_track_cblk_t*& cblk, bool fromStart)
 {
     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 @@ status_t AudioTrack::restoreTrack_l(audio_track_cblk_t*& cblk, bool fromStart)
         }
 
         // 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 @@ void AudioTrack::AudioTrackThread::onFirstRef()
 
 // =========================================================================
 
+
 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 @@ uint32_t audio_track_cblk_t::stepUser(uint32_t frameCount)
 
     // 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 @@ uint32_t audio_track_cblk_t::stepUser(uint32_t frameCount)
 
 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 @@ uint32_t audio_track_cblk_t::framesReady()
         if (u < loopEnd) {
             return u - s;
         } else {
-            Mutex::Autolock _l(lock);
+            // 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) {
-                return (loopEnd - loopStart)*loopCount + u - s;
-            } else {
-                return UINT_MAX;
+                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 @@ uint32_t AudioFlinger::MixerThread::prepareTracks_l(const SortedVector< wp<Track
                     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 @@ void AudioFlinger::MixerThread::invalidateTracks(int streamType)
     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 @@ bool AudioFlinger::PlaybackThread::Track::isReady() const {
 
     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 @@ void AudioFlinger::PlaybackThread::Track::flush()
         // 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 @@ void AudioFlinger::RecordThread::RecordTrack::stop()
     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);
     }
 }