From 14d218b589719842142dd7455484f80025fb63f3 Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Tue, 13 Jul 2021 13:57:39 -0700 Subject: SF: Avoid promoting parent layer in binder thread Some components in SF extend the life cycle of layer objects causing them to be destroyed without the state lock held. If a binder thread promotes a layer, there is a chance it will be left holding the last reference. This will cause the layer to be destroyed in the binder thread, resulting in invalid accesses and crashes. Fix this by tracking root layers with a variable to avoid promoting parent layer in binder thread. Test: presubmit, manually test refresh rate overlay Test: go/wm-smoke Fixes: 186412934 Change-Id: Icd9f4e851bbd92c887e113e52505ce4d8eb3ea0c --- services/surfaceflinger/Layer.h | 7 +++++++ services/surfaceflinger/RefreshRateOverlay.cpp | 1 + services/surfaceflinger/SurfaceFlinger.cpp | 5 ++++- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index e726d379d6..6b56b2d924 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -796,6 +796,11 @@ public: // for symmetry with Vector::remove ssize_t removeChild(const sp& layer); sp getParent() const { return mCurrentParent.promote(); } + + // Should be called with the surfaceflinger statelock held + bool isAtRoot() const { return mIsAtRoot; } + void setIsAtRoot(bool isAtRoot) { mIsAtRoot = isAtRoot; } + bool hasParent() const { return getParent() != nullptr; } Rect getScreenBounds(bool reduceTransparentRegion = true) const; bool setChildLayer(const sp& childLayer, int32_t z); @@ -1103,6 +1108,8 @@ private: // A list of regions on this layer that should have blurs. const std::vector getBlurRegions() const; + + bool mIsAtRoot = false; }; std::ostream& operator<<(std::ostream& stream, const Layer::FrameRate& rate); diff --git a/services/surfaceflinger/RefreshRateOverlay.cpp b/services/surfaceflinger/RefreshRateOverlay.cpp index 663e62a2f5..27a1c280fb 100644 --- a/services/surfaceflinger/RefreshRateOverlay.cpp +++ b/services/surfaceflinger/RefreshRateOverlay.cpp @@ -197,6 +197,7 @@ bool RefreshRateOverlay::createLayer() { Mutex::Autolock _l(mFlinger.mStateLock); mLayer = mClient->getLayerUser(mIBinder); mLayer->setFrameRate(Layer::FrameRate(Fps(0.0f), Layer::FrameRateCompatibility::NoVote)); + mLayer->setIsAtRoot(true); // setting Layer's Z requires resorting layersSortedByZ ssize_t idx = mFlinger.mDrawingState.layersSortedByZ.indexOf(mLayer); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 2650fa0583..72700162ba 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -4181,6 +4181,7 @@ uint32_t SurfaceFlinger::setClientStateLocked( : nullptr; if (layer->reparent(parentHandle)) { if (!hadParent) { + layer->setIsAtRoot(false); mCurrentState.layersSortedByZ.remove(layer); } flags |= eTransactionNeeded | eTraversalNeeded; @@ -4447,7 +4448,8 @@ void SurfaceFlinger::onHandleDestroyed(sp& layer) { // with the idea that the parent holds a reference and will eventually // be cleaned up. However no one cleans up the top-level so we do so // here. - if (layer->getParent() == nullptr) { + if (layer->isAtRoot()) { + layer->setIsAtRoot(false); mCurrentState.layersSortedByZ.remove(layer); } markLayerPendingRemovalLocked(layer); @@ -6924,6 +6926,7 @@ sp SurfaceFlinger::handleLayerCreatedLocked(const sp& handle, bo } if (parent == nullptr && allowAddRoot) { + layer->setIsAtRoot(true); mCurrentState.layersSortedByZ.add(layer); } else if (parent == nullptr) { layer->onRemovedFromCurrentState(); -- cgit v1.2.3