fix a memory corruption where a SF Client could be used after it's been destroyed

12 files changed, 85 insertions, 71 deletions

diff --git a/libs/surfaceflinger/Layer.cpp b/libs/surfaceflinger/Layer.cpp
index 5b260c4..218cb39 100644
--- a/libs/surfaceflinger/Layer.cpp
+++ b/libs/surfaceflinger/Layer.cpp
@@ -47,7 +47,7 @@ const char* const Layer::typeID = "Layer";
 
 // ---------------------------------------------------------------------------
 
-Layer::Layer(SurfaceFlinger* flinger, DisplayID display, Client* c, int32_t i)
+Layer::Layer(SurfaceFlinger* flinger, DisplayID display, const sp<Client>& c, int32_t i)
     :   LayerBaseClient(flinger, display, c, i),
         mSecure(false),
         mFrontBufferIndex(1),
@@ -100,8 +100,7 @@ status_t Layer::ditch()
     return NO_ERROR;
 }
 
-status_t Layer::setBuffers( Client* client,
-                            uint32_t w, uint32_t h,
+status_t Layer::setBuffers( uint32_t w, uint32_t h,
                             PixelFormat format, uint32_t flags)
 {
     PixelFormatInfo info;
@@ -285,6 +284,14 @@ sp<SurfaceBuffer> Layer::peekBuffer()
     return buffer;
 }
 
+void Layer::scheduleBroadcast()
+{
+    sp<Client> ourClient(client.promote());
+    if (ourClient != 0) {
+        mFlinger->scheduleBroadcast(ourClient);
+    }
+}
+
 uint32_t Layer::doTransaction(uint32_t flags)
 {
     const Layer::State& front(drawingState());
@@ -380,7 +387,7 @@ uint32_t Layer::doTransaction(uint32_t flags)
                         eResizeBuffer1 : eResizeBuffer0;
                 android_atomic_and(~mask, &(lcblk->swapState));
                 // since a buffer became available, we can let the client go...
-                mFlinger->scheduleBroadcast(client);
+                scheduleBroadcast();
                 mResizeTransactionDone = true;
 
                 // we're being resized and there is a freeze display request,
@@ -489,7 +496,7 @@ void Layer::lockPageFlip(bool& recomputeVisibleRegions)
     if (UNLIKELY(state & eInvalidSurface)) {
         // if eInvalidSurface is set, this means the surface
         // became invalid during a transaction (NO_MEMORY for instance)
-        mFlinger->scheduleBroadcast(client);
+        scheduleBroadcast();
         return;
     }
 
@@ -627,7 +634,7 @@ void Layer::unlockPageFlip(
 
         // client could be blocked, so signal them so they get a
         // chance to reevaluate their condition.
-        mFlinger->scheduleBroadcast(client);
+        scheduleBroadcast();
     }
 }
 
@@ -638,7 +645,7 @@ void Layer::finishPageFlip()
                 "layer %p wasn't locked!", this);
         android_atomic_and(~eBusy, &(lcblk->swapState));
     }
-    mFlinger->scheduleBroadcast(client);
+    scheduleBroadcast();
 }
 
 // ---------------------------------------------------------------------------
diff --git a/libs/surfaceflinger/Layer.h b/libs/surfaceflinger/Layer.h
index e16d9f4..a19c171 100644
--- a/libs/surfaceflinger/Layer.h
+++ b/libs/surfaceflinger/Layer.h
@@ -58,7 +58,7 @@ public:
     virtual uint32_t getTypeInfo() const { return typeInfo; }
 
                  Layer(SurfaceFlinger* flinger, DisplayID display,
-                         Client* c, int32_t i);
+                         const sp<Client>& client, int32_t i);
 
         virtual ~Layer();
 
@@ -66,8 +66,7 @@ public:
         return frontBuffer().getPixelFormat();
     }
 
