From a860beaf68faa48ab18d212de25e1abc8dd6fe9b Mon Sep 17 00:00:00 2001 From: Malathi Gottam Date: Fri, 1 Sep 2023 16:48:38 +0530 Subject: screenrecord: get codec capabilities & limit frame rate configuration Screenrecord is using display refresh rate and configuring it as framerate for any dimensions (width*height) set. But few devices may not support this fps for given resolution causing encoder config failure. To avoid such issues, obtain corresponding performance point value and limit the frame rate, if its less than display refresh rate. CRs-Fixed: 3602397 Change-Id: I150590d2ce7d458af20a1740efc37efc1eb867be --- cmds/screenrecord/Android.bp | 1 + cmds/screenrecord/screenrecord.cpp | 29 ++++++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/cmds/screenrecord/Android.bp b/cmds/screenrecord/Android.bp index d0b3ce074f..6e15724941 100644 --- a/cmds/screenrecord/Android.bp +++ b/cmds/screenrecord/Android.bp @@ -43,6 +43,7 @@ cc_binary { "libmedia", "libmediandk", "libmedia_omx", + "libmedia_codeclist", "libutils", "libbinder", "libstagefright_foundation", diff --git a/cmds/screenrecord/screenrecord.cpp b/cmds/screenrecord/screenrecord.cpp index d866c18fe2..e7dae65b64 100644 --- a/cmds/screenrecord/screenrecord.cpp +++ b/cmds/screenrecord/screenrecord.cpp @@ -49,16 +49,20 @@ #include #include #include +#include #include #include #include #include #include #include +#include +#include #include #include #include #include +#include #include #include #include @@ -75,9 +79,12 @@ using android::ui::DisplayMode; using android::FrameOutput; using android::IBinder; using android::IGraphicBufferProducer; +using android::IMediaCodecList; using android::ISurfaceComposer; using android::MediaCodec; using android::MediaCodecBuffer; +using android::MediaCodecList; +using android::MediaCodecInfo; using android::Overlay; using android::PersistentSurface; using android::PhysicalDisplayId; @@ -197,7 +204,6 @@ static status_t prepareEncoder(float displayFps, sp* pCodec, format->setString(KEY_MIME, kMimeTypeAvc); format->setInt32(KEY_COLOR_FORMAT, OMX_COLOR_FormatAndroidOpaque); format->setInt32(KEY_BIT_RATE, gBitRate); - format->setFloat(KEY_FRAME_RATE, displayFps); format->setInt32(KEY_I_FRAME_INTERVAL, 10); format->setInt32(KEY_MAX_B_FRAMES, gBframes); if (gBframes > 0) { @@ -226,6 +232,27 @@ static status_t prepareEncoder(float displayFps, sp* pCodec, } } + // set frame-rate based on codec capability + const sp mcl = MediaCodecList::getInstance(); + AString codecName; + codec->getName(&codecName); + ssize_t codecIdx = mcl->findCodecByName(codecName.c_str()); + sp info = mcl->getCodecInfo(codecIdx); + sp capabilities = info->getCapabilitiesFor(kMimeTypeAvc); + const sp &details = capabilities->getDetails(); + std::string perfPointKey = "performance-point-" + std::to_string(gVideoWidth) + "x" + + std::to_string(gVideoHeight) + "-range"; + AString minPerfPoint, maxPerfPoint; + AString perfPoint; + if (details->findString(perfPointKey.c_str(), &perfPoint) + && splitString(perfPoint, "-", &minPerfPoint, &maxPerfPoint)) { + int perfPointValue = strtol(maxPerfPoint.c_str(), NULL, 10); + if (perfPointValue < displayFps) { + displayFps = perfPointValue; + } + } + format->setFloat(KEY_FRAME_RATE, displayFps); + err = codec->configure(format, NULL, NULL, MediaCodec::CONFIGURE_FLAG_ENCODE); if (err != NO_ERROR) { -- cgit v1.2.3 From c95adf13fc93ecdbf1f88cf95006a92076d26569 Mon Sep 17 00:00:00 2001 From: Shruti Bihani Date: Mon, 10 Jul 2023 08:53:42 +0000 Subject: Fix for heap buffer overflow issue flagged by fuzzer test. OOB write occurs when a value is assigned to a buffer index which is greater than the buffer size. Adding a check on buffer bounds fixes the issue. Similar checks have been added wherever applicable on other such methods of the class. Bug: 243463593 Test: Build mtp_packet_fuzzer and run on the target device (cherry picked from commit a669e34bb8e6f0f7b5d7a35144bd342271a24712) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:b3768224b8c6330768c5006d50539d27a264da95) Merged-In: Icd0f2307803a1a35e655bc08d9d4cca5e2b58a9b Change-Id: Icd0f2307803a1a35e655bc08d9d4cca5e2b58a9b --- media/mtp/MtpPacket.cpp | 40 +++++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/media/mtp/MtpPacket.cpp b/media/mtp/MtpPacket.cpp index f069a83b5f..5faaac2026 100644 --- a/media/mtp/MtpPacket.cpp +++ b/media/mtp/MtpPacket.cpp @@ -92,24 +92,46 @@ void MtpPacket::copyFrom(const MtpPacket& src) { } uint16_t MtpPacket::getUInt16(int offset) const { - return ((uint16_t)mBuffer[offset + 1] << 8) | (uint16_t)mBuffer[offset]; + if ((unsigned long)(offset+2) <= mBufferSize) { + return ((uint16_t)mBuffer[offset + 1] << 8) | (uint16_t)mBuffer[offset]; + } + else { + ALOGE("offset for buffer read is greater than buffer size!"); + abort(); + } } uint32_t MtpPacket::getUInt32(int offset) const { - return ((uint32_t)mBuffer[offset + 3] << 24) | ((uint32_t)mBuffer[offset + 2] << 16) | - ((uint32_t)mBuffer[offset + 1] << 8) | (uint32_t)mBuffer[offset]; + if ((unsigned long)(offset+4) <= mBufferSize) { + return ((uint32_t)mBuffer[offset + 3] << 24) | ((uint32_t)mBuffer[offset + 2] << 16) | + ((uint32_t)mBuffer[offset + 1] << 8) | (uint32_t)mBuffer[offset]; + } + else { + ALOGE("offset for buffer read is greater than buffer size!"); + abort(); + } } void MtpPacket::putUInt16(int offset, uint16_t value) { - mBuffer[offset++] = (uint8_t)(value & 0xFF); - mBuffer[offset++] = (uint8_t)((value >> 8) & 0xFF); + if ((unsigned long)(offset+2) <= mBufferSize) { + mBuffer[offset++] = (uint8_t)(value & 0xFF); + mBuffer[offset++] = (uint8_t)((value >> 8) & 0xFF); + } + else { + ALOGE("offset for buffer write is greater than buffer size!"); + } } void MtpPacket::putUInt32(int offset, uint32_t value) { - mBuffer[offset++] = (uint8_t)(value & 0xFF); - mBuffer[offset++] = (uint8_t)((value >> 8) & 0xFF); - mBuffer[offset++] = (uint8_t)((value >> 16) & 0xFF); - mBuffer[offset++] = (uint8_t)((value >> 24) & 0xFF); + if ((unsigned long)(offset+4) <= mBufferSize) { + mBuffer[offset++] = (uint8_t)(value & 0xFF); + mBuffer[offset++] = (uint8_t)((value >> 8) & 0xFF); + mBuffer[offset++] = (uint8_t)((value >> 16) & 0xFF); + mBuffer[offset++] = (uint8_t)((value >> 24) & 0xFF); + } + else { + ALOGE("offset for buffer write is greater than buffer size!"); + } } uint16_t MtpPacket::getContainerCode() const { -- cgit v1.2.3 From 415a187751728016295a1d2f30f12d0366efa138 Mon Sep 17 00:00:00 2001 From: Shruti Bihani Date: Thu, 13 Jul 2023 09:19:08 +0000 Subject: Fix heap-use-after-free issue flagged by fuzzer test. A data member of class MtpFfsHandle is being accessed after the class object has been freed in the fuzzer. The method accessing the data member is running in a separate thread that gets detached from its parent. Using a conditional variable with an atomic int predicate in the close() function to ensure the detached thread's execution has completed before freeing the object fixes the issue without blocking the processing mid-way. Bug: 243381410 Test: Build mtp_handle_fuzzer and run on the target device (cherry picked from commit 50bf46a3f62136386548a9187a749936bda3ee8f) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:05f6e7b4bc7549a3160404557e6d7077733f5eb5) Merged-In: I41dde165a5eba151c958b81417d9e1065af1b411 Change-Id: I41dde165a5eba151c958b81417d9e1065af1b411 --- media/mtp/MtpFfsHandle.cpp | 14 ++++++++++++++ media/mtp/MtpFfsHandle.h | 4 ++++ 2 files changed, 18 insertions(+) diff --git a/media/mtp/MtpFfsHandle.cpp b/media/mtp/MtpFfsHandle.cpp index 2ffd7759e0..ef8c9aa789 100644 --- a/media/mtp/MtpFfsHandle.cpp +++ b/media/mtp/MtpFfsHandle.cpp @@ -297,6 +297,10 @@ int MtpFfsHandle::start(bool ptp) { } void MtpFfsHandle::close() { + auto timeout = std::chrono::seconds(2); + std::unique_lock lk(m); + cv.wait_for(lk, timeout ,[this]{return child_threads==0;}); + io_destroy(mCtx); closeEndpoints(); closeConfig(); @@ -669,6 +673,11 @@ int MtpFfsHandle::sendEvent(mtp_event me) { char *temp = new char[me.length]; memcpy(temp, me.data, me.length); me.data = temp; + + std::unique_lock lk(m); + child_threads++; + lk.unlock(); + std::thread t([this, me]() { return this->doSendEvent(me); }); t.detach(); return 0; @@ -680,6 +689,11 @@ void MtpFfsHandle::doSendEvent(mtp_event me) { if (static_cast(ret) != length) PLOG(ERROR) << "Mtp error sending event thread!"; delete[] reinterpret_cast(me.data); + + std::unique_lock lk(m); + child_threads--; + lk.unlock(); + cv.notify_one(); } } // namespace android diff --git a/media/mtp/MtpFfsHandle.h b/media/mtp/MtpFfsHandle.h index e552e03bec..51cdef0953 100644 --- a/media/mtp/MtpFfsHandle.h +++ b/media/mtp/MtpFfsHandle.h @@ -60,6 +60,10 @@ protected: bool mCanceled; bool mBatchCancel; + std::mutex m; + std::condition_variable cv; + std::atomic child_threads{0}; + android::base::unique_fd mControl; // "in" from the host's perspective => sink for mtp server android::base::unique_fd mBulkIn; -- cgit v1.2.3 From 645b3094b961310293e576e53d3e1a3b6ae9038e Mon Sep 17 00:00:00 2001 From: Venkatarama Avadhani Date: Fri, 11 Aug 2023 15:19:24 +0000 Subject: Initialise VPS buffer to NULL in constructor Missing initialisation of this pointer could lead to an incorrect free if the ARTWriter object is cleared immeddiately after the constructor call. Bug: 287298721 Test: rtp_writer_fuzzer (cherry picked from https://partner-android-review.googlesource.com/q/commit:2710696b001f2e95586151c1ee337a4e3c4da48a) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:900195c1d3589c7cbf9e116f61bebaefc0519101) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:0efe2b4d6b739650039c2cab176ef11d5f5ac49c) Merged-In: I08eacd7a0201bc9a41b821e20cae916d8870147a Change-Id: I08eacd7a0201bc9a41b821e20cae916d8870147a --- media/libstagefright/rtsp/ARTPWriter.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/media/libstagefright/rtsp/ARTPWriter.cpp b/media/libstagefright/rtsp/ARTPWriter.cpp index 41f2d67bb7..bc8341078d 100644 --- a/media/libstagefright/rtsp/ARTPWriter.cpp +++ b/media/libstagefright/rtsp/ARTPWriter.cpp @@ -105,6 +105,7 @@ ARTPWriter::ARTPWriter(int fd) mRTCPAddr = mRTPAddr; mRTCPAddr.sin_port = htons(ntohs(mRTPAddr.sin_port) | 1); + mVPSBuf = NULL; mSPSBuf = NULL; mPPSBuf = NULL; -- cgit v1.2.3