fix a memory corruption where a SF Client could be used after it's been destroyed
diff --git a/libs/surfaceflinger/Layer.cpp b/libs/surfaceflinger/Layer.cpp
index 5b260c4f..218cb39 100644
--- a/libs/surfaceflinger/Layer.cpp
+++ b/libs/surfaceflinger/Layer.cpp
@@ -47,7 +47,7 @@
// ---------------------------------------------------------------------------
-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 @@
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 @@
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 @@
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 @@
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 @@
// client could be blocked, so signal them so they get a
// chance to reevaluate their condition.
- mFlinger->scheduleBroadcast(client);
+ scheduleBroadcast();
}
}
@@ -638,7 +645,7 @@
"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 @@
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 @@
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 @@
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 @@
// ---------------------------------------------------------------------------
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 @@
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 @@
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 @@
// ---------------------------------------------------------------------------
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 @@
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 @@
// ---------------------------------------------------------------------------
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 @@
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 @@
// ---------------------------------------------------------------------------
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 @@
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 @@
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 @@
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::scheduleBroadcast(Client* client)
+void SurfaceFlinger::scheduleBroadcast(const sp<Client>& client)
{
if (mLastScheduledBroadcast != client) {
mLastScheduledBroadcast = client;
@@ -886,19 +885,22 @@
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 @@
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 @@
scheduledBroadcasts.removeItemsAt(index);
}
mTokens.release(client->cid);
- delete client;
}
disconnectedClients.clear();
}
@@ -1194,14 +1195,14 @@
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 @@
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<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<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::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::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::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 @@
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 @@
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 @@
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 @@
// ---------------------------------------------------------------------------
-class Client
+class Client : public RefBase
{
public:
Client(ClientID cid, const sp<SurfaceFlinger>& flinger);
@@ -188,20 +188,20 @@
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 @@
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 @@
// 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 @@
Region mDirtyRegion;
Region mInvalidRegion;
Region mWormholeRegion;
- Client* mLastScheduledBroadcast;
- SortedVector<Client*> mScheduledBroadcasts;
+ wp<Client> mLastScheduledBroadcast;
+ SortedVector< wp<Client> > mScheduledBroadcasts;
bool mVisibleRegionsDirty;
bool mDeferReleaseConsole;
bool mFreezeDisplay;