-    status_t setBuffers(    Client* client,
-                            uint32_t w, uint32_t h,
+    status_t setBuffers(    uint32_t w, uint32_t h,
                             PixelFormat format, uint32_t flags=0);
 
     virtual void onDraw(const Region& clip) const;
@@ -105,6 +104,7 @@ private:
     Region post(uint32_t* oldState, bool& recomputeVisibleRegions);
     sp<SurfaceBuffer> peekBuffer();
     void destroy();
+    void scheduleBroadcast();
 
     
     class SurfaceLayer : public LayerBaseClient::Surface
diff --git a/libs/surfaceflinger/LayerBase.cpp b/libs/surfaceflinger/LayerBase.cpp
index b65c983..3b86350 100644
--- a/libs/surfaceflinger/LayerBase.cpp
+++ b/libs/surfaceflinger/LayerBase.cpp
@@ -640,16 +640,17 @@ regular:
 // ---------------------------------------------------------------------------
 
 LayerBaseClient::LayerBaseClient(SurfaceFlinger* flinger, DisplayID display,
-        Client* c, int32_t i)
-    : LayerBase(flinger, display), client(c),
-      lcblk( c ? &(c->ctrlblk->layers[i]) : 0 ),
+        const sp<Client>& client, int32_t i)
+    : LayerBase(flinger, display), client(client),
+      lcblk( client!=0 ? &(client->ctrlblk->layers[i]) : 0 ),
       mIndex(i)
 {
 }
 
 void LayerBaseClient::onFirstRef()
 {    
-    if (client) {
+    sp<Client> client(this->client.promote());
+    if (client != 0) {
         client->bindLayer(this, mIndex);
         // Initialize this layer's control block
         memset(this->lcblk, 0, sizeof(layer_cblk_t));
@@ -661,13 +662,16 @@ void LayerBaseClient::onFirstRef()
 
 LayerBaseClient::~LayerBaseClient()
 {
-    if (client) {
+    sp<Client> client(this->client.promote());
+    if (client != 0) {
         client->free(mIndex);
     }
 }
 
-int32_t LayerBaseClient::serverIndex() const {
-    if (client) {
+int32_t LayerBaseClient::serverIndex() const 
+{
+    sp<Client> client(this->client.promote());
+    if (client != 0) {
         return (client->cid<<16)|mIndex;
     }
     return 0xFFFF0000 | mIndex;
diff --git a/libs/surfaceflinger/LayerBase.h b/libs/surfaceflinger/LayerBase.h
index 509dedd..b5265d2 100644
--- a/libs/surfaceflinger/LayerBase.h
+++ b/libs/surfaceflinger/LayerBase.h
@@ -297,11 +297,11 @@ public:
     virtual uint32_t getTypeInfo() const { return typeInfo; }
 
     LayerBaseClient(SurfaceFlinger* flinger, DisplayID display, 
-            Client* client, int32_t i);
+            const sp<Client>& client, int32_t i);
     virtual ~LayerBaseClient();
     virtual void onFirstRef();
 
-    Client*             const client;
+    wp<Client>          client;
     layer_cblk_t*       const lcblk;
 
     inline  uint32_t    getIdentity() const { return mIdentity; }
diff --git a/libs/surfaceflinger/LayerBlur.cpp b/libs/surfaceflinger/LayerBlur.cpp
index 3d22e3a..00abd5a 100644
--- a/libs/surfaceflinger/LayerBlur.cpp
+++ b/libs/surfaceflinger/LayerBlur.cpp
@@ -38,7 +38,7 @@ const char* const LayerBlur::typeID = "LayerBlur";
 // ---------------------------------------------------------------------------
 
 LayerBlur::LayerBlur(SurfaceFlinger* flinger, DisplayID display,
-        Client* client, int32_t i)
+        const sp<Client>& client, int32_t i)
      : LayerBaseClient(flinger, display, client, i), mCacheDirty(true),
      mRefreshCache(true), mCacheAge(0), mTextureName(-1U)
 {
diff --git a/libs/surfaceflinger/LayerBlur.h b/libs/surfaceflinger/LayerBlur.h
index 24b1156..0c3e6eb 100644
--- a/libs/surfaceflinger/LayerBlur.h
+++ b/libs/surfaceflinger/LayerBlur.h
@@ -39,7 +39,7 @@ public:
     virtual uint32_t getTypeInfo() const { return typeInfo; }
     
                 LayerBlur(SurfaceFlinger* flinger, DisplayID display,
-                        Client* client, int32_t i);
+                        const sp<Client>& client, int32_t i);
         virtual ~LayerBlur();
 
     virtual void onDraw(const Region& clip) const;
diff --git a/libs/surfaceflinger/LayerBuffer.cpp b/libs/surfaceflinger/LayerBuffer.cpp
index 8be91c9..02b3651 100644
--- a/libs/surfaceflinger/LayerBuffer.cpp
+++ b/libs/surfaceflinger/LayerBuffer.cpp
@@ -40,7 +40,7 @@ const char* const LayerBuffer::typeID = "LayerBuffer";
 // ---------------------------------------------------------------------------
 
 LayerBuffer::LayerBuffer(SurfaceFlinger* flinger, DisplayID display,
-        Client* client, int32_t i)
+        const sp<Client>& client, int32_t i)
     : LayerBaseClient(flinger, display, client, i),
       mNeedsBlending(false)
 {
diff --git a/libs/surfaceflinger/LayerBuffer.h b/libs/surfaceflinger/LayerBuffer.h
index 22af43e..587d31b 100644
--- a/libs/surfaceflinger/LayerBuffer.h
+++ b/libs/surfaceflinger/LayerBuffer.h
@@ -57,7 +57,7 @@ public:
     virtual uint32_t getTypeInfo() const { return typeInfo; }
 
             LayerBuffer(SurfaceFlinger* flinger, DisplayID display,
-                        Client* client, int32_t i);
+                    const sp<Client>& client, int32_t i);
         virtual ~LayerBuffer();
 
     virtual void onFirstRef();
diff --git a/libs/surfaceflinger/LayerDim.cpp b/libs/surfaceflinger/LayerDim.cpp
index b95e184..5cb883c 100644
--- a/libs/surfaceflinger/LayerDim.cpp
+++ b/libs/surfaceflinger/LayerDim.cpp
@@ -40,7 +40,7 @@ int32_t LayerDim::sHeight;
 // ---------------------------------------------------------------------------
 
 LayerDim::LayerDim(SurfaceFlinger* flinger, DisplayID display,
-        Client* client, int32_t i)
+        const sp<Client>& client, int32_t i)
     : LayerBaseClient(flinger, display, client, i)
 {
 }
diff --git a/libs/surfaceflinger/LayerDim.h b/libs/surfaceflinger/LayerDim.h
index d9b70b4..33bd49d 100644
--- a/libs/surfaceflinger/LayerDim.h
+++ b/libs/surfaceflinger/LayerDim.h
@@ -44,7 +44,7 @@ public:
     virtual uint32_t getTypeInfo() const { return typeInfo; }
     
                 LayerDim(SurfaceFlinger* flinger, DisplayID display,
-                        Client* client, int32_t i);
+                        const sp<Client>& client, int32_t i);
         virtual ~LayerDim();
 
     virtual void onDraw(const Region& clip) const;
diff --git a/libs/surfaceflinger/SurfaceFlinger.cpp b/libs/surfaceflinger/SurfaceFlinger.cpp
index 90e0bb5..2e7bbea 100644
--- a/libs/surfaceflinger/SurfaceFlinger.cpp
+++ b/libs/surfaceflinger/SurfaceFlinger.cpp
@@ -227,14 +227,13 @@ sp<ISurfaceFlingerClient> SurfaceFlinger::createConnection()
     Mutex::Autolock _l(mStateLock);
     uint32_t token = mTokens.acquire();
 
-    Client* client = new Client(token, this);
-    if ((client == 0) || (client->ctrlblk == 0)) {
+    sp<Client> client = new Client(token, this);
+    if (client->ctrlblk == 0) {
         mTokens.release(token);
         return 0;
     }
     status_t err = mClientsMap.add(token, client);
     if (err < 0) {
-        delete client;
         mTokens.release(token);
         return 0;
     }
@@ -246,8 +245,8 @@ sp<ISurfaceFlingerClient> SurfaceFlinger::createConnection()
 void SurfaceFlinger::destroyConnection(ClientID cid)
 {
     Mutex::Autolock _l(mStateLock);
-    Client* const client = mClientsMap.valueFor(cid);
-    if (client) {
+    sp<Client> client = mClientsMap.valueFor(cid);
+    if (client != 0) {
         // free all the layers this client owns
         Vector< wp<LayerBaseClient> > layers(client->getLayers());
         const size_t count = layers.size();
@@ -876,7 +875,7 @@ void SurfaceFlinger::unlockClients()
     }
 }
 
-void SurfaceFlinger::scheduleBroadcast(Client* client)
+void SurfaceFlinger::scheduleBroadcast(const sp<Client>& client)
 {
     if (mLastScheduledBroadcast != client) {
         mLastScheduledBroadcast = client;
@@ -886,19 +885,22 @@ void SurfaceFlinger::scheduleBroadcast(Client* client)
 
 void SurfaceFlinger::executeScheduledBroadcasts()
 {
-    SortedVector<Client*>& list = mScheduledBroadcasts;
+    SortedVector< wp<Client> >& list(mScheduledBroadcasts);
     size_t count = list.size();
     while (count--) {
-        per_client_cblk_t* const cblk = list[count]->ctrlblk;
-        if (cblk->lock.tryLock() == NO_ERROR) {
-            cblk->cv.broadcast();
-            list.removeAt(count);
-            cblk->lock.unlock();
-        } else {
-            // schedule another round
-            LOGW("executeScheduledBroadcasts() skipped, "
-                "contention on the client. We'll try again later...");
-            signalDelayedEvent(ms2ns(4));
+        sp<Client> client = list[count].promote();
+        if (client != 0) {
+            per_client_cblk_t* const cblk = client->ctrlblk;
+            if (cblk->lock.tryLock() == NO_ERROR) {
+                cblk->cv.broadcast();
+                list.removeAt(count);
+                cblk->lock.unlock();
+            } else {
+                // schedule another round
+                LOGW("executeScheduledBroadcasts() skipped, "
+                        "contention on the client. We'll try again later...");
+                signalDelayedEvent(ms2ns(4));
+            }
         }
     }
     mLastScheduledBroadcast = 0;
@@ -1090,11 +1092,11 @@ void SurfaceFlinger::free_resources_l()
     mLayersRemoved = false;
     
     // free resources associated with disconnected clients
-    SortedVector<Client*>& scheduledBroadcasts(mScheduledBroadcasts);
-    Vector<Client*>& disconnectedClients(mDisconnectedClients);
+    SortedVector< wp<Client> >& scheduledBroadcasts(mScheduledBroadcasts);
+    Vector< sp<Client> >& disconnectedClients(mDisconnectedClients);
     const size_t count = disconnectedClients.size();
     for (size_t i=0 ; i<count ; i++) {
-        Client* client = disconnectedClients[i];
+        sp<Client> client = disconnectedClients[i];
         // if this client is the scheduled broadcast list,
         // remove it from there (and we don't need to signal it
         // since it is dead).
@@ -1103,7 +1105,6 @@ void SurfaceFlinger::free_resources_l()
             scheduledBroadcasts.removeItemsAt(index);
         }
         mTokens.release(client->cid);
-        delete client;
     }
     disconnectedClients.clear();
 }
@@ -1194,14 +1195,14 @@ sp<ISurface> SurfaceFlinger::createSurface(ClientID clientId, int pid,
     sp<LayerBaseClient> layer;
     sp<LayerBaseClient::Surface> surfaceHandle;
     Mutex::Autolock _l(mStateLock);
-    Client* const c = mClientsMap.valueFor(clientId);
-    if (UNLIKELY(!c)) {
+    sp<Client> client = mClientsMap.valueFor(clientId);
+    if (UNLIKELY(client == 0)) {
         LOGE("createSurface() failed, client not found (id=%d)", clientId);
         return surfaceHandle;
     }
 
     //LOGD("createSurface for pid %d (%d x %d)", pid, w, h);
-    int32_t id = c->generateId(pid);
+    int32_t id = client->generateId(pid);
     if (uint32_t(id) >= NUM_LAYERS_MAX) {
         LOGE("createSurface() failed, generateId = %d", id);
         return surfaceHandle;
@@ -1210,16 +1211,16 @@ sp<ISurface> SurfaceFlinger::createSurface(ClientID clientId, int pid,
     switch (flags & eFXSurfaceMask) {
         case eFXSurfaceNormal:
             if (UNLIKELY(flags & ePushBuffers)) {
-                layer = createPushBuffersSurfaceLocked(c, d, id, w, h, flags);
+                layer = createPushBuffersSurfaceLocked(client, d, id, w, h, flags);
             } else {
-                layer = createNormalSurfaceLocked(c, d, id, w, h, format, flags);
+                layer = createNormalSurfaceLocked(client, d, id, w, h, format, flags);
             }
             break;
         case eFXSurfaceBlur:
-            layer = createBlurSurfaceLocked(c, d, id, w, h, flags);
+            layer = createBlurSurfaceLocked(client, d, id, w, h, flags);
             break;
         case eFXSurfaceDim:
-            layer = createDimSurfaceLocked(c, d, id, w, h, flags);
+            layer = createDimSurfaceLocked(client, d, id, w, h, flags);
             break;
     }
 
@@ -1234,7 +1235,7 @@ sp<ISurface> SurfaceFlinger::createSurface(ClientID clientId, int pid,
 }
 
 sp<LayerBaseClient> SurfaceFlinger::createNormalSurfaceLocked(
-        Client* client, DisplayID display,
+        const sp<Client>& client, DisplayID display,
         int32_t id, uint32_t w, uint32_t h, PixelFormat format, uint32_t flags)
 {
     // initialize the surfaces
@@ -1249,7 +1250,7 @@ sp<LayerBaseClient> SurfaceFlinger::createNormalSurfaceLocked(
     }
 
     sp<Layer> layer = new Layer(this, display, client, id);
-    status_t err = layer->setBuffers(client, w, h, format, flags);
+    status_t err = layer->setBuffers(w, h, format, flags);
     if (LIKELY(err == NO_ERROR)) {
         layer->initStates(w, h, flags);
         addLayer_l(layer);
@@ -1261,7 +1262,7 @@ sp<LayerBaseClient> SurfaceFlinger::createNormalSurfaceLocked(
 }
 
 sp<LayerBaseClient> SurfaceFlinger::createBlurSurfaceLocked(
-        Client* client, DisplayID display,
+        const sp<Client>& client, DisplayID display,
         int32_t id, uint32_t w, uint32_t h, uint32_t flags)
 {
     sp<LayerBlur> layer = new LayerBlur(this, display, client, id);
@@ -1271,7 +1272,7 @@ sp<LayerBaseClient> SurfaceFlinger::createBlurSurfaceLocked(
 }
 
 sp<LayerBaseClient> SurfaceFlinger::createDimSurfaceLocked(
-        Client* client, DisplayID display,
+        const sp<Client>& client, DisplayID display,
         int32_t id, uint32_t w, uint32_t h, uint32_t flags)
 {
     sp<LayerDim> layer = new LayerDim(this, display, client, id);
@@ -1281,7 +1282,7 @@ sp<LayerBaseClient> SurfaceFlinger::createDimSurfaceLocked(
 }
 
 sp<LayerBaseClient> SurfaceFlinger::createPushBuffersSurfaceLocked(
-        Client* client, DisplayID display,
+        const sp<Client>& client, DisplayID display,
         int32_t id, uint32_t w, uint32_t h, uint32_t flags)
 {
     sp<LayerBuffer> layer = new LayerBuffer(this, display, client, id);
@@ -1446,7 +1447,7 @@ status_t SurfaceFlinger::dump(int fd, const Vector<String16>& args)
         size_t s = mClientsMap.size();
         char name[64];
         for (size_t i=0 ; i<s ; i++) {
-            Client* client = mClientsMap.valueAt(i);
+            sp<Client> client = mClientsMap.valueAt(i);
             sprintf(name, "  Client (id=0x%08x)", client->cid);
             client->dump(name);
         }
@@ -1474,10 +1475,11 @@ status_t SurfaceFlinger::dump(int fd, const Vector<String16>& args)
             sp<LayerBaseClient> lbc =
                 LayerBase::dynamicCast< LayerBaseClient* >(layer.get());
             if (lbc != 0) {
+                sp<Client> client(lbc->client.promote());
                 snprintf(buffer, SIZE,
                         "      "
                         "id=0x%08x, client=0x%08x, identity=%u\n",
-                        lbc->clientIndex(), lbc->client ? lbc->client->cid : 0,
+                        lbc->clientIndex(), client.get() ? client->cid : 0,
                         lbc->getIdentity());
             }
             result.append(buffer);
@@ -1512,7 +1514,8 @@ status_t SurfaceFlinger::dump(int fd, const Vector<String16>& args)
                 mFreezeDisplay?"yes":"no", mFreezeCount,
                 mCurrentState.orientation, hw.canDraw());
         result.append(buffer);
-        snprintf(buffer, SIZE, "  purgatory size: %d\n", mLayerPurgatory.size());
+        snprintf(buffer, SIZE, "  purgatory size: %d, client count: %d\n",
+                mLayerPurgatory.size(), mClientsMap.size());
         result.append(buffer);
         const BufferAllocator& alloc(BufferAllocator::get());
         alloc.dump(result);
diff --git a/libs/surfaceflinger/SurfaceFlinger.h b/libs/surfaceflinger/SurfaceFlinger.h
index ac13340..149eef0 100644
--- a/libs/surfaceflinger/SurfaceFlinger.h
+++ b/libs/surfaceflinger/SurfaceFlinger.h
@@ -64,7 +64,7 @@ typedef int32_t ClientID;
 
 // ---------------------------------------------------------------------------
 
-class Client
+class Client : public RefBase
 {
 public:
             Client(ClientID cid, const sp<SurfaceFlinger>& flinger);
@@ -188,20 +188,20 @@ private:
             uint32_t flags);
 
     sp<LayerBaseClient> createNormalSurfaceLocked(
-            Client* client, DisplayID display,
+            const sp<Client>& client, DisplayID display,
             int32_t id, uint32_t w, uint32_t h, 
             PixelFormat format, uint32_t flags);
 
     sp<LayerBaseClient> createBlurSurfaceLocked(
-            Client* client, DisplayID display,
+            const sp<Client>& client, DisplayID display,
             int32_t id, uint32_t w, uint32_t h, uint32_t flags);
 
     sp<LayerBaseClient> createDimSurfaceLocked(
-            Client* client, DisplayID display,
+            const sp<Client>& client, DisplayID display,
             int32_t id, uint32_t w, uint32_t h, uint32_t flags);
 
     sp<LayerBaseClient> createPushBuffersSurfaceLocked(
-            Client* client, DisplayID display,
+            const sp<Client>& client, DisplayID display,
             int32_t id, uint32_t w, uint32_t h, uint32_t flags);
 
     status_t removeSurface(SurfaceID surface_id);
@@ -262,7 +262,7 @@ private:
             bool        lockPageFlip(const LayerVector& currentLayers);
             void        unlockPageFlip(const LayerVector& currentLayers);
             void        handleRepaint();
-            void        scheduleBroadcast(Client* client);
+            void        scheduleBroadcast(const sp<Client>& client);
             void        executeScheduledBroadcasts();
             void        postFramebuffer();
             void        composeSurfaces(const Region& dirty);
@@ -312,11 +312,11 @@ private:
                 
                 // protected by mStateLock (but we could use another lock)
                 Tokenizer                               mTokens;
-                DefaultKeyedVector<ClientID, Client*>   mClientsMap;
+                DefaultKeyedVector<ClientID, sp<Client> >   mClientsMap;
                 DefaultKeyedVector<SurfaceID, sp<LayerBaseClient> >   mLayerMap;
                 GraphicPlane                            mGraphicPlanes[1];
                 bool                                    mLayersRemoved;
-                Vector<Client*>                         mDisconnectedClients;
+                Vector< sp<Client> >                    mDisconnectedClients;
 
                 // constant members (no synchronization needed for access)
                 sp<MemoryDealer>            mServerHeap;
@@ -333,8 +333,8 @@ private:
                 Region                      mDirtyRegion;
                 Region                      mInvalidRegion;
                 Region                      mWormholeRegion;
-                Client*                     mLastScheduledBroadcast;
-                SortedVector<Client*>       mScheduledBroadcasts;
+                wp<Client>                  mLastScheduledBroadcast;
+                SortedVector< wp<Client> >  mScheduledBroadcasts;
                 bool                        mVisibleRegionsDirty;
                 bool                        mDeferReleaseConsole;
                 bool                        mFreezeDisplay;