Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit ae29b763 authored by Eric Laurent's avatar Eric Laurent
Browse files

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
parent 42bc0e94

include/private/media/AudioTrackShared.h

+4 −4
Original line number Diff line number Diff line
@@ -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();

};

media/libmedia/AudioRecord.cpp

+7 −14
Original line number Diff line number Diff line
@@ -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");

media/libmedia/AudioTrack.cpp

+46 −46
Original line number Diff line number Diff line
@@ -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();

        android_atomic_and(~CBLK_DISABLED_ON, &cblk->flags);

        LOGW("obtainBuffer() track %p disabled, restarting", this);

        mAudioTrack->start();

            cblk->lock.lock();

        }

    }



    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

services/audioflinger/AudioFlinger.cpp

+16 −24
Original line number Diff line number Diff line
@@ -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!

        // 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;

        }

        android_atomic_or(CBLK_UNDERRUN_ON, &mCblk->flags);

    }

}