From f2c006d5fac9cfb2e5eb3ef8c960b5eab72af0ed Mon Sep 17 00:00:00 2001 From: Ana Krulec Date: Fri, 21 Jun 2019 15:37:07 -0700 Subject: SF: Renaming IdleTimer to OneShotTimer IdleTimer class is used for more than just idle timer, so renaming the class to a more appropriate name. Test: manual Bug: 132811842 Change-Id: Iabfaa28038dc90430a51536ef874618e35ed9014 --- services/surfaceflinger/RegionSamplingThread.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'services/surfaceflinger/RegionSamplingThread.h') diff --git a/services/surfaceflinger/RegionSamplingThread.h b/services/surfaceflinger/RegionSamplingThread.h index 3c6fcf3872..ac7339c42a 100644 --- a/services/surfaceflinger/RegionSamplingThread.h +++ b/services/surfaceflinger/RegionSamplingThread.h @@ -27,7 +27,7 @@ #include #include #include -#include "Scheduler/IdleTimer.h" +#include "Scheduler/OneShotTimer.h" namespace android { @@ -107,7 +107,7 @@ private: SurfaceFlinger& mFlinger; Scheduler& mScheduler; const TimingTunables mTunables; - scheduler::IdleTimer mIdleTimer; + scheduler::OneShotTimer mIdleTimer; std::unique_ptr const mPhaseCallback; -- cgit v1.2.3-59-g8ed1b From 9a02eda8d5f95d754aa908e66089750183355511 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Tue, 21 Apr 2020 17:39:34 -0700 Subject: Lookup layer handle when registering region sampling listener We must do this in order to prevent clients from providing a bogus handle when registering a region sampling listener. Fortunately, this particular path required a permissions check so it cannot be accessed from arbitrary apps on unrooted devices. But, we should not allow this type of memory corruption to be reachable by the system. Bug: 153467444 Test: libgui_test Test: Repro steps in the bug no longer reproduce Change-Id: I883506798574dfd0688371fdb6305cfad9d153fc --- libs/gui/tests/RegionSampling_test.cpp | 13 ++++++++++++ services/surfaceflinger/RegionSamplingThread.cpp | 7 +------ services/surfaceflinger/RegionSamplingThread.h | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 23 ++++++++++++++-------- services/surfaceflinger/SurfaceFlinger.h | 7 ++++++- .../tests/unittests/TestableSurfaceFlinger.h | 1 - .../tests/unittests/TransactionApplicationTest.cpp | 2 +- 7 files changed, 37 insertions(+), 18 deletions(-) (limited to 'services/surfaceflinger/RegionSamplingThread.h') diff --git a/libs/gui/tests/RegionSampling_test.cpp b/libs/gui/tests/RegionSampling_test.cpp index dbd4ef9d7e..6746b0a827 100644 --- a/libs/gui/tests/RegionSampling_test.cpp +++ b/libs/gui/tests/RegionSampling_test.cpp @@ -240,6 +240,19 @@ protected: float const luma_gray = 0.50; }; +TEST_F(RegionSamplingTest, invalidLayerHandle_doesNotCrash) { + sp composer = ComposerService::getComposerService(); + sp listener = new Listener(); + const Rect sampleArea{100, 100, 200, 200}; + // Passing in composer service as the layer handle should not crash, we'll + // treat it as a layer that no longer exists and silently allow sampling to + // occur. + status_t status = composer->addRegionSamplingListener(sampleArea, + IInterface::asBinder(composer), listener); + ASSERT_EQ(NO_ERROR, status); + composer->removeRegionSamplingListener(listener); +} + TEST_F(RegionSamplingTest, DISABLED_CollectsLuma) { fill_render(rgba_green); diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 68cd84f661..19c204cddb 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -199,13 +199,8 @@ RegionSamplingThread::~RegionSamplingThread() { } } -void RegionSamplingThread::addListener(const Rect& samplingArea, const sp& stopLayerHandle, +void RegionSamplingThread::addListener(const Rect& samplingArea, const wp& stopLayer, const sp& listener) { - wp stopLayer; - if (stopLayerHandle != nullptr && stopLayerHandle->localBinder() != nullptr) { - stopLayer = static_cast(stopLayerHandle.get())->owner; - } - sp asBinder = IInterface::asBinder(listener); asBinder->linkToDeath(this); std::lock_guard lock(mSamplingMutex); diff --git a/services/surfaceflinger/RegionSamplingThread.h b/services/surfaceflinger/RegionSamplingThread.h index 99c07c288e..b9b7a3c436 100644 --- a/services/surfaceflinger/RegionSamplingThread.h +++ b/services/surfaceflinger/RegionSamplingThread.h @@ -69,7 +69,7 @@ public: // Add a listener to receive luma notifications. The luma reported via listener will // report the median luma for the layers under the stopLayerHandle, in the samplingArea region. - void addListener(const Rect& samplingArea, const sp& stopLayerHandle, + void addListener(const Rect& samplingArea, const wp& stopLayer, const sp& listener); // Remove the listener to stop receiving median luma notifications. void removeListener(const sp& listener); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index ddf0775c7f..54b7ef39c2 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -1449,7 +1449,9 @@ status_t SurfaceFlinger::addRegionSamplingListener(const Rect& samplingArea, if (!listener || samplingArea == Rect::INVALID_RECT) { return BAD_VALUE; } - mRegionSamplingThread->addListener(samplingArea, stopLayerHandle, listener); + + const wp stopLayer = fromHandle(stopLayerHandle); + mRegionSamplingThread->addListener(samplingArea, stopLayer, listener); return NO_ERROR; } @@ -3173,7 +3175,7 @@ status_t SurfaceFlinger::addClientLayer(const sp& client, const sp parent; if (parentHandle != nullptr) { - parent = fromHandle(parentHandle); + parent = fromHandleLocked(parentHandle).promote(); if (parent == nullptr) { return NAME_NOT_FOUND; } @@ -3548,7 +3550,7 @@ uint32_t SurfaceFlinger::setClientStateLocked( sp layer = nullptr; if (s.surface) { - layer = fromHandle(s.surface); + layer = fromHandleLocked(s.surface).promote(); } else { // The client may provide us a null handle. Treat it as if the layer was removed. ALOGW("Attempt to set client state with a null layer handle"); @@ -3864,7 +3866,7 @@ status_t SurfaceFlinger::mirrorLayer(const sp& client, const sp { Mutex::Autolock _l(mStateLock); - mirrorFrom = fromHandle(mirrorFromHandle); + mirrorFrom = fromHandleLocked(mirrorFromHandle).promote(); if (!mirrorFrom) { return NAME_NOT_FOUND; } @@ -5566,7 +5568,7 @@ status_t SurfaceFlinger::captureLayers( { Mutex::Autolock lock(mStateLock); - parent = fromHandle(layerHandleBinder); + parent = fromHandleLocked(layerHandleBinder).promote(); if (parent == nullptr || parent->isRemovedFromCurrentState()) { ALOGE("captureLayers called with an invalid or removed parent"); return NAME_NOT_FOUND; @@ -5599,7 +5601,7 @@ status_t SurfaceFlinger::captureLayers( reqHeight = crop.height() * frameScale; for (const auto& handle : excludeHandles) { - sp excludeLayer = fromHandle(handle); + sp excludeLayer = fromHandleLocked(handle).promote(); if (excludeLayer != nullptr) { excludeLayers.emplace(excludeLayer); } else { @@ -6062,7 +6064,12 @@ void SurfaceFlinger::SetInputWindowsListener::onSetInputWindowsFinished() { mFlinger->setInputWindowsFinished(); } -sp SurfaceFlinger::fromHandle(const sp& handle) { +wp SurfaceFlinger::fromHandle(const sp& handle) { + Mutex::Autolock _l(mStateLock); + return fromHandleLocked(handle); +} + +wp SurfaceFlinger::fromHandleLocked(const sp& handle) { BBinder* b = nullptr; if (handle) { b = handle->localBinder(); @@ -6072,7 +6079,7 @@ sp SurfaceFlinger::fromHandle(const sp& handle) { } auto it = mLayersByLocalBinderToken.find(b); if (it != mLayersByLocalBinderToken.end()) { - return it->second.promote(); + return it->second; } return nullptr; } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index f3c481aa8b..c59d3ff580 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -332,7 +332,12 @@ public: return mTransactionCompletedThread; } - sp fromHandle(const sp& handle) REQUIRES(mStateLock); + // Converts from a binder handle to a Layer + // Returns nullptr if the handle does not point to an existing layer. + // Otherwise, returns a weak reference so that callers off the main-thread + // won't accidentally hold onto the last strong reference. + wp fromHandle(const sp& handle); + wp fromHandleLocked(const sp& handle) REQUIRES(mStateLock); // Inherit from ClientCache::ErasedRecipient void bufferErased(const client_cache_t& clientCacheId) override; diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index 6995ee0097..319a95984d 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -402,7 +402,6 @@ public: auto& mutableUseFrameRateApi() { return mFlinger->useFrameRateApi; } auto fromHandle(const sp& handle) { - Mutex::Autolock _l(mFlinger->mStateLock); return mFlinger->fromHandle(handle); } diff --git a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp index f1739e5e8a..65de48c462 100644 --- a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp +++ b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp @@ -322,7 +322,7 @@ TEST_F(TransactionApplicationTest, BlockWithPriorTransaction_SyncInputWindows) { TEST_F(TransactionApplicationTest, FromHandle) { sp badHandle; auto ret = mFlinger.fromHandle(badHandle); - EXPECT_EQ(nullptr, ret.get()); + EXPECT_EQ(nullptr, ret.promote().get()); } } // namespace android -- cgit v1.2.3-59-g8ed1b