From dbc93d7ae8c7660aab002bcec2ade86daad7c172 Mon Sep 17 00:00:00 2001
From: Garfield Tan <xutan@google.com>
Date: Tue, 26 Oct 2021 18:28:57 -0700
Subject: Save Layer instead of Handle to LayerCreatedState

Saving Handle to LayerCreatedState can lead to the destruction of Handle
when handleLayerCreatedLocked returns if the client side releases the
Handle before that. That destruction will attempt to acquire mStateLock
in the main thread again and lead to a deadlock.

Not saving Handle to LayerCreatedState will avoid Handle being destroyed
in the main thread. The destruction of Layer in the main thread is OK.

To make that happen I moved the final resolution of addToRoot to
createLayer as well because the treatments for invalid parentHandle
and invalid parentLayer are different.

Bug: 204204635
Bug: 202621651
Test: atest SurfaceFlinger_test
Test: atest libsurfaceflinger_unittest
Change-Id: I0854203c0949302d7f514d34b956eb7ae976c51f
---
 services/surfaceflinger/SurfaceFlinger.cpp | 35 +++++++++++++++---------------
 1 file changed, 18 insertions(+), 17 deletions(-)

(limited to 'services/surfaceflinger/SurfaceFlinger.cpp')

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 8d7221c1dc..48c3c108c1 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3356,8 +3356,7 @@ bool SurfaceFlinger::latchBuffers() {
 
 status_t SurfaceFlinger::addClientLayer(const sp<Client>& client, const sp<IBinder>& handle,
                                         const sp<IGraphicBufferProducer>& gbc, const sp<Layer>& lbc,
-                                        const sp<IBinder>& parentHandle,
-                                        const sp<Layer>& parentLayer, bool addToRoot,
+                                        const wp<Layer>& parent, bool addToRoot,
                                         uint32_t* outTransformHint) {
     if (mNumLayers >= ISurfaceComposer::MAX_LAYERS) {
         ALOGE("AddClientLayer failed, mNumLayers (%zu) >= MAX_LAYERS (%zu)", mNumLayers.load(),
@@ -3369,7 +3368,7 @@ status_t SurfaceFlinger::addClientLayer(const sp<Client>& client, const sp<IBind
     if (gbc != nullptr) {
         initialProducer = IInterface::asBinder(gbc);
     }
-    setLayerCreatedState(handle, lbc, parentHandle, parentLayer, initialProducer, addToRoot);
+    setLayerCreatedState(handle, lbc, parent, initialProducer, addToRoot);
 
     // Create a transaction includes the initial parent and producer.
     Vector<ComposerState> states;
@@ -4227,7 +4226,7 @@ status_t SurfaceFlinger::mirrorLayer(const sp<Client>& client, const sp<IBinder>
     }
 
     *outLayerId = mirrorLayer->sequence;
-    return addClientLayer(client, *outHandle, nullptr, mirrorLayer, nullptr, nullptr, false,
+    return addClientLayer(client, *outHandle, nullptr, mirrorLayer, nullptr, false,
                           nullptr /* outTransformHint */);
 }
 
@@ -4296,8 +4295,15 @@ status_t SurfaceFlinger::createLayer(const String8& name, const sp<Client>& clie
     }
 
     bool addToRoot = callingThreadHasUnscopedSurfaceFlingerAccess();
-    result = addClientLayer(client, *handle, *gbp, layer, parentHandle, parentLayer, addToRoot,
-                            outTransformHint);
+    wp<Layer> parent(parentHandle != nullptr ? fromHandle(parentHandle) : parentLayer);
+    if (parentHandle != nullptr && parent == nullptr) {
+        ALOGE("Invalid parent handle %p.", parentHandle.get());
+        addToRoot = false;
+    }
+    if (parentLayer != nullptr) {
+        addToRoot = false;
+    }
+    result = addClientLayer(client, *handle, *gbp, layer, parent, addToRoot, outTransformHint);
     if (result != NO_ERROR) {
         return result;
     }
@@ -6777,11 +6783,11 @@ void TransactionState::traverseStatesWithBuffers(
 }
 
 void SurfaceFlinger::setLayerCreatedState(const sp<IBinder>& handle, const wp<Layer>& layer,
-                                          const sp<IBinder>& parent, const wp<Layer> parentLayer,
-                                          const wp<IBinder>& producer, bool addToRoot) {
+                                          const wp<Layer> parent, const wp<IBinder>& producer,
+                                          bool addToRoot) {
     Mutex::Autolock lock(mCreatedLayersLock);
     mCreatedLayers[handle->localBinder()] =
-            std::make_unique<LayerCreatedState>(layer, parent, parentLayer, producer, addToRoot);
+            std::make_unique<LayerCreatedState>(layer, parent, producer, addToRoot);
 }
 
 auto SurfaceFlinger::getLayerCreatedState(const sp<IBinder>& handle) {
@@ -6819,19 +6825,14 @@ sp<Layer> SurfaceFlinger::handleLayerCreatedLocked(const sp<IBinder>& handle) {
     }
 
     sp<Layer> parent;
-    bool allowAddRoot = state->addToRoot;
     if (state->initialParent != nullptr) {
-        parent = fromHandle(state->initialParent).promote();
+        parent = state->initialParent.promote();
         if (parent == nullptr) {
-            ALOGE("Invalid parent %p", state->initialParent.get());
-            allowAddRoot = false;
+            ALOGE("Invalid parent %p", state->initialParent.unsafe_get());
         }
-    } else if (state->initialParentLayer != nullptr) {
-        parent = state->initialParentLayer.promote();
-        allowAddRoot = false;
     }
 
-    if (parent == nullptr && allowAddRoot) {
+    if (parent == nullptr && state->addToRoot) {
         layer->setIsAtRoot(true);
         mCurrentState.layersSortedByZ.add(layer);
     } else if (parent == nullptr) {
-- 
cgit v1.2.3-59-g8ed1b