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

Commit 14167e00 authored by Robert Carr's avatar Robert Carr
Browse files

SurfaceFlinger: Avoid calling to SystemServer with lock held. 

PermissionCache::checkPermission may make a blocking call in to system-server. We don't
have any lock ordering issues as we end up acquiring the PackageManager lock, and PM
does not call back to SF. However, we can have Binder thread exhaustion issues, in that
many threads can be calling in to the ActivityManager, which could be blocked on the WM,
which could be blocked on SF. If the number of threads calling in to AM is > than our
binder thread pool size we deadlock. I'm not sure we are totally resistant against
thread exhaustion issues but this one is easy enough to fix and there are reports of it.

Bug: 124281288
Test: Existing tests pass
Change-Id: I4ffab2df921478df020bb1089dcd13dd79d99ce6
parent f6d3044c

services/surfaceflinger/Client.cpp

+0 −1
Original line number Diff line number Diff line
@@ -17,7 +17,6 @@ 
#include <stdint.h>

#include <sys/types.h>



#include <binder/PermissionCache.h>

#include <binder/IPCThreadState.h>



#include <private/android_filesystem_config.h>

services/surfaceflinger/SurfaceFlinger.cpp

+16 −9
Original line number Diff line number Diff line
@@ -3702,11 +3702,12 @@ bool SurfaceFlinger::flushTransactionQueues() { 
        auto& [applyToken, transactionQueue] = *it;



        while (!transactionQueue.empty()) {

            const auto& [states, displays, flags, desiredPresentTime] = transactionQueue.front();

            const auto& [states, displays, flags, desiredPresentTime, privileged] =

                    transactionQueue.front();

            if (!transactionIsReadyToBeApplied(desiredPresentTime, states)) {

                break;

            }

            applyTransactionState(states, displays, flags, mPendingInputWindowCommands);

            applyTransactionState(states, displays, flags, mPendingInputWindowCommands, privileged);

            transactionQueue.pop();

        }
@@ -3770,6 +3771,9 @@ void SurfaceFlinger::setTransactionState(const Vector<ComposerState>& states, 
                                         const InputWindowCommands& inputWindowCommands,

                                         int64_t desiredPresentTime) {

    ATRACE_CALL();



    bool privileged = callingThreadHasUnscopedSurfaceFlingerAccess();



    Mutex::Autolock _l(mStateLock);



    if (containsAnyInvalidClientState(states)) {
@@ -3779,17 +3783,19 @@ void SurfaceFlinger::setTransactionState(const Vector<ComposerState>& states, 
    // If its TransactionQueue already has a pending TransactionState or if it is pending

    if (mTransactionQueues.find(applyToken) != mTransactionQueues.end() ||

        !transactionIsReadyToBeApplied(desiredPresentTime, states)) {

        mTransactionQueues[applyToken].emplace(states, displays, flags, desiredPresentTime);

        mTransactionQueues[applyToken].emplace(states, displays, flags, desiredPresentTime,

                privileged);

        setTransactionFlags(eTransactionNeeded);

        return;

    }



    applyTransactionState(states, displays, flags, inputWindowCommands);

    applyTransactionState(states, displays, flags, inputWindowCommands, privileged);

}



void SurfaceFlinger::applyTransactionState(const Vector<ComposerState>& states,

                                           const Vector<DisplayState>& displays, uint32_t flags,

                                           const InputWindowCommands& inputWindowCommands) {

                                           const InputWindowCommands& inputWindowCommands,

                                           bool privileged) {

    uint32_t transactionFlags = 0;



    if (flags & eAnimation) {
@@ -3814,7 +3820,7 @@ void SurfaceFlinger::applyTransactionState(const Vector<ComposerState>& states, 


    uint32_t clientStateFlags = 0;

    for (const ComposerState& state : states) {

        clientStateFlags |= setClientStateLocked(state);

        clientStateFlags |= setClientStateLocked(state, privileged);

    }

    // If the state doesn't require a traversal and there are callbacks, send them now

    if (!(clientStateFlags & eTraversalNeeded)) {
@@ -3912,7 +3918,7 @@ uint32_t SurfaceFlinger::setDisplayStateLocked(const DisplayState& s) { 
    return flags;

}



bool callingThreadHasUnscopedSurfaceFlingerAccess() {

bool SurfaceFlinger::callingThreadHasUnscopedSurfaceFlingerAccess() {

    IPCThreadState* ipc = IPCThreadState::self();

    const int pid = ipc->getCallingPid();

    const int uid = ipc->getCallingUid();
@@ -3923,7 +3929,8 @@ bool callingThreadHasUnscopedSurfaceFlingerAccess() { 
    return true;

}



uint32_t SurfaceFlinger::setClientStateLocked(const ComposerState& composerState) {

uint32_t SurfaceFlinger::setClientStateLocked(const ComposerState& composerState,

        bool privileged) {

    const layer_state_t& s = composerState.state;

    sp<Client> client(static_cast<Client*>(composerState.client.get()));
@@ -4024,7 +4031,7 @@ uint32_t SurfaceFlinger::setClientStateLocked(const ComposerState& composerState 
        // of cropped areas, we need to prevent non-root clients without permission ACCESS_SURFACE_FLINGER

        // (a.k.a. everyone except WindowManager and tests) from setting non rectangle preserving

        // transformations.

        if (layer->setMatrix(s.matrix, callingThreadHasUnscopedSurfaceFlingerAccess()))

        if (layer->setMatrix(s.matrix, privileged))

            flags |= eTraversalNeeded;

    }

    if (what & layer_state_t::eTransparentRegionChanged) {

services/surfaceflinger/SurfaceFlinger.h

+9 −4
Original line number Diff line number Diff line
@@ -408,6 +408,7 @@ private: 
     */

    status_t onTransact(uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) override;

    status_t dump(int fd, const Vector<String16>& args) override { return priorityDump(fd, args); }

    bool callingThreadHasUnscopedSurfaceFlingerAccess() EXCLUDES(mStateLock);



    /* ------------------------------------------------------------------------

     * ISurfaceComposer interface
@@ -553,7 +554,8 @@ private: 
     */

    void applyTransactionState(const Vector<ComposerState>& state,

                               const Vector<DisplayState>& displays, uint32_t flags,

                               const InputWindowCommands& inputWindowCommands) REQUIRES(mStateLock);

                               const InputWindowCommands& inputWindowCommands,

                               bool privileged) REQUIRES(mStateLock);

    bool flushTransactionQueues();

    uint32_t getTransactionFlags(uint32_t flags);

    uint32_t peekTransactionFlags();
@@ -565,7 +567,7 @@ private: 
    bool containsAnyInvalidClientState(const Vector<ComposerState>& states);

    bool transactionIsReadyToBeApplied(int64_t desiredPresentTime,

                                       const Vector<ComposerState>& states);

    uint32_t setClientStateLocked(const ComposerState& composerState);

    uint32_t setClientStateLocked(const ComposerState& composerState, bool privileged);

    uint32_t setDisplayStateLocked(const DisplayState& s);

    uint32_t addInputWindowCommands(const InputWindowCommands& inputWindowCommands)

            REQUIRES(mStateLock);
@@ -1055,16 +1057,19 @@ private: 
    struct TransactionState {

        TransactionState(const Vector<ComposerState>& composerStates,

                         const Vector<DisplayState>& displayStates, uint32_t transactionFlags,

                         int64_t desiredPresentTime)

                         int64_t desiredPresentTime,

                         bool privileged)

              : states(composerStates),

                displays(displayStates),

                flags(transactionFlags),

                time(desiredPresentTime) {}

                time(desiredPresentTime),

                privileged(privileged) {}



        Vector<ComposerState> states;

        Vector<DisplayState> displays;

        uint32_t flags;

        int64_t time;

        bool privileged;

    };

    std::unordered_map<sp<IBinder>, std::queue<TransactionState>, IBinderHash> mTransactionQueues;