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

Commit 5acf1be4 authored by Yu-Ting Tseng's avatar Yu-Ting Tseng
Browse files

Add an option to defer BC_REQUEST_FREEZE_NOTIFICATION when needed 

Before this commit, user space may send BC_REQUEST_FREEZE_NOTIFICATION
before receiving BR_CLEAR_FREEZE_NOTIFICATION_DONE for the previous
listener session on the same binder node. This leads to a race condition
where notifications could be received out of order.

Here is an example
1. User space sends BC_REQUEST_FREEZE_NOTIFICATION with cookie A
2. Kernel sends BR_FROZEN_BINDER with frozen=false to user space
3. User space sends BC_CLEAR_FREEZE_NOTIFICATION with cookie A
4. Target process is frozen
5. User space sends BC_REQUEST_FREEZE_NOTIFICATION with cookie A
6. Kernel sends BR_FROZEN_BINDER with frozen=true to user space
7. The two BR_FROZEN_BINDER from step 2 and 6 may be picked up by
   user space in an arbitrary order. This is undesired. We want the last
   notification to be frozen=true.

The commit adds a new codepath (default off) that fixes this issue by
deferring BC_REQUEST_FREEZE_NOTIFICATION until user space receives
BR_CLEAR_FREEZE_NOTIFICATION_DONE. This ensures that a new listener
session never happens until the previous one is fully completed.

Change-Id: Ia026db0b2065ad8ee4cd56c75d49b459c6319ae0
Bug: 425638507
Flag: build.RELEASE_LIBBINDER_DEFER_BC_REQUEST_FREEZE_NOTIFICATION
parent c4dc4b74

libs/binder/Android.bp

+14 −0
Original line number Diff line number Diff line
@@ -245,6 +245,10 @@ cc_defaults { 
    name: "libbinder_common_defaults",

    host_supported: true,



    defaults: [

        "libbinder_defer_bc_request_freeze_notification_flag_default",

    ],



    srcs: [

        "Binder.cpp",

        "BpBinder.cpp",
@@ -546,10 +550,20 @@ libbinder_addservice_cache_config { 
    },

}



cc_defaults {

    name: "libbinder_defer_bc_request_freeze_notification_flag_default",

    cflags: select(release_flag("RELEASE_LIBBINDER_DEFER_BC_REQUEST_FREEZE_NOTIFICATION"), {

        true: ["-DLIBBINDER_DEFER_BC_REQUEST_FREEZE_NOTIFICATION"],

        false: ["-DNO_LIBBINDER_DEFER_BC_REQUEST_FREEZE_NOTIFICATION"],

        default: ["-DNO_LIBBINDER_DEFER_BC_REQUEST_FREEZE_NOTIFICATION"],

    }),

}



cc_defaults {

    name: "libbinder_kernel_defaults",

    defaults: [

        "libbinder_client_cache_flag",

        "libbinder_defer_bc_request_freeze_notification_flag_default",

        "libbinder_addservice_cache_flag",

        "libbinder_remove_cache_static_list_flag",

    ],

libs/binder/BpBinder.cpp

+51 −3
Original line number Diff line number Diff line
@@ -36,6 +36,16 @@ 


namespace android {



namespace {

    bool waitForFrozenListenerRemovalCompletion() {

#if defined(LIBBINDER_DEFER_BC_REQUEST_FREEZE_NOTIFICATION)

        return true;

#else

        return false;

#endif

    }

}



using android::binder::unique_fd;



// ---------------------------------------------------------------------------
@@ -575,6 +585,22 @@ void BpBinder::sendObituary() 
    }

}



void BpBinder::onFrozenStateChangeListenerRemoved() {

    if (!waitForFrozenListenerRemovalCompletion()) {

        return;

    }

    {

        RpcMutexUniqueLock _l(mLock);

        if (mFrozen->callbacks.size() == 0) {

            mFrozen.reset();

        } else {

            mFrozen->isPendingClear = false;

            std::ignore =

                    IPCThreadState::self()->addFrozenStateChangeCallback(binderHandle(), this);

        }

    }

}



status_t BpBinder::addFrozenStateChangeCallback(const wp<FrozenStateChangeCallback>& callback) {

    LOG_ALWAYS_FATAL_IF(isRpcBinder(),

                        "addFrozenStateChangeCallback() is not supported for RPC Binder.");
@@ -645,6 +671,9 @@ status_t BpBinder::removeFrozenStateChangeCallback(const wp<FrozenStateChangeCal 
            mFrozen->callbacks.removeAt(i);

            if (mFrozen->callbacks.size() == 0) {

                ALOGV("Clearing freeze notification: %p handle %d\n", this, binderHandle());

                if (mFrozen->isPendingClear) {

                    return NO_ERROR;

                }

                status_t status =

                        IPCThreadState::self()->removeFrozenStateChangeCallback(binderHandle(),

                                                                                this);
@@ -654,8 +683,13 @@ status_t BpBinder::removeFrozenStateChangeCallback(const wp<FrozenStateChangeCal 
                          "%p handle %d\n",

                          statusToString(status).c_str(), this, binderHandle());

                }

                if (waitForFrozenListenerRemovalCompletion()) {

                    mFrozen->isPendingClear = true;

                    mFrozen->initialStateReceived = false;

                } else {

                    mFrozen.reset();

                }

            }

            return NO_ERROR;

        }

    }
@@ -674,6 +708,9 @@ void BpBinder::onFrozenStateChanged(bool isFrozen) { 
    if (!mFrozen) {

        return;

    }

    if (mFrozen->isPendingClear) {

        return;

    }

    bool stateChanged = !mFrozen->initialStateReceived || mFrozen->isFrozen != isFrozen;

    if (stateChanged) {

        mFrozen->isFrozen = isFrozen;
@@ -822,9 +859,20 @@ void BpBinder::onLastStrongRef(const void* /*id*/) { 
        mObituaries = nullptr;

    }

    if (mFrozen != nullptr) {

        std::ignore = IPCThreadState::self()->removeFrozenStateChangeCallback(binderHandle(), this);

        if (waitForFrozenListenerRemovalCompletion()) {

            if (!mFrozen->isPendingClear) {

                std::ignore =

                        IPCThreadState::self()->removeFrozenStateChangeCallback(binderHandle(),

                                                                                this);

                mFrozen->isPendingClear = true;

            }

            mFrozen->callbacks.clear();

        } else {

            std::ignore =

                    IPCThreadState::self()->removeFrozenStateChangeCallback(binderHandle(), this);

            mFrozen.reset();

        }

    }

    mLock.unlock();



    if (obits != nullptr) {

libs/binder/IPCThreadState.cpp

+1 −0
Original line number Diff line number Diff line
@@ -1626,6 +1626,7 @@ status_t IPCThreadState::executeCommand(int32_t cmd) 
   case BR_CLEAR_FREEZE_NOTIFICATION_DONE:

        {

            BpBinder* proxy = (BpBinder*)mIn.readPointer();

            proxy->getPrivateAccessor().onFrozenStateChangeListenerRemoved();

            proxy->getWeakRefs()->decWeak(proxy);

        }

        break;

libs/binder/include/binder/BpBinder.h

+5 −0
Original line number Diff line number Diff line
@@ -157,6 +157,9 @@ public: 
        const sp<RpcSession>& rpcSession() const { return mBinder->rpcSession(); }



        void onFrozenStateChanged(bool isFrozen) { mMutableBinder->onFrozenStateChanged(isFrozen); }

        void onFrozenStateChangeListenerRemoved() {

            mMutableBinder->onFrozenStateChangeListenerRemoved();

        }

        const BpBinder* mBinder;

        BpBinder* mMutableBinder;

    };
@@ -208,11 +211,13 @@ private: 
    };



    void onFrozenStateChanged(bool isFrozen);

    void onFrozenStateChangeListenerRemoved();



    struct FrozenStateChange {

        bool isFrozen = false;

        Vector<wp<FrozenStateChangeCallback>> callbacks;

        bool initialStateReceived = false;

        bool isPendingClear = false;

    };



    void reportOneDeath(const Obituary& obit);