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

Commit 320910fc authored by Phil Burk's avatar Phil Burk
Browse files

aaudio: fix crash from callbacks during close 

Move some code from release_l() into a new close_l().
Also stop callbacks before freeing memory in the
FixedBlockReader.

The AudioTrack and AudioRecord callbacks can occur
up until their destructors are called. This can lead
to race conditions if the AAudio stream is dismantled
while the AudioTrack or AudioRecord is still alive.
The AudioRecord was being deleted but not the AudioTrack.
That caused some streams to fail if they were using
a FixedBlockReader, which is used when the app
calls AAudioStreamBuilder_setFramesPerDataCallback().

There was also a problem with a few functions like
AAudioStream_getFramesPerBurst() or AAudioStream_getTimestamp(),
which would crash if called after AAudioStream_release( for
INPUT streams.

Bug: 161914201
Bug: 163165126
Test: see bug for repro of the crash
Test: atest CtsNativeMediaAAudioTestCases
Change-Id: If8f6f6f17ffe06eae98eb8b3930bca08c49a15f8
parent f356189f

media/libaaudio/include/aaudio/AAudio.h

+5 −0
Original line number Diff line number Diff line
@@ -1037,6 +1037,11 @@ AAUDIO_API aaudio_result_t AAudioStreamBuilder_delete(AAudioStreamBuilder* buil 
 * but still allow queries to the stream to occur from other threads. This often

 * happens if you are monitoring stream progress from a UI thread.

 *

 * NOTE: This function is only fully implemented for MMAP streams,

 * which are low latency streams supported by some devices.

 * On other "Legacy" streams some audio resources will still be in use

 * and some callbacks may still be in process after this call.

 *

 * @param stream reference provided by AAudioStreamBuilder_openStream()

 * @return {@link #AAUDIO_OK} or a negative error.

 */

media/libaaudio/src/core/AAudioAudio.cpp

+2 −4
Original line number Diff line number Diff line
@@ -255,16 +255,14 @@ AAUDIO_API aaudio_result_t AAudioStream_close(AAudioStream* stream) { 
    if (audioStream != nullptr) {

        aaudio_stream_id_t id = audioStream->getId();

        ALOGD("%s(s#%u) called ---------------", __func__, id);

        result = audioStream->safeRelease();

        // safeRelease will only fail if called illegally, for example, from a callback.

        result = audioStream->safeReleaseClose();

        // safeReleaseClose will only fail if called illegally, for example, from a callback.

        // That would result in deleting an active stream, which would cause a crash.

        if (result != AAUDIO_OK) {

            ALOGW("%s(s#%u) failed. Close it from another thread.",

                  __func__, id);

        } else {

            audioStream->unregisterPlayerBase();

             // Mark CLOSED to keep destructors from asserting.

            audioStream->closeFinal();

            delete audioStream;

        }

        ALOGD("%s(s#%u) returned %d ---------", __func__, id, result);

media/libaaudio/src/core/AudioStream.cpp

+13 −2
Original line number Diff line number Diff line
@@ -301,18 +301,29 @@ aaudio_result_t AudioStream::safeStop() { 
}



aaudio_result_t AudioStream::safeRelease() {

    // This get temporarily unlocked in the release() when joining callback threads.

    // This get temporarily unlocked in the MMAP release() when joining callback threads.

    std::lock_guard<std::mutex> lock(mStreamLock);

    if (collidesWithCallback()) {

        ALOGE("%s cannot be called from a callback!", __func__);

        return AAUDIO_ERROR_INVALID_STATE;

    }

    if (getState() == AAUDIO_STREAM_STATE_CLOSING) {

    if (getState() == AAUDIO_STREAM_STATE_CLOSING) { // already released?

        return AAUDIO_OK;

    }

    return release_l();

}



aaudio_result_t AudioStream::safeReleaseClose() {

    // This get temporarily unlocked in the MMAP release() when joining callback threads.

    std::lock_guard<std::mutex> lock(mStreamLock);

    if (collidesWithCallback()) {

        ALOGE("%s cannot be called from a callback!", __func__);

        return AAUDIO_ERROR_INVALID_STATE;

    }

    releaseCloseFinal();

    return AAUDIO_OK;

}



void AudioStream::setState(aaudio_stream_state_t state) {

    ALOGD("%s(s#%d) from %d to %d", __func__, getId(), mState, state);

    if (state == mState) {

media/libaaudio/src/core/AudioStream.h

+36 −8
Original line number Diff line number Diff line
@@ -117,6 +117,17 @@ public: 
    virtual void logOpen();

    void logReleaseBufferState();



    /* Note about naming for "release"  and "close" related methods.

     *

     * These names are intended to match the public AAudio API.

     * The original AAudio API had an AAudioStream_close() function that

     * released the hardware and deleted the stream. That made it difficult

     * because apps want to release the HW ASAP but are not in a rush to delete

     * the stream object. So in R we added an AAudioStream_release() function

     * that just released the hardware.

     * The AAudioStream_close() method releases if needed and then closes.

     */



    /**

     * Free any hardware or system resources from the open() call.

     * It is safe to call release_l() multiple times.
@@ -126,22 +137,27 @@ public: 
        return AAUDIO_OK;

    }



    aaudio_result_t closeFinal() {

    /**

     * Free any resources not already freed by release_l().

     * Assume release_l() already called.

     */

    virtual void close_l() {

        // Releasing the stream will set the state to CLOSING.

        assert(getState() == AAUDIO_STREAM_STATE_CLOSING);

        // setState() prevents a transition from CLOSING to any state other than CLOSED.

        // State is checked by destructor.

        setState(AAUDIO_STREAM_STATE_CLOSED);

        return AAUDIO_OK;

    }



    /**

     * Release then close the stream.

     * @return AAUDIO_OK or negative error.

     */

    aaudio_result_t releaseCloseFinal() {

        aaudio_result_t result = release_l(); // TODO review locking

        if (result == AAUDIO_OK) {

          result = closeFinal();

    void releaseCloseFinal() {

        if (getState() != AAUDIO_STREAM_STATE_CLOSING) { // not already released?

            // Ignore result and keep closing.

            (void) release_l();

        }

        return result;

        close_l();

    }



    // This is only used to identify a stream in the logs without
@@ -395,8 +411,20 @@ public: 
     */

    aaudio_result_t systemStopFromCallback();



    /**

     * Safely RELEASE a stream after taking mStreamLock and checking

     * to make sure we are not being called from a callback.

     * @return AAUDIO_OK or a negative error

     */

    aaudio_result_t safeRelease();



    /**

     * Safely RELEASE and CLOSE a stream after taking mStreamLock and checking

     * to make sure we are not being called from a callback.

     * @return AAUDIO_OK or a negative error

     */

    aaudio_result_t safeReleaseClose();



protected:



    // PlayerBase allows the system to control the stream volume.

media/libaaudio/src/legacy/AudioStreamRecord.cpp

+8 −2
Original line number Diff line number Diff line
@@ -290,14 +290,20 @@ aaudio_result_t AudioStreamRecord::release_l() { 
    if (getState() != AAUDIO_STREAM_STATE_CLOSING) {

        mAudioRecord->removeAudioDeviceCallback(mDeviceCallback);

        logReleaseBufferState();

        mAudioRecord.clear();

        mFixedBlockWriter.close();

        // Data callbacks may still be running!

        return AudioStream::release_l();

    } else {

        return AAUDIO_OK; // already released

    }

}



void AudioStreamRecord::close_l() {

    // Stop callbacks before deleting mFixedBlockWriter memory.

    mAudioRecord.clear();

    mFixedBlockWriter.close();

    AudioStream::close_l();

}



const void * AudioStreamRecord::maybeConvertDeviceData(const void *audioData, int32_t numFrames) {

    if (mFormatConversionBufferFloat.get() != nullptr) {

        LOG_ALWAYS_FATAL_IF(numFrames > mFormatConversionBufferSizeInFrames,