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.

Bug: 4483046

Change-Id: I5262bf1124d9a65ec6f8ffd8e367356fc33a7536
This commit is contained in:
Mathias Agopian 2011-05-03 16:21:41 -07:00
parent 8949eabac7
commit 02cf75e0ca
2 changed files with 21 additions and 11 deletions

View File

@ -1030,15 +1030,15 @@ status_t SurfaceFlinger::addLayer_l(const sp<LayerBase>& layer)
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)
@ -2253,15 +2253,17 @@ status_t Client::initCheck() const {
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++) {
@ -2271,9 +2273,11 @@ void Client::detachLayer(const LayerBaseClient* layer)
}
}
}
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));

View File

@ -66,7 +66,7 @@ public:
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;
@ -82,9 +82,15 @@ private:
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