commit 4f113740180b6512b43723c4728f262882dc9b45
author Mathias Agopian <mathias@google.com> Tue May 03 16:21:41 2011 -0700
committer Mathias Agopian <mathias@google.com> Tue May 03 16:21:41 2011 -0700
tree a0a71ffb48dbe6daee69d6c4e906be2103303b52
parent 1c52ae3b726efee0af2c4bc9ad9b2bbc8236de92

Fix a race-condtion in SurfaceFlinger that could lead to a crash.

Client::mLayers could be accessed from different threads.
On one side from Client::attachLayer() which is currently
called from a binder thread; on the other side from
Client::detachLayer() which is always called from the main
thread.

This could lead to a corruption of Client::mLayers.

We fix this issue by adding an internal lock to Client.

Change-Id: Ib1317d7750ed5030e6f577efe34b69fc10198bd3

services/surfaceflinger/SurfaceFlinger.cpp

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 7506f29..ad6ab8e 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1094,15 +1094,15 @@
 ssize_t SurfaceFlinger::addClientLayer(const sp<Client>& client,
         const sp<LayerBaseClient>& lbc)
 {
-    Mutex::Autolock _l(mStateLock);
-
     // attach this layer to the client
-    ssize_t name = client->attachLayer(lbc);
+    size_t name = client->attachLayer(lbc);
+
+    Mutex::Autolock _l(mStateLock);
 
     // add this layer to the current state list
     addLayer_l(lbc);
 
-    return name;
+    return ssize_t(name);
 }
 
 status_t SurfaceFlinger::removeLayer(const sp<LayerBase>& layer)
@@ -2381,15 +2381,17 @@
     return NO_ERROR;
 }
 
-ssize_t Client::attachLayer(const sp<LayerBaseClient>& layer)
+size_t Client::attachLayer(const sp<LayerBaseClient>& layer)
 {
-    int32_t name = android_atomic_inc(&mNameGenerator);
+    Mutex::Autolock _l(mLock);
+    size_t name = mNameGenerator++;
     mLayers.add(name, layer);
     return name;
 }
 
 void Client::detachLayer(const LayerBaseClient* layer)
 {
+    Mutex::Autolock _l(mLock);
     // we do a linear search here, because this doesn't happen often
     const size_t count = mLayers.size();
     for (size_t i=0 ; i<count ; i++) {
@@ -2399,9 +2401,11 @@
         }
     }
 }
-sp<LayerBaseClient> Client::getLayerUser(int32_t i) const {
+sp<LayerBaseClient> Client::getLayerUser(int32_t i) const
+{
+    Mutex::Autolock _l(mLock);
     sp<LayerBaseClient> lbc;
-    const wp<LayerBaseClient>& layer(mLayers.valueFor(i));
+    wp<LayerBaseClient> layer(mLayers.valueFor(i));
     if (layer != 0) {
         lbc = layer.promote();
         LOGE_IF(lbc==0, "getLayerUser(name=%d) is dead", int(i));

services/surfaceflinger/SurfaceFlinger.h

diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 1b36d1c..3eb4c11 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -65,7 +65,7 @@
     status_t initCheck() const;
 
     // protected by SurfaceFlinger::mStateLock
-    ssize_t attachLayer(const sp<LayerBaseClient>& layer);
+    size_t attachLayer(const sp<LayerBaseClient>& layer);
     void detachLayer(const LayerBaseClient* layer);
     sp<LayerBaseClient> getLayerUser(int32_t i) const;
 
@@ -81,9 +81,15 @@
     virtual status_t destroySurface(SurfaceID surfaceId);
     virtual status_t setState(int32_t count, const layer_state_t* states);
 
-    DefaultKeyedVector< size_t, wp<LayerBaseClient> > mLayers;
+    // constant
     sp<SurfaceFlinger> mFlinger;
-    int32_t mNameGenerator;
+
+    // protected by mLock
+    DefaultKeyedVector< size_t, wp<LayerBaseClient> > mLayers;
+    size_t mNameGenerator;
+
+    // thread-safe
+    mutable Mutex mLock;
 };
 
 class UserClient : public BnSurfaceComposerClient