From ac0bbe16f3eba46b3d8057b66c2aff9101fc6f7d Mon Sep 17 00:00:00 2001 From: Jeff Brown Date: Fri, 9 Aug 2013 18:32:30 -0700 Subject: Fix potential leak of audio input handle. The audio input handle is ultimately owned by the audio recorder object but it could be dropped on the floor if an error occurred before that object was fully initialized. Rearranged some of the argument validation and merged getInput_l with openRecord_l to simplify the code and prevent such a leak from occurring. Bug: 10265163 Change-Id: I124dce344b1d11c2dd66ca5e2c9aec0c52c230e2 --- include/media/AudioRecord.h | 9 +--- media/libmedia/AudioRecord.cpp | 103 +++++++++++++++++------------------------ 2 files changed, 43 insertions(+), 69 deletions(-) diff --git a/include/media/AudioRecord.h b/include/media/AudioRecord.h index f457261..62f0c64 100644 --- a/include/media/AudioRecord.h +++ b/include/media/AudioRecord.h @@ -423,14 +423,7 @@ private: nsecs_t processAudioBuffer(const sp& thread); // caller must hold lock on mLock for all _l methods - status_t openRecord_l(uint32_t sampleRate, - audio_format_t format, - size_t frameCount, - audio_input_flags_t flags, - audio_io_handle_t input, - size_t epoch); - - audio_io_handle_t getInput_l(); + status_t openRecord_l(size_t epoch); // FIXME enum is faster than strcmp() for parameter 'from' status_t restoreRecord_l(const char *from); diff --git a/media/libmedia/AudioRecord.cpp b/media/libmedia/AudioRecord.cpp index 616c3d6..e934a3e 100644 --- a/media/libmedia/AudioRecord.cpp +++ b/media/libmedia/AudioRecord.cpp @@ -175,6 +175,7 @@ status_t AudioRecord::set( if (inputSource == AUDIO_SOURCE_DEFAULT) { inputSource = AUDIO_SOURCE_MIC; } + mInputSource = inputSource; if (sampleRate == 0) { ALOGE("Invalid sample rate %u", sampleRate); @@ -210,28 +211,10 @@ status_t AudioRecord::set( // Assumes audio_is_linear_pcm(format), else sizeof(uint8_t) mFrameSize = channelCount * audio_bytes_per_sample(format); - if (sessionId == 0 ) { - mSessionId = AudioSystem::newAudioSessionId(); - } else { - mSessionId = sessionId; - } - ALOGV("set(): mSessionId %d", mSessionId); - - mFlags = flags; - - audio_io_handle_t input = AudioSystem::getInput(inputSource, - sampleRate, - format, - channelMask, - mSessionId); - if (input == 0) { - ALOGE("Could not get audio input for record source %d", inputSource); - return BAD_VALUE; - } - // validate framecount size_t minFrameCount = 0; - status_t status = getMinFrameCount(&minFrameCount, sampleRate, format, channelMask); + status_t status = AudioRecord::getMinFrameCount(&minFrameCount, + sampleRate, format, channelMask); if (status != NO_ERROR) { ALOGE("getMinFrameCount() failed; status %d", status); return status; @@ -244,13 +227,23 @@ status_t AudioRecord::set( ALOGE("frameCount %u < minFrameCount %u", frameCount, minFrameCount); return BAD_VALUE; } + mFrameCount = frameCount; mNotificationFramesReq = notificationFrames; mNotificationFramesAct = 0; + if (sessionId == 0 ) { + mSessionId = AudioSystem::newAudioSessionId(); + } else { + mSessionId = sessionId; + } + ALOGV("set(): mSessionId %d", mSessionId); + + mFlags = flags; + // create the IAudioRecord - status = openRecord_l(sampleRate, format, frameCount, mFlags, input, 0 /*epoch*/); - if (status != NO_ERROR) { + status = openRecord_l(0 /*epoch*/); + if (status) { return status; } @@ -274,8 +267,6 @@ status_t AudioRecord::set( mMarkerReached = false; mNewPosition = 0; mUpdatePeriod = 0; - mInputSource = inputSource; - mInput = input; AudioSystem::acquireAudioSessionId(mSessionId); mSequence = 1; mObservedSequence = mSequence; @@ -429,13 +420,7 @@ unsigned int AudioRecord::getInputFramesLost() const // ------------------------------------------------------------------------- // must be called with mLock held -status_t AudioRecord::openRecord_l( - uint32_t sampleRate, - audio_format_t format, - size_t frameCount, - audio_input_flags_t flags, - audio_io_handle_t input, - size_t epoch) +status_t AudioRecord::openRecord_l(size_t epoch) { status_t status; const sp& audioFlinger = AudioSystem::get_audio_flinger(); @@ -449,12 +434,11 @@ status_t AudioRecord::openRecord_l( // Client can only express a preference for FAST. Server will perform additional tests. // The only supported use case for FAST is callback transfer mode. - if (flags & AUDIO_INPUT_FLAG_FAST) { + if (mFlags & AUDIO_INPUT_FLAG_FAST) { if ((mTransfer != TRANSFER_CALLBACK) || (mAudioRecordThread == 0)) { ALOGW("AUDIO_INPUT_FLAG_FAST denied by client"); // once denied, do not request again if IAudioRecord is re-created - flags = (audio_input_flags_t) (flags & ~AUDIO_INPUT_FLAG_FAST); - mFlags = flags; + mFlags = (audio_input_flags_t) (mFlags & ~AUDIO_INPUT_FLAG_FAST); } else { trackFlags |= IAudioFlinger::TRACK_FAST; tid = mAudioRecordThread->getTid(); @@ -463,18 +447,25 @@ status_t AudioRecord::openRecord_l( mNotificationFramesAct = mNotificationFramesReq; - if (!(flags & AUDIO_INPUT_FLAG_FAST)) { + if (!(mFlags & AUDIO_INPUT_FLAG_FAST)) { // Make sure that application is notified with sufficient margin before overrun - if (mNotificationFramesAct == 0 || mNotificationFramesAct > frameCount/2) { - mNotificationFramesAct = frameCount/2; + if (mNotificationFramesAct == 0 || mNotificationFramesAct > mFrameCount/2) { + mNotificationFramesAct = mFrameCount/2; } } + audio_io_handle_t input = AudioSystem::getInput(mInputSource, mSampleRate, mFormat, + mChannelMask, mSessionId); + if (input == 0) { + ALOGE("Could not get audio input for record source %d", mInputSource); + return BAD_VALUE; + } + int originalSessionId = mSessionId; sp record = audioFlinger->openRecord(input, - sampleRate, format, + mSampleRate, mFormat, mChannelMask, - frameCount, + mFrameCount, &trackFlags, tid, &mSessionId, @@ -484,6 +475,7 @@ status_t AudioRecord::openRecord_l( if (record == 0) { ALOGE("AudioFlinger could not create record track, status: %d", status); + AudioSystem::releaseInput(input); return status; } sp iMem = record->getCblk(); @@ -495,27 +487,27 @@ status_t AudioRecord::openRecord_l( mAudioRecord->asBinder()->unlinkToDeath(mDeathNotifier, this); mDeathNotifier.clear(); } + mInput = input; mAudioRecord = record; mCblkMemory = iMem; audio_track_cblk_t* cblk = static_cast(iMem->pointer()); mCblk = cblk; // FIXME missing fast track frameCount logic mAwaitBoost = false; - if (flags & AUDIO_INPUT_FLAG_FAST) { + if (mFlags & AUDIO_INPUT_FLAG_FAST) { if (trackFlags & IAudioFlinger::TRACK_FAST) { - ALOGV("AUDIO_INPUT_FLAG_FAST successful; frameCount %u", frameCount); + ALOGV("AUDIO_INPUT_FLAG_FAST successful; frameCount %u", mFrameCount); mAwaitBoost = true; // double-buffering is not required for fast tracks, due to tighter scheduling - if (mNotificationFramesAct == 0 || mNotificationFramesAct > frameCount) { - mNotificationFramesAct = frameCount; + if (mNotificationFramesAct == 0 || mNotificationFramesAct > mFrameCount) { + mNotificationFramesAct = mFrameCount; } } else { - ALOGV("AUDIO_INPUT_FLAG_FAST denied by server; frameCount %u", frameCount); + ALOGV("AUDIO_INPUT_FLAG_FAST denied by server; frameCount %u", mFrameCount); // once denied, do not request again if IAudioRecord is re-created - flags = (audio_input_flags_t) (flags & ~AUDIO_INPUT_FLAG_FAST); - mFlags = flags; - if (mNotificationFramesAct == 0 || mNotificationFramesAct > frameCount/2) { - mNotificationFramesAct = frameCount/2; + mFlags = (audio_input_flags_t) (mFlags & ~AUDIO_INPUT_FLAG_FAST); + if (mNotificationFramesAct == 0 || mNotificationFramesAct > mFrameCount/2) { + mNotificationFramesAct = mFrameCount/2; } } } @@ -524,7 +516,7 @@ status_t AudioRecord::openRecord_l( void *buffers = (char*)cblk + sizeof(audio_track_cblk_t); // update proxy - mProxy = new AudioRecordClientProxy(cblk, buffers, frameCount, mFrameSize); + mProxy = new AudioRecordClientProxy(cblk, buffers, mFrameCount, mFrameSize); mProxy->setEpoch(epoch); mProxy->setMinimum(mNotificationFramesAct); @@ -651,17 +643,6 @@ audio_io_handle_t AudioRecord::getInput() const return mInput; } -// must be called with mLock held -audio_io_handle_t AudioRecord::getInput_l() -{ - mInput = AudioSystem::getInput(mInputSource, - mSampleRate, - mFormat, - mChannelMask, - mSessionId); - return mInput; -} - // ------------------------------------------------------------------------- ssize_t AudioRecord::read(void* buffer, size_t userSize) @@ -949,7 +930,7 @@ status_t AudioRecord::restoreRecord_l(const char *from) // It will also delete the strong references on previous IAudioRecord and IMemory size_t position = mProxy->getPosition(); mNewPosition = position + mUpdatePeriod; - result = openRecord_l(mSampleRate, mFormat, mFrameCount, mFlags, getInput_l(), position); + result = openRecord_l(position); if (result == NO_ERROR) { if (mActive) { // callback thread or sync event hasn't changed -- cgit v1.1