From 99306c80f061e4c11f05c21a19bfab6eac01c529 Mon Sep 17 00:00:00 2001 From: Phil Burk Date: Mon, 14 Aug 2017 12:38:58 -0700 Subject: [PATCH] aaudio: prevent memory leak from double configure Prevent AudioEndpoint from being configure()d twice. Check return value of AudioEndpoint.configure(). Prevent AudioStreamInternal from being open()ed twice. Bug: 64522125 Test: adb shell dumpsys media.audio_flinger --unreachable Change-Id: I3ae07af5610fd1f9e539f884923f781eefcd645f --- media/libaaudio/src/client/AudioEndpoint.cpp | 18 ++- media/libaaudio/src/client/AudioStreamInternal.cpp | 152 +++++++++++---------- 2 files changed, 94 insertions(+), 76 deletions(-) diff --git a/media/libaaudio/src/client/AudioEndpoint.cpp b/media/libaaudio/src/client/AudioEndpoint.cpp index 3ee450a861..604eed57ab 100644 --- a/media/libaaudio/src/client/AudioEndpoint.cpp +++ b/media/libaaudio/src/client/AudioEndpoint.cpp @@ -121,24 +121,28 @@ aaudio_result_t AudioEndpoint::configure(const EndpointDescriptor *pEndpointDesc { aaudio_result_t result = AudioEndpoint_validateDescriptor(pEndpointDescriptor); if (result != AAUDIO_OK) { - ALOGE("AudioEndpoint_validateQueueDescriptor returned %d %s", - result, AAudio_convertResultToText(result)); return result; } // ============================ up message queue ============================= const RingBufferDescriptor *descriptor = &pEndpointDescriptor->upMessageQueueDescriptor; if(descriptor->bytesPerFrame != sizeof(AAudioServiceMessage)) { - ALOGE("AudioEndpoint::configure() bytesPerFrame != sizeof(AAudioServiceMessage) = %d", + ALOGE("AudioEndpoint.configure() bytesPerFrame != sizeof(AAudioServiceMessage) = %d", descriptor->bytesPerFrame); return AAUDIO_ERROR_INTERNAL; } if(descriptor->readCounterAddress == nullptr || descriptor->writeCounterAddress == nullptr) { - ALOGE("AudioEndpoint_validateQueueDescriptor() NULL counter address"); + ALOGE("AudioEndpoint.configure() NULL counter address"); return AAUDIO_ERROR_NULL; } + // Prevent memory leak and reuse. + if(mUpCommandQueue != nullptr || mDataQueue != nullptr) { + ALOGE("AudioEndpoint.configure() endpoint already used"); + return AAUDIO_ERROR_INTERNAL; + } + mUpCommandQueue = new FifoBuffer( descriptor->bytesPerFrame, descriptor->capacityInFrames, @@ -149,8 +153,8 @@ aaudio_result_t AudioEndpoint::configure(const EndpointDescriptor *pEndpointDesc // ============================ data queue ============================= descriptor = &pEndpointDescriptor->dataQueueDescriptor; - ALOGV("AudioEndpoint::configure() data framesPerBurst = %d", descriptor->framesPerBurst); - ALOGV("AudioEndpoint::configure() data readCounterAddress = %p", + ALOGV("AudioEndpoint.configure() data framesPerBurst = %d", descriptor->framesPerBurst); + ALOGV("AudioEndpoint.configure() data readCounterAddress = %p", descriptor->readCounterAddress); // An example of free running is when the other side is read or written by hardware DMA @@ -159,7 +163,7 @@ aaudio_result_t AudioEndpoint::configure(const EndpointDescriptor *pEndpointDesc ? descriptor->readCounterAddress // read by other side : descriptor->writeCounterAddress; // written by other side mFreeRunning = (remoteCounter == nullptr); - ALOGV("AudioEndpoint::configure() mFreeRunning = %d", mFreeRunning ? 1 : 0); + ALOGV("AudioEndpoint.configure() mFreeRunning = %d", mFreeRunning ? 1 : 0); int64_t *readCounterAddress = (descriptor->readCounterAddress == nullptr) ? &mDataReadCounter diff --git a/media/libaaudio/src/client/AudioStreamInternal.cpp b/media/libaaudio/src/client/AudioStreamInternal.cpp index 41d4909e07..bdebf8f5a6 100644 --- a/media/libaaudio/src/client/AudioStreamInternal.cpp +++ b/media/libaaudio/src/client/AudioStreamInternal.cpp @@ -80,9 +80,16 @@ AudioStreamInternal::~AudioStreamInternal() { aaudio_result_t AudioStreamInternal::open(const AudioStreamBuilder &builder) { aaudio_result_t result = AAUDIO_OK; + int32_t capacity; AAudioStreamRequest request; - AAudioStreamConfiguration configuration; + AAudioStreamConfiguration configurationOutput; + if (getState() != AAUDIO_STREAM_STATE_UNINITIALIZED) { + ALOGE("AudioStreamInternal::open(): already open! state = %d", getState()); + return AAUDIO_ERROR_INVALID_STATE; + } + + // Copy requested parameters to the stream. result = AudioStream::open(builder); if (result < 0) { return result; @@ -109,87 +116,95 @@ aaudio_result_t AudioStreamInternal::open(const AudioStreamBuilder &builder) { request.getConfiguration().setBufferCapacity(builder.getBufferCapacity()); - mServiceStreamHandle = mServiceInterface.openStream(request, configuration); + mServiceStreamHandle = mServiceInterface.openStream(request, configurationOutput); if (mServiceStreamHandle < 0) { result = mServiceStreamHandle; - ALOGE("AudioStreamInternal.open(): openStream() returned %d", result); - } else { - result = configuration.validate(); - if (result != AAUDIO_OK) { - close(); - return result; - } - // Save results of the open. - setSampleRate(configuration.getSampleRate()); - setSamplesPerFrame(configuration.getSamplesPerFrame()); - setDeviceId(configuration.getDeviceId()); - setSharingMode(configuration.getSharingMode()); - - // Save device format so we can do format conversion and volume scaling together. - mDeviceFormat = configuration.getFormat(); - - result = mServiceInterface.getStreamDescription(mServiceStreamHandle, mEndPointParcelable); - if (result != AAUDIO_OK) { - mServiceInterface.closeStream(mServiceStreamHandle); - return result; - } + ALOGE("AudioStreamInternal::open(): openStream() returned %d", result); + return result; + } - // resolve parcelable into a descriptor - result = mEndPointParcelable.resolve(&mEndpointDescriptor); - if (result != AAUDIO_OK) { - mServiceInterface.closeStream(mServiceStreamHandle); - return result; - } + result = configurationOutput.validate(); + if (result != AAUDIO_OK) { + goto error; + } + // Save results of the open. + setSampleRate(configurationOutput.getSampleRate()); + setSamplesPerFrame(configurationOutput.getSamplesPerFrame()); + setDeviceId(configurationOutput.getDeviceId()); + setSharingMode(configurationOutput.getSharingMode()); - // Configure endpoint based on descriptor. - mAudioEndpoint.configure(&mEndpointDescriptor, getDirection()); + // Save device format so we can do format conversion and volume scaling together. + mDeviceFormat = configurationOutput.getFormat(); - mFramesPerBurst = mEndpointDescriptor.dataQueueDescriptor.framesPerBurst; - int32_t capacity = mEndpointDescriptor.dataQueueDescriptor.capacityInFrames; + result = mServiceInterface.getStreamDescription(mServiceStreamHandle, mEndPointParcelable); + if (result != AAUDIO_OK) { + goto error; + } - // Validate result from server. - if (mFramesPerBurst < 16 || mFramesPerBurst > 16 * 1024) { - ALOGE("AudioStream::open(): framesPerBurst out of range = %d", mFramesPerBurst); - return AAUDIO_ERROR_OUT_OF_RANGE; - } - if (capacity < mFramesPerBurst || capacity > 32 * 1024) { - ALOGE("AudioStream::open(): bufferCapacity out of range = %d", capacity); - return AAUDIO_ERROR_OUT_OF_RANGE; - } + // Resolve parcelable into a descriptor. + result = mEndPointParcelable.resolve(&mEndpointDescriptor); + if (result != AAUDIO_OK) { + goto error; + } - mClockModel.setSampleRate(getSampleRate()); - mClockModel.setFramesPerBurst(mFramesPerBurst); + // Configure endpoint based on descriptor. + result = mAudioEndpoint.configure(&mEndpointDescriptor, getDirection()); + if (result != AAUDIO_OK) { + goto error; + } - if (getDataCallbackProc()) { - mCallbackFrames = builder.getFramesPerDataCallback(); - if (mCallbackFrames > getBufferCapacity() / 2) { - ALOGE("AudioStreamInternal::open(): framesPerCallback too big = %d, capacity = %d", - mCallbackFrames, getBufferCapacity()); - mServiceInterface.closeStream(mServiceStreamHandle); - return AAUDIO_ERROR_OUT_OF_RANGE; + mFramesPerBurst = mEndpointDescriptor.dataQueueDescriptor.framesPerBurst; + capacity = mEndpointDescriptor.dataQueueDescriptor.capacityInFrames; + + // Validate result from server. + if (mFramesPerBurst < 16 || mFramesPerBurst > 16 * 1024) { + ALOGE("AudioStreamInternal::open(): framesPerBurst out of range = %d", mFramesPerBurst); + result = AAUDIO_ERROR_OUT_OF_RANGE; + goto error; + } + if (capacity < mFramesPerBurst || capacity > 32 * 1024) { + ALOGE("AudioStreamInternal::open(): bufferCapacity out of range = %d", capacity); + result = AAUDIO_ERROR_OUT_OF_RANGE; + goto error; + } - } else if (mCallbackFrames < 0) { - ALOGE("AudioStreamInternal::open(): framesPerCallback negative"); - mServiceInterface.closeStream(mServiceStreamHandle); - return AAUDIO_ERROR_OUT_OF_RANGE; + mClockModel.setSampleRate(getSampleRate()); + mClockModel.setFramesPerBurst(mFramesPerBurst); - } - if (mCallbackFrames == AAUDIO_UNSPECIFIED) { - mCallbackFrames = mFramesPerBurst; - } + if (getDataCallbackProc()) { + mCallbackFrames = builder.getFramesPerDataCallback(); + if (mCallbackFrames > getBufferCapacity() / 2) { + ALOGE("AudioStreamInternal::open(): framesPerCallback too big = %d, capacity = %d", + mCallbackFrames, getBufferCapacity()); + result = AAUDIO_ERROR_OUT_OF_RANGE; + goto error; - int32_t bytesPerFrame = getSamplesPerFrame() - * AAudioConvert_formatToSizeInBytes(getFormat()); - int32_t callbackBufferSize = mCallbackFrames * bytesPerFrame; - mCallbackBuffer = new uint8_t[callbackBufferSize]; - } + } else if (mCallbackFrames < 0) { + ALOGE("AudioStreamInternal::open(): framesPerCallback negative"); + result = AAUDIO_ERROR_OUT_OF_RANGE; + goto error; - setState(AAUDIO_STREAM_STATE_OPEN); - // only connect to AudioManager if this is a playback stream running in client process - if (!mInService && getDirection() == AAUDIO_DIRECTION_OUTPUT) { - init(android::PLAYER_TYPE_AAUDIO, AUDIO_USAGE_MEDIA); } + if (mCallbackFrames == AAUDIO_UNSPECIFIED) { + mCallbackFrames = mFramesPerBurst; + } + + int32_t bytesPerFrame = getSamplesPerFrame() + * AAudioConvert_formatToSizeInBytes(getFormat()); + int32_t callbackBufferSize = mCallbackFrames * bytesPerFrame; + mCallbackBuffer = new uint8_t[callbackBufferSize]; + } + + setState(AAUDIO_STREAM_STATE_OPEN); + // Only connect to AudioManager if this is a playback stream running in client process. + if (!mInService && getDirection() == AAUDIO_DIRECTION_OUTPUT) { + init(android::PLAYER_TYPE_AAUDIO, AUDIO_USAGE_MEDIA); } + + return result; + +error: + close(); return result; } @@ -224,7 +239,6 @@ aaudio_result_t AudioStreamInternal::close() { } } - static void *aaudio_callback_thread_proc(void *context) { AudioStreamInternal *stream = (AudioStreamInternal *)context; -- 2.11.0