surfaceflinger: fix a potential child layer leak

We should not remove a child layer from its already removed parent.
Call p->removeChild only after we've checked that the ancestor is
alive.

Apply e6b63e1ae12692327f7e46d5f10d6ade5a7bf192 and this fix to
SurfaceFlinger_hwc1.cpp as well.

Bug: 37121786
Test: manual stress test
Change-Id: I7b811450a998acc4ad9690bd4eda058ce6588e14

2 files changed, 17 insertions, 3 deletions

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 6174185969..29e7bd6792 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2689,8 +2689,6 @@ status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly)
             return NO_ERROR;
         }
 
-        index = p->removeChild(layer);
-
         sp<Layer> ancestor = p;
         while (ancestor->getParent() != nullptr) {
             ancestor = ancestor->getParent();
@@ -2699,6 +2697,8 @@ status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly)
             ALOGE("removeLayer called with a layer whose parent has been removed");
             return NAME_NOT_FOUND;
         }
+
+        index = p->removeChild(layer);
     } else {
         index = mCurrentState.layersSortedByZ.remove(layer);
     }
diff --git a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp
index 3d421d2154..0904fab4aa 100644
--- a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp
+++ b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp
@@ -2326,8 +2326,13 @@ status_t SurfaceFlinger::addClientLayer(const sp<Client>& client,
         if (parent == nullptr) {
             mCurrentState.layersSortedByZ.add(lbc);
         } else {
+            if (mCurrentState.layersSortedByZ.indexOf(parent) < 0) {
+                ALOGE("addClientLayer called with a removed parent");
+                return NAME_NOT_FOUND;
+            }
             parent->addChild(lbc);
         }
+
         mGraphicBufferProducerList.add(IInterface::asBinder(gbc));
         mLayersAdded = true;
         mNumLayers++;
@@ -2349,6 +2354,15 @@ status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly)
             return NO_ERROR;
         }
 
+        sp<Layer> ancestor = p;
+        while (ancestor->getParent() != nullptr) {
+            ancestor = ancestor->getParent();
+        }
+        if (mCurrentState.layersSortedByZ.indexOf(ancestor) < 0) {
+            ALOGE("removeLayer called with a layer whose parent has been removed");
+            return NAME_NOT_FOUND;
+        }
+
         index = p->removeChild(layer);
     } else {
         index = mCurrentState.layersSortedByZ.remove(layer);
@@ -2370,7 +2384,7 @@ status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly)
 
     mLayersPendingRemoval.add(layer);
     mLayersRemoved = true;
-    mNumLayers--;
+    mNumLayers -= 1 + layer->getChildrenCount();
     setTransactionFlags(eTransactionNeeded);
     return NO_ERROR;
 }