From 6f22680db6bfe392a2f9f6b934ea8a54abea0ad7 Mon Sep 17 00:00:00 2001 From: Kevin Rocard Date: Mon, 3 Apr 2017 10:29:34 -0700 Subject: [PATCH 1/5] Audio HAL VTS: Fix documentation Some test did not have any documentation Test: compile & run Bug: 36311550 Change-Id: I37c40f6f17993a275e5c40b9a835ac04acf4f8e6 Signed-off-by: Kevin Rocard --- .../2.0/vts/functional/AudioPrimaryHidlHalTest.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp b/audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp index 10215698b8..8ccf55e81d 100644 --- a/audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp +++ b/audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp @@ -990,10 +990,12 @@ struct Capability { }; TEST_P(OutputStreamTest, SupportsPauseAndResumeAndDrain) { + doc::test("Implementation must expose pause, resume and drain capabilities"); Capability(stream.get()); } TEST_P(OutputStreamTest, GetRenderPosition) { + doc::test("The render position should be 0 on a not started"); uint32_t dspFrames; ASSERT_OK(stream->getRenderPosition(returnIn(res, dspFrames))); if (res == Result::NOT_SUPPORTED) { @@ -1005,6 +1007,7 @@ TEST_P(OutputStreamTest, GetRenderPosition) { } TEST_P(OutputStreamTest, GetNextWriteTimestamp) { + doc::test("The render position of a stream just created should be 0"); uint64_t timestampUs; ASSERT_OK(stream->getNextWriteTimestamp(returnIn(res, timestampUs))); if (res == Result::NOT_SUPPORTED) { @@ -1030,6 +1033,7 @@ static bool isAsyncModeSupported(IStreamOut *stream) { } TEST_P(OutputStreamTest, SetCallback) { + doc::test("If supported, registering callback for async operation should never fail"); if (!isAsyncModeSupported(stream.get())) { doc::partialTest("The stream does not support async operations"); return; @@ -1039,6 +1043,7 @@ TEST_P(OutputStreamTest, SetCallback) { } TEST_P(OutputStreamTest, clearCallback) { + doc::test("If supported, clearing a callback to go back to sync operation should not fail"); if (!isAsyncModeSupported(stream.get())) { doc::partialTest("The stream does not support async operations"); return; @@ -1049,6 +1054,7 @@ TEST_P(OutputStreamTest, clearCallback) { } TEST_P(OutputStreamTest, Resume) { + doc::test("If supported, a stream should fail to resume if not previously paused"); if (!Capability(stream.get()).resume) { doc::partialTest("The output stream does not support resume"); return; @@ -1057,6 +1063,7 @@ TEST_P(OutputStreamTest, Resume) { } TEST_P(OutputStreamTest, Pause) { + doc::test("If supported, a stream should fail to pause if not previously started"); if (!Capability(stream.get()).pause) { doc::partialTest("The output stream does not support pause"); return; @@ -1066,21 +1073,24 @@ TEST_P(OutputStreamTest, Pause) { static void testDrain(IStreamOut *stream, AudioDrain type) { if (!Capability(stream).drain) { - doc::partialTest("The output stream does not support pause"); + doc::partialTest("The output stream does not support drain"); return; } ASSERT_RESULT(Result::OK, stream->drain(type)); } TEST_P(OutputStreamTest, DrainAll) { + doc::test("If supported, a stream should always succeed to drain"); testDrain(stream.get(), AudioDrain::ALL); } TEST_P(OutputStreamTest, DrainEarlyNotify) { + doc::test("If supported, a stream should always succeed to drain"); testDrain(stream.get(), AudioDrain::EARLY_NOTIFY); } TEST_P(OutputStreamTest, FlushStop) { + doc::test("If supported, a stream should always succeed to flush"); auto ret = stream->flush(); ASSERT_TRUE(ret.isOk()); if (ret == Result::NOT_SUPPORTED) { @@ -1091,6 +1101,7 @@ TEST_P(OutputStreamTest, FlushStop) { } TEST_P(OutputStreamTest, GetPresentationPositionStop) { + doc::test("If supported, a stream should always succeed to retrieve the presentation position"); uint64_t frames; TimeSpec mesureTS; ASSERT_OK(stream->getPresentationPosition(returnIn(res, frames, mesureTS))); From fba442a60d199441d06bf03375cb7904768403da Mon Sep 17 00:00:00 2001 From: Kevin Rocard Date: Fri, 31 Mar 2017 19:34:41 -0700 Subject: [PATCH 2/5] Audio HAL: Detect openDevice failure The result status was not checked. Test: Run test on target Bug: 36311550 Change-Id: I197b52d0b5a1276d3e3beba105bb91639f89e060 Signed-off-by: Kevin Rocard --- audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp b/audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp index 8ccf55e81d..4b00f35c37 100644 --- a/audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp +++ b/audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp @@ -171,6 +171,7 @@ public: sp baseDevice; ASSERT_OK(devicesFactory->openDevice(IDevicesFactory::Device::PRIMARY, returnIn(result, baseDevice))); + ASSERT_OK(result); ASSERT_TRUE(baseDevice != nullptr); environment->registerTearDown([]{ device.clear(); }); From 67d550888a021633cc33cb284bb0658b008887c6 Mon Sep 17 00:00:00 2001 From: Kevin Rocard Date: Fri, 31 Mar 2017 19:34:04 -0700 Subject: [PATCH 3/5] Audio HAL: Detect buffer memory allocation failure If the requested buffer was too big, memory allocation would fail, resulting if a audio hal crash (uncatch exception thrown by new). Properly hadle the failure by retuning INVALID_PARAMETERS in such case. Bug: 36311550 Test: Run test on target Change-Id: Ib4170eb6a9f88f9352d0912083b43d600771bb8e Signed-off-by: Kevin Rocard --- audio/2.0/default/StreamIn.cpp | 15 +++++++++++++-- audio/2.0/default/StreamOut.cpp | 18 +++++++++++++++--- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/audio/2.0/default/StreamIn.cpp b/audio/2.0/default/StreamIn.cpp index 2745607e8b..97f8307bdf 100644 --- a/audio/2.0/default/StreamIn.cpp +++ b/audio/2.0/default/StreamIn.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include "StreamIn.h" @@ -52,7 +53,11 @@ class ReadThread : public Thread { mDataMQ(dataMQ), mStatusMQ(statusMQ), mEfGroup(efGroup), - mBuffer(new uint8_t[dataMQ->getQuantumCount()]) { + mBuffer(nullptr) { + } + bool init() { + mBuffer.reset(new(std::nothrow) uint8_t[mDataMQ->getQuantumCount()]); + return mBuffer != nullptr; } virtual ~ReadThread() {} @@ -328,13 +333,18 @@ Return StreamIn::prepareForReading( } // Create and launch the thread. - mReadThread = new ReadThread( + auto tempReadThread = std::make_unique( &mStopReadThread, mStream, tempCommandMQ.get(), tempDataMQ.get(), tempStatusMQ.get(), mEfGroup); + if (!tempReadThread->init()) { + _hidl_cb(Result::INVALID_ARGUMENTS, + CommandMQ::Descriptor(), DataMQ::Descriptor(), StatusMQ::Descriptor(), threadInfo); + return Void(); + } status = mReadThread->run("reader", PRIORITY_URGENT_AUDIO); if (status != OK) { ALOGW("failed to start reader thread: %s", strerror(-status)); @@ -346,6 +356,7 @@ Return StreamIn::prepareForReading( mCommandMQ = std::move(tempCommandMQ); mDataMQ = std::move(tempDataMQ); mStatusMQ = std::move(tempStatusMQ); + mReadThread = tempReadThread.release(); threadInfo.pid = getpid(); threadInfo.tid = mReadThread->getTid(); _hidl_cb(Result::OK, diff --git a/audio/2.0/default/StreamOut.cpp b/audio/2.0/default/StreamOut.cpp index 88045a0afe..cc16a82a58 100644 --- a/audio/2.0/default/StreamOut.cpp +++ b/audio/2.0/default/StreamOut.cpp @@ -18,6 +18,8 @@ //#define LOG_NDEBUG 0 #define ATRACE_TAG ATRACE_TAG_AUDIO +#include + #include #include #include @@ -50,7 +52,11 @@ class WriteThread : public Thread { mDataMQ(dataMQ), mStatusMQ(statusMQ), mEfGroup(efGroup), - mBuffer(new uint8_t[dataMQ->getQuantumCount()]) { + mBuffer(nullptr) { + } + bool init() { + mBuffer.reset(new(std::nothrow) uint8_t[mDataMQ->getQuantumCount()]); + return mBuffer != nullptr; } virtual ~WriteThread() {} @@ -311,14 +317,19 @@ Return StreamOut::prepareForWriting( } // Create and launch the thread. - mWriteThread = new WriteThread( + auto tempWriteThread = std::make_unique( &mStopWriteThread, mStream, tempCommandMQ.get(), tempDataMQ.get(), tempStatusMQ.get(), mEfGroup); - status = mWriteThread->run("writer", PRIORITY_URGENT_AUDIO); + if (!tempWriteThread->init()) { + _hidl_cb(Result::INVALID_ARGUMENTS, + CommandMQ::Descriptor(), DataMQ::Descriptor(), StatusMQ::Descriptor(), threadInfo); + return Void(); + } + status = tempWriteThread->run("writer", PRIORITY_URGENT_AUDIO); if (status != OK) { ALOGW("failed to start writer thread: %s", strerror(-status)); _hidl_cb(Result::INVALID_ARGUMENTS, @@ -329,6 +340,7 @@ Return StreamOut::prepareForWriting( mCommandMQ = std::move(tempCommandMQ); mDataMQ = std::move(tempDataMQ); mStatusMQ = std::move(tempStatusMQ); + mWriteThread = tempWriteThread.release(); threadInfo.pid = getpid(); threadInfo.tid = mWriteThread->getTid(); _hidl_cb(Result::OK, From b6498cbdf6c04c631ccf6a6e65a1264b455e3088 Mon Sep 17 00:00:00 2001 From: Kevin Rocard Date: Fri, 31 Mar 2017 19:36:29 -0700 Subject: [PATCH 4/5] Audio HAL: Check for buffer size overflow The audio buffer size is not provided by the client, it is computed from the sample size and the number of sample. No check was done as if the multiplication of these two numbers would produce an overflow. This leaded to erroneous memory access crashing the media server. Test: Run on target Bug: 36311550 Change-Id: I3436800ab6ac1b5e6a6aa4d03d6b96910eb54652 Signed-off-by: Kevin Rocard --- audio/2.0/default/StreamIn.cpp | 10 ++++++++-- audio/2.0/default/StreamOut.cpp | 11 +++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/audio/2.0/default/StreamIn.cpp b/audio/2.0/default/StreamIn.cpp index 97f8307bdf..4b7915117e 100644 --- a/audio/2.0/default/StreamIn.cpp +++ b/audio/2.0/default/StreamIn.cpp @@ -313,8 +313,14 @@ Return StreamIn::prepareForReading( return Void(); } std::unique_ptr tempCommandMQ(new CommandMQ(1)); - std::unique_ptr tempDataMQ( - new DataMQ(frameSize * framesCount, true /* EventFlag */)); + if (frameSize > std::numeric_limits::max() / framesCount) { + ALOGE("Requested buffer is too big, %d*%d can not fit in size_t", frameSize, framesCount); + _hidl_cb(Result::INVALID_ARGUMENTS, + CommandMQ::Descriptor(), DataMQ::Descriptor(), StatusMQ::Descriptor(), threadInfo); + return Void(); + } + std::unique_ptr tempDataMQ(new DataMQ(frameSize * framesCount, true /* EventFlag */)); + std::unique_ptr tempStatusMQ(new StatusMQ(1)); if (!tempCommandMQ->isValid() || !tempDataMQ->isValid() || !tempStatusMQ->isValid()) { ALOGE_IF(!tempCommandMQ->isValid(), "command MQ is invalid"); diff --git a/audio/2.0/default/StreamOut.cpp b/audio/2.0/default/StreamOut.cpp index cc16a82a58..1fe3a69430 100644 --- a/audio/2.0/default/StreamOut.cpp +++ b/audio/2.0/default/StreamOut.cpp @@ -297,8 +297,15 @@ Return StreamOut::prepareForWriting( return Void(); } std::unique_ptr tempCommandMQ(new CommandMQ(1)); - std::unique_ptr tempDataMQ( - new DataMQ(frameSize * framesCount, true /* EventFlag */)); + + if (frameSize > std::numeric_limits::max() / framesCount) { + ALOGE("Requested buffer is too big, %d*%d can not fit in size_t", frameSize, framesCount); + _hidl_cb(Result::INVALID_ARGUMENTS, + CommandMQ::Descriptor(), DataMQ::Descriptor(), StatusMQ::Descriptor(), threadInfo); + return Void(); + } + std::unique_ptr tempDataMQ(new DataMQ(frameSize * framesCount, true /* EventFlag */)); + std::unique_ptr tempStatusMQ(new StatusMQ(1)); if (!tempCommandMQ->isValid() || !tempDataMQ->isValid() || !tempStatusMQ->isValid()) { ALOGE_IF(!tempCommandMQ->isValid(), "command MQ is invalid"); From 40343061d5f6f309c68a54940e24d404e6cd620a Mon Sep 17 00:00:00 2001 From: Kevin Rocard Date: Fri, 31 Mar 2017 19:36:55 -0700 Subject: [PATCH 5/5] Audio HAL: Destroy EventFlag on failed prepareTo{write,read} If prepareToWrite or prepareToRead fails after EventFlag is created, it is not destroyed. This lead to strange random crashes (double free it seems). Use the RAII pattern to manage the EventFlag life cycle. Test: Run test on target Bug: 36311550 Change-Id: I53a04a62b7d12fdcc94afd8ced3e547aa6edff50 Signed-off-by: Kevin Rocard --- audio/2.0/default/StreamIn.cpp | 10 +++++++--- audio/2.0/default/StreamOut.cpp | 10 +++++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/audio/2.0/default/StreamIn.cpp b/audio/2.0/default/StreamIn.cpp index 4b7915117e..0798bbea9d 100644 --- a/audio/2.0/default/StreamIn.cpp +++ b/audio/2.0/default/StreamIn.cpp @@ -330,8 +330,11 @@ Return StreamIn::prepareForReading( CommandMQ::Descriptor(), DataMQ::Descriptor(), StatusMQ::Descriptor(), threadInfo); return Void(); } - status = EventFlag::createEventFlag(tempDataMQ->getEventFlagWord(), &mEfGroup); - if (status != OK || !mEfGroup) { + EventFlag* tempRawEfGroup{}; + status = EventFlag::createEventFlag(tempDataMQ->getEventFlagWord(), &tempRawEfGroup); + std::unique_ptr tempElfGroup(tempRawEfGroup, [](auto *ef) { + EventFlag::deleteEventFlag(&ef); }); + if (status != OK || !tempElfGroup) { ALOGE("failed creating event flag for data MQ: %s", strerror(-status)); _hidl_cb(Result::INVALID_ARGUMENTS, CommandMQ::Descriptor(), DataMQ::Descriptor(), StatusMQ::Descriptor(), threadInfo); @@ -345,7 +348,7 @@ Return StreamIn::prepareForReading( tempCommandMQ.get(), tempDataMQ.get(), tempStatusMQ.get(), - mEfGroup); + tempElfGroup.get()); if (!tempReadThread->init()) { _hidl_cb(Result::INVALID_ARGUMENTS, CommandMQ::Descriptor(), DataMQ::Descriptor(), StatusMQ::Descriptor(), threadInfo); @@ -363,6 +366,7 @@ Return StreamIn::prepareForReading( mDataMQ = std::move(tempDataMQ); mStatusMQ = std::move(tempStatusMQ); mReadThread = tempReadThread.release(); + mEfGroup = tempElfGroup.release(); threadInfo.pid = getpid(); threadInfo.tid = mReadThread->getTid(); _hidl_cb(Result::OK, diff --git a/audio/2.0/default/StreamOut.cpp b/audio/2.0/default/StreamOut.cpp index 1fe3a69430..3339b63006 100644 --- a/audio/2.0/default/StreamOut.cpp +++ b/audio/2.0/default/StreamOut.cpp @@ -315,8 +315,11 @@ Return StreamOut::prepareForWriting( CommandMQ::Descriptor(), DataMQ::Descriptor(), StatusMQ::Descriptor(), threadInfo); return Void(); } - status = EventFlag::createEventFlag(tempDataMQ->getEventFlagWord(), &mEfGroup); - if (status != OK || !mEfGroup) { + EventFlag* tempRawEfGroup{}; + status = EventFlag::createEventFlag(tempDataMQ->getEventFlagWord(), &tempRawEfGroup); + std::unique_ptr tempElfGroup(tempRawEfGroup,[](auto *ef) { + EventFlag::deleteEventFlag(&ef); }); + if (status != OK || !tempElfGroup) { ALOGE("failed creating event flag for data MQ: %s", strerror(-status)); _hidl_cb(Result::INVALID_ARGUMENTS, CommandMQ::Descriptor(), DataMQ::Descriptor(), StatusMQ::Descriptor(), threadInfo); @@ -330,7 +333,7 @@ Return StreamOut::prepareForWriting( tempCommandMQ.get(), tempDataMQ.get(), tempStatusMQ.get(), - mEfGroup); + tempElfGroup.get()); if (!tempWriteThread->init()) { _hidl_cb(Result::INVALID_ARGUMENTS, CommandMQ::Descriptor(), DataMQ::Descriptor(), StatusMQ::Descriptor(), threadInfo); @@ -348,6 +351,7 @@ Return StreamOut::prepareForWriting( mDataMQ = std::move(tempDataMQ); mStatusMQ = std::move(tempStatusMQ); mWriteThread = tempWriteThread.release(); + mEfGroup = tempElfGroup.release(); threadInfo.pid = getpid(); threadInfo.tid = mWriteThread->getTid(); _hidl_cb(Result::OK,