From 14e4cfae36aa878c6a9838299bc7b9aa42a16dfa Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Thu, 3 Jun 2021 21:40:45 +0000 Subject: libbinder: +2 bytes in BBinder from stability rep sizeof(BBinder) may not be changed unless 1,000s of people in many different companies fundamentally change the way they work. So, with precious few bits to spare, we make room by changing the way that binder stability reserves space on the wire. Now, it uses the least significant 16-bits of the 32-bits which is reserved on the wire. The sideeffect of this straightforward implementation is that the wire protocol is slightly changed. This is an intentional change in order to exercise its instability, perhaps as an early warning. Bug: 166282674 Test: boot, binderLibTest Change-Id: I654fcd2cc9d20cbac557d1a176a5095c491d88cf --- libs/binder/Parcel.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'libs/binder/Parcel.cpp') diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index d19b4d83fb..10188fee6e 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -173,8 +173,8 @@ static void release_object(const sp& proc, status_t Parcel::finishFlattenBinder(const sp& binder) { internal::Stability::tryMarkCompilationUnit(binder.get()); - auto category = internal::Stability::getCategory(binder.get()); - return writeInt32(category.repr()); + int16_t rep = internal::Stability::getCategory(binder.get()).repr(); + return writeInt32(rep); } status_t Parcel::finishUnflattenBinder( @@ -184,7 +184,8 @@ status_t Parcel::finishUnflattenBinder( status_t status = readInt32(&stability); if (status != OK) return status; - status = internal::Stability::setRepr(binder.get(), stability, true /*log*/); + status = internal::Stability::setRepr(binder.get(), static_cast(stability), + true /*log*/); if (status != OK) return status; *out = binder; -- cgit v1.2.3-59-g8ed1b From d67c8e89d2589866f509640bfbac3799c1eca4f8 Mon Sep 17 00:00:00 2001 From: Kalesh Singh Date: Tue, 29 Dec 2020 15:46:25 -0500 Subject: libbinder: Add binder already sent checks These operations should only be done before the binder object is sent out to another process: - setRequestingSid - setMinSchedulerPolicy - setInheritRt - setExtension Add log and abort if these are attempted after the binder object has been sent already. Bug: 166282674 Test: binderParcelTest Change-Id: Id2c1d0dc783cad75754a06a3047cf6c7bf704c63 --- libs/binder/Binder.cpp | 28 +++++++++++++++++++++++++--- libs/binder/Parcel.cpp | 8 +++++--- libs/binder/include/binder/Binder.h | 10 +++++++++- libs/binder/tests/binderLibTest.cpp | 8 ++++++++ 4 files changed, 47 insertions(+), 7 deletions(-) (limited to 'libs/binder/Parcel.cpp') diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp index c83c383513..415b44e683 100644 --- a/libs/binder/Binder.cpp +++ b/libs/binder/Binder.cpp @@ -197,9 +197,7 @@ public: // --------------------------------------------------------------------------- -BBinder::BBinder() : mExtras(nullptr), mStability(0) -{ -} +BBinder::BBinder() : mExtras(nullptr), mStability(0), mParceled(false) {} bool BBinder::isBinderAlive() const { @@ -322,6 +320,10 @@ bool BBinder::isRequestingSid() void BBinder::setRequestingSid(bool requestingSid) { + ALOGW_IF(mParceled, + "setRequestingSid() should not be called after a binder object " + "is parceled/sent to another process"); + Extras* e = mExtras.load(std::memory_order_acquire); if (!e) { @@ -344,6 +346,10 @@ sp BBinder::getExtension() { } void BBinder::setMinSchedulerPolicy(int policy, int priority) { + ALOGW_IF(mParceled, + "setMinSchedulerPolicy() should not be called after a binder object " + "is parceled/sent to another process"); + switch (policy) { case SCHED_NORMAL: LOG_ALWAYS_FATAL_IF(priority < -20 || priority > 19, "Invalid priority for SCHED_NORMAL: %d", priority); @@ -391,6 +397,10 @@ bool BBinder::isInheritRt() { } void BBinder::setInheritRt(bool inheritRt) { + ALOGW_IF(mParceled, + "setInheritRt() should not be called after a binder object " + "is parceled/sent to another process"); + Extras* e = mExtras.load(std::memory_order_acquire); if (!e) { @@ -410,10 +420,22 @@ pid_t BBinder::getDebugPid() { } void BBinder::setExtension(const sp& extension) { + ALOGW_IF(mParceled, + "setExtension() should not be called after a binder object " + "is parceled/sent to another process"); + Extras* e = getOrCreateExtras(); e->mExtension = extension; } +bool BBinder::wasParceled() { + return mParceled; +} + +void BBinder::setParceled() { + mParceled = true; +} + status_t BBinder::setRpcClientDebug(const Parcel& data) { if constexpr (!kEnableRpcDevServers) { ALOGW("%s: disallowed because RPC is not enabled", __PRETTY_FUNCTION__); diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 10188fee6e..232a70c894 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -196,8 +196,11 @@ static constexpr inline int schedPolicyMask(int policy, int priority) { return (priority & FLAT_BINDER_FLAG_PRIORITY_MASK) | ((policy & 3) << FLAT_BINDER_FLAG_SCHED_POLICY_SHIFT); } -status_t Parcel::flattenBinder(const sp& binder) -{ +status_t Parcel::flattenBinder(const sp& binder) { + BBinder* local = nullptr; + if (binder) local = binder->localBinder(); + if (local) local->setParceled(); + if (isForRpc()) { if (binder) { status_t status = writeInt32(1); // non-null @@ -223,7 +226,6 @@ status_t Parcel::flattenBinder(const sp& binder) } if (binder != nullptr) { - BBinder *local = binder->localBinder(); if (!local) { BpBinder *proxy = binder->remoteBinder(); if (proxy == nullptr) { diff --git a/libs/binder/include/binder/Binder.h b/libs/binder/include/binder/Binder.h index 27db32c27d..d162dda16c 100644 --- a/libs/binder/include/binder/Binder.h +++ b/libs/binder/include/binder/Binder.h @@ -94,6 +94,13 @@ public: pid_t getDebugPid(); + // Whether this binder has been sent to another process. + bool wasParceled(); + // Consider this binder as parceled (setup/init-related calls should no + // longer by called. This is automatically set by when this binder is sent + // to another process. + void setParceled(); + [[nodiscard]] status_t setRpcClientDebug(android::base::unique_fd clientFd, uint32_t maxRpcThreads); @@ -120,7 +127,8 @@ private: friend ::android::internal::Stability; int16_t mStability; - int16_t mReserved0; + bool mParceled; + uint8_t mReserved0; #ifdef __LP64__ int32_t mReserved1; diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp index 3289b5f765..12b54c2d1b 100644 --- a/libs/binder/tests/binderLibTest.cpp +++ b/libs/binder/tests/binderLibTest.cpp @@ -443,6 +443,14 @@ class TestDeathRecipient : public IBinder::DeathRecipient, public BinderLibTestE }; }; +TEST_F(BinderLibTest, WasParceled) { + auto binder = sp::make(); + EXPECT_FALSE(binder->wasParceled()); + Parcel data; + data.writeStrongBinder(binder); + EXPECT_TRUE(binder->wasParceled()); +} + TEST_F(BinderLibTest, NopTransaction) { Parcel data, reply; EXPECT_THAT(m_server->transact(BINDER_LIB_TEST_NOP_TRANSACTION, data, &reply), -- cgit v1.2.3-59-g8ed1b