Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 14d218b5 authored by Vishnu Nair's avatar Vishnu Nair
Browse files

SF: Avoid promoting parent layer in binder thread 

Some components in SF extend the life cycle of layer objects
causing them to be destroyed without the state lock held. If a binder
thread promotes a layer, there is a chance it will be left holding the
last reference. This will cause the layer to be destroyed in the binder
thread, resulting in invalid accesses and crashes.

Fix this by tracking root layers with a variable to avoid promoting
parent layer in binder thread.

Test: presubmit, manually test refresh rate overlay
Test: go/wm-smoke
Fixes: 186412934
Change-Id: Icd9f4e851bbd92c887e113e52505ce4d8eb3ea0c
parent b058a604

services/surfaceflinger/Layer.h

+7 −0
Original line number Original line Diff line number Diff line
@@ -796,6 +796,11 @@ public: 
    // for symmetry with Vector::remove

    // for symmetry with Vector::remove

    ssize_t removeChild(const sp<Layer>& layer);

    ssize_t removeChild(const sp<Layer>& layer);

    sp<Layer> getParent() const { return mCurrentParent.promote(); }

    sp<Layer> getParent() const { return mCurrentParent.promote(); }



    // Should be called with the surfaceflinger statelock held

    bool isAtRoot() const { return mIsAtRoot; }

    void setIsAtRoot(bool isAtRoot) { mIsAtRoot = isAtRoot; }



    bool hasParent() const { return getParent() != nullptr; }

    bool hasParent() const { return getParent() != nullptr; }

    Rect getScreenBounds(bool reduceTransparentRegion = true) const;

    Rect getScreenBounds(bool reduceTransparentRegion = true) const;

    bool setChildLayer(const sp<Layer>& childLayer, int32_t z);

    bool setChildLayer(const sp<Layer>& childLayer, int32_t z);
@@ -1103,6 +1108,8 @@ private: 




    // A list of regions on this layer that should have blurs.

    // A list of regions on this layer that should have blurs.

    const std::vector<BlurRegion> getBlurRegions() const;

    const std::vector<BlurRegion> getBlurRegions() const;



    bool mIsAtRoot = false;

};

};





std::ostream& operator<<(std::ostream& stream, const Layer::FrameRate& rate);

std::ostream& operator<<(std::ostream& stream, const Layer::FrameRate& rate);

services/surfaceflinger/RefreshRateOverlay.cpp

+1 −0
Original line number Original line Diff line number Diff line
@@ -197,6 +197,7 @@ bool RefreshRateOverlay::createLayer() { 
    Mutex::Autolock _l(mFlinger.mStateLock);

    Mutex::Autolock _l(mFlinger.mStateLock);

    mLayer = mClient->getLayerUser(mIBinder);

    mLayer = mClient->getLayerUser(mIBinder);

    mLayer->setFrameRate(Layer::FrameRate(Fps(0.0f), Layer::FrameRateCompatibility::NoVote));

    mLayer->setFrameRate(Layer::FrameRate(Fps(0.0f), Layer::FrameRateCompatibility::NoVote));

    mLayer->setIsAtRoot(true);





    // setting Layer's Z requires resorting layersSortedByZ

    // setting Layer's Z requires resorting layersSortedByZ

    ssize_t idx = mFlinger.mDrawingState.layersSortedByZ.indexOf(mLayer);

    ssize_t idx = mFlinger.mDrawingState.layersSortedByZ.indexOf(mLayer);

services/surfaceflinger/SurfaceFlinger.cpp

+4 −1
Original line number Original line Diff line number Diff line
@@ -4181,6 +4181,7 @@ uint32_t SurfaceFlinger::setClientStateLocked( 
                : nullptr;

                : nullptr;

        if (layer->reparent(parentHandle)) {

        if (layer->reparent(parentHandle)) {

            if (!hadParent) {

            if (!hadParent) {

                layer->setIsAtRoot(false);

                mCurrentState.layersSortedByZ.remove(layer);

                mCurrentState.layersSortedByZ.remove(layer);

            }

            }

            flags |= eTransactionNeeded | eTraversalNeeded;

            flags |= eTransactionNeeded | eTraversalNeeded;
@@ -4447,7 +4448,8 @@ void SurfaceFlinger::onHandleDestroyed(sp<Layer>& layer) { 
    // with the idea that the parent holds a reference and will eventually

    // with the idea that the parent holds a reference and will eventually

    // be cleaned up. However no one cleans up the top-level so we do so

    // be cleaned up. However no one cleans up the top-level so we do so

    // here.

    // here.

    if (layer->getParent() == nullptr) {

    if (layer->isAtRoot()) {

        layer->setIsAtRoot(false);

        mCurrentState.layersSortedByZ.remove(layer);

        mCurrentState.layersSortedByZ.remove(layer);

    }

    }

    markLayerPendingRemovalLocked(layer);

    markLayerPendingRemovalLocked(layer);
@@ -6924,6 +6926,7 @@ sp<Layer> SurfaceFlinger::handleLayerCreatedLocked(const sp<IBinder>& handle, bo 
    }

    }





    if (parent == nullptr && allowAddRoot) {

    if (parent == nullptr && allowAddRoot) {

        layer->setIsAtRoot(true);

        mCurrentState.layersSortedByZ.add(layer);

        mCurrentState.layersSortedByZ.add(layer);

    } else if (parent == nullptr) {

    } else if (parent == nullptr) {

        layer->onRemovedFromCurrentState();

        layer->onRemovedFromCurrentState();