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

Commit e655fda7 authored by Tomislav Novak's avatar Tomislav Novak
Browse files

libaudioclient: Use atomic_sp in TrackPlayerBase 

TrackPlayerBase::destroy() can race with other methods exposed via the
IPlayer interface. For example:

- app thread calls Destroy on the OpenSLES player object
- PlaybackActivityMonitor (in system_server) calls applyVolumeShaper()

Since there's no synchronization on mAudioTrack, this can cause IPlayer
methods to access an already destructed AudioTrack object, leading to a
crash. Fix it by replacing sp<> with atomic_sp<>.

Note: while having a destroy method requires extra care when accessing
mAudioTrack, it allows us to destruct the AudioTrack object before its
TrackPlayerBase owner, whose lifetime may be extended indefinitely by
IPlayer binder proxies (see aosp/2909337).

Test: atest trackplayerbase_tests
Test: manual - stress test with injected delays to trigger the race
Change-Id: I8e00ffd6b5fe0e9dbff7de9294bcdcc878602bb2
parent 980a962d

media/libaudioclient/Android.bp

+1 −0
Original line number Original line Diff line number Diff line
@@ -156,6 +156,7 @@ cc_library { 
        "framework-permission-aidl-cpp",

        "framework-permission-aidl-cpp",

        "libbinder",

        "libbinder",

        "libmediametrics",

        "libmediametrics",

        "libmediautils",

        "spatializer-aidl-cpp",

        "spatializer-aidl-cpp",

    ],

    ],

media/libaudioclient/TrackPlayerBase.cpp

+22 −20
Original line number Original line Diff line number Diff line
@@ -38,12 +38,12 @@ void TrackPlayerBase::init(const sp<AudioTrack>& pat, 
                           player_type_t playerType, audio_usage_t usage,

                           player_type_t playerType, audio_usage_t usage,

                           audio_session_t sessionId) {

                           audio_session_t sessionId) {

    PlayerBase::init(playerType, usage, sessionId);

    PlayerBase::init(playerType, usage, sessionId);

    mAudioTrack = pat;

    mAudioTrack.store(pat);

    if (mAudioTrack != 0) {

    if (pat != 0) {

        mCallbackHandle = callback;

        mCallbackHandle = callback;

        mSelfAudioDeviceCallback = new SelfAudioDeviceCallback(*this);

        mSelfAudioDeviceCallback = new SelfAudioDeviceCallback(*this);

        mAudioTrack->addAudioDeviceCallback(mSelfAudioDeviceCallback);

        pat->addAudioDeviceCallback(mSelfAudioDeviceCallback);

        mAudioTrack->setPlayerIId(mPIId); // set in PlayerBase::init().

        pat->setPlayerIId(mPIId);  // set in PlayerBase::init().

    }

    }

}

}
@@ -65,12 +65,15 @@ void TrackPlayerBase::SelfAudioDeviceCallback::onAudioDeviceUpdate(audio_io_hand 
}

}





void TrackPlayerBase::doDestroy() {

void TrackPlayerBase::doDestroy() {

    if (mAudioTrack != 0) {

    sp<AudioTrack> audioTrack = getAudioTrack();

        mAudioTrack->stop();



        mAudioTrack->removeAudioDeviceCallback(mSelfAudioDeviceCallback);

        mSelfAudioDeviceCallback.clear();

    // Note that there may still be another reference in post-unlock phase of SetPlayState

    // Note that there may still be another reference in post-unlock phase of SetPlayState

        mAudioTrack.clear();

    clearAudioTrack();



    if (audioTrack != 0) {

        audioTrack->stop();

        audioTrack->removeAudioDeviceCallback(mSelfAudioDeviceCallback);

        mSelfAudioDeviceCallback.clear();

    }

    }

}

}
@@ -87,16 +90,16 @@ void TrackPlayerBase::setPlayerVolume(float vl, float vr) { 
// Implementation of IPlayer

// Implementation of IPlayer

status_t TrackPlayerBase::playerStart() {

status_t TrackPlayerBase::playerStart() {

    status_t status = NO_INIT;

    status_t status = NO_INIT;

    if (mAudioTrack != 0) {

    if (sp<AudioTrack> audioTrack = getAudioTrack(); audioTrack != 0) {

        status = mAudioTrack->start();

        status = audioTrack->start();

    }

    }

    return status;

    return status;

}

}





status_t TrackPlayerBase::playerPause() {

status_t TrackPlayerBase::playerPause() {

    status_t status = NO_INIT;

    status_t status = NO_INIT;

    if (mAudioTrack != 0) {

    if (sp<AudioTrack> audioTrack = getAudioTrack(); audioTrack != 0) {

        mAudioTrack->pause();

        audioTrack->pause();

        status = NO_ERROR;

        status = NO_ERROR;

    }

    }

    return status;

    return status;
@@ -105,8 +108,8 @@ status_t TrackPlayerBase::playerPause() { 




status_t TrackPlayerBase::playerStop() {

status_t TrackPlayerBase::playerStop() {

    status_t status = NO_INIT;

    status_t status = NO_INIT;

    if (mAudioTrack != 0) {

    if (sp<AudioTrack> audioTrack = getAudioTrack(); audioTrack != 0) {

        mAudioTrack->stop();

        audioTrack->stop();

        status = NO_ERROR;

        status = NO_ERROR;

    }

    }

    return status;

    return status;
@@ -118,10 +121,10 @@ status_t TrackPlayerBase::playerSetVolume() { 




status_t TrackPlayerBase::doSetVolume() {

status_t TrackPlayerBase::doSetVolume() {

    status_t status = NO_INIT;

    status_t status = NO_INIT;

    if (mAudioTrack != 0) {

    if (sp<AudioTrack> audioTrack = getAudioTrack(); audioTrack != 0) {

        float tl = mPlayerVolumeL * mPanMultiplierL * mVolumeMultiplierL;

        float tl = mPlayerVolumeL * mPanMultiplierL * mVolumeMultiplierL;

        float tr = mPlayerVolumeR * mPanMultiplierR * mVolumeMultiplierR;

        float tr = mPlayerVolumeR * mPanMultiplierR * mVolumeMultiplierR;

        mAudioTrack->setVolume(tl, tr);

        audioTrack->setVolume(tl, tr);

        status = NO_ERROR;

        status = NO_ERROR;

    }

    }

    return status;

    return status;
@@ -140,10 +143,9 @@ binder::Status TrackPlayerBase::applyVolumeShaper( 
    if (s != OK) {

    if (s != OK) {

        return binderStatusFromStatusT(s);

        return binderStatusFromStatusT(s);

    }

    }



    if (sp<AudioTrack> audioTrack = getAudioTrack(); audioTrack != 0) {

    if (mAudioTrack != 0) {

        ALOGD("TrackPlayerBase::applyVolumeShaper() from IPlayer");

        ALOGD("TrackPlayerBase::applyVolumeShaper() from IPlayer");

        VolumeShaper::Status status = mAudioTrack->applyVolumeShaper(spConfiguration, spOperation);

        VolumeShaper::Status status = audioTrack->applyVolumeShaper(spConfiguration, spOperation);

        if (status < 0) { // a non-negative value is the volume shaper id.

        if (status < 0) { // a non-negative value is the volume shaper id.

            ALOGE("TrackPlayerBase::applyVolumeShaper() failed with status %d", status);

            ALOGE("TrackPlayerBase::applyVolumeShaper() failed with status %d", status);

        }

        }

media/libaudioclient/include/media/TrackPlayerBase.h

+5 −6
Original line number Original line Diff line number Diff line
@@ -19,6 +19,7 @@ 




#include <media/AudioTrack.h>

#include <media/AudioTrack.h>

#include <media/PlayerBase.h>

#include <media/PlayerBase.h>

#include <mediautils/Synchronization.h>





namespace android {

namespace android {
@@ -37,12 +38,9 @@ public: 
            const media::VolumeShaperConfiguration& configuration,

            const media::VolumeShaperConfiguration& configuration,

            const media::VolumeShaperOperation& operation);

            const media::VolumeShaperOperation& operation);





    sp<AudioTrack> getAudioTrack() { return mAudioTrack; }

    sp<AudioTrack> getAudioTrack() { return mAudioTrack.load(); }





    void clearAudioTrack() { mAudioTrack.clear(); }

    void clearAudioTrack() { mAudioTrack.store(nullptr); }



    // FIXME: make private once all users switch to getAudioTrack()

    sp<AudioTrack> mAudioTrack;





    void setPlayerVolume(float vl, float vr);

    void setPlayerVolume(float vl, float vr);
@@ -72,6 +70,7 @@ private: 
    float mPlayerVolumeL, mPlayerVolumeR;

    float mPlayerVolumeL, mPlayerVolumeR;

    sp<AudioTrack::IAudioTrackCallback> mCallbackHandle;

    sp<AudioTrack::IAudioTrackCallback> mCallbackHandle;

    sp<SelfAudioDeviceCallback> mSelfAudioDeviceCallback;

    sp<SelfAudioDeviceCallback> mSelfAudioDeviceCallback;

    mediautils::atomic_sp<AudioTrack> mAudioTrack;

};

};





} // namespace android

} // namespace android