From 43564fd82bcbe66865c8bcb8c23d85df4bf95dd4 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Thu, 8 Aug 2019 17:27:12 -0700 Subject: binderStabilityTest: rewrite pieces w/ no AIDL Since AIDL sets interface stability now, we can't use AIDL interfaces in this test. A custom C++/NDK interface has instead been written there so that we can set different stability levels in the same compilation unit. Writing custom interfaces like this is strongly discouraged and is not supported. However, in order to test different stability levels in the same module, we need to be able to do this, and android testing infrastructure doesn't easily support running things in multiple compilation contexts yet. Bug: 138467287 Test: binderStabilityTest Change-Id: I4461b82baf4c9850cfb4c32d7b0c5ca82fa83a74 --- libs/binder/tests/Android.bp | 1 - libs/binder/tests/IBinderStabilityTest.aidl | 14 +- libs/binder/tests/IBinderStabilityTestSub.aidl | 19 -- libs/binder/tests/binderStabilityTest.cpp | 250 ++++++++++++++----------- 4 files changed, 151 insertions(+), 133 deletions(-) delete mode 100644 libs/binder/tests/IBinderStabilityTestSub.aidl diff --git a/libs/binder/tests/Android.bp b/libs/binder/tests/Android.bp index d59a40d8d9..bc457ce30c 100644 --- a/libs/binder/tests/Android.bp +++ b/libs/binder/tests/Android.bp @@ -142,7 +142,6 @@ aidl_interface { name: "binderStabilityTestIface", srcs: [ "IBinderStabilityTest.aidl", - "IBinderStabilityTestSub.aidl", ], } diff --git a/libs/binder/tests/IBinderStabilityTest.aidl b/libs/binder/tests/IBinderStabilityTest.aidl index 9559196db4..36e1c2cbc4 100644 --- a/libs/binder/tests/IBinderStabilityTest.aidl +++ b/libs/binder/tests/IBinderStabilityTest.aidl @@ -14,34 +14,32 @@ * limitations under the License. */ -import IBinderStabilityTestSub; - // DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS! // THIS IS ONLY FOR TESTING! interface IBinderStabilityTest { // DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS! // THIS IS ONLY FOR TESTING! - void sendBinder(IBinderStabilityTestSub binder); + void sendBinder(IBinder binder); // DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS! // THIS IS ONLY FOR TESTING! - void sendAndCallBinder(IBinderStabilityTestSub binder); + void sendAndCallBinder(IBinder binder); // DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS! // THIS IS ONLY FOR TESTING! - IBinderStabilityTestSub returnNoStabilityBinder(); + IBinder returnNoStabilityBinder(); // DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS! // THIS IS ONLY FOR TESTING! - IBinderStabilityTestSub returnLocalStabilityBinder(); + IBinder returnLocalStabilityBinder(); // DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS! // THIS IS ONLY FOR TESTING! - IBinderStabilityTestSub returnVintfStabilityBinder(); + IBinder returnVintfStabilityBinder(); // DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS! // THIS IS ONLY FOR TESTING! - IBinderStabilityTestSub returnVendorStabilityBinder(); + IBinder returnVendorStabilityBinder(); } // DO NOT EVER IN A MILLION YEARS WRITE AN INTERFACE LIKE THIS! // THIS IS ONLY FOR TESTING! diff --git a/libs/binder/tests/IBinderStabilityTestSub.aidl b/libs/binder/tests/IBinderStabilityTestSub.aidl deleted file mode 100644 index a81ebf97b9..0000000000 --- a/libs/binder/tests/IBinderStabilityTestSub.aidl +++ /dev/null @@ -1,19 +0,0 @@ -/* - * Copyright (C) 2019 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -interface IBinderStabilityTestSub { - void userDefinedTransaction(); -} diff --git a/libs/binder/tests/binderStabilityTest.cpp b/libs/binder/tests/binderStabilityTest.cpp index 936b82109a..490850ebf4 100644 --- a/libs/binder/tests/binderStabilityTest.cpp +++ b/libs/binder/tests/binderStabilityTest.cpp @@ -26,9 +26,7 @@ #include -#include "aidl/BnBinderStabilityTestSub.h" #include "aidl/BnBinderStabilityTest.h" -#include "BnBinderStabilityTestSub.h" #include "BnBinderStabilityTest.h" using namespace android; @@ -36,167 +34,217 @@ using namespace ndk; using android::binder::Status; using android::internal::Stability; // for testing only! -const String16 kNoStabilityServer = String16("binder_stability_test_service_low"); -const String16 kCompilationUnitServer = String16("binder_stability_test_service_compl"); -const String16 kVintfServer = String16("binder_stability_test_service_vintf"); +const String16 kSystemStabilityServer = String16("binder_stability_test_service_system"); -const String16 kCompilationUnitNdkServer = String16("binder_stability_test_service_compl"); - -class BadStabilityTestSub : public BnBinderStabilityTestSub { +// This is handwritten so that we can test different stability levels w/o having the AIDL +// compiler assign them. Hand-writing binder interfaces is considered a bad practice +// sanity reasons. YOU SHOULD DEFINE AN AIDL INTERFACE INSTEAD! +class BadStableBinder : public BBinder { public: - Status userDefinedTransaction() { - return Status::ok(); + static constexpr uint32_t USER_TRANSACTION = IBinder::FIRST_CALL_TRANSACTION; + static String16 kDescriptor; + + bool gotUserTransaction = false; + + static status_t doUserTransaction(const sp& binder) { + Parcel data, reply; + data.writeInterfaceToken(kDescriptor); + return binder->transact(USER_TRANSACTION, data, &reply, 0/*flags*/); + } + + status_t onTransact(uint32_t code, + const Parcel& data, Parcel* reply, uint32_t flags) override { + if (code == USER_TRANSACTION) { + // not interested in this kind of stability. Make sure + // we have a test failure + LOG_ALWAYS_FATAL_IF(!data.enforceInterface(kDescriptor)); + + gotUserTransaction = true; + + ALOGE("binder stability: Got user transaction"); + return OK; + } + return BBinder::onTransact(code, data, reply, flags); } - static sp system() { - sp iface = new BadStabilityTestSub(); - // NO! NO! NO! NO! DO NOT EVERY DO SOMETHING LIKE THIS? - // WHAT ARE YOU CRAZY? IT'S VERY DANGEROUS - Stability::markCompilationUnit(iface.get()); // <- BAD, NO! DO NOT COPY + static sp undef() { + sp iface = new BadStableBinder(); return iface; } - static sp vintf() { - sp iface = new BadStabilityTestSub(); - // NO! NO! NO! NO! DO NOT EVERY DO SOMETHING LIKE THIS? - // WHAT ARE YOU CRAZY? IT'S VERY DANGEROUS - Stability::markVintf(iface.get()); // <- BAD, NO! DO NOT COPY + static sp system() { + sp iface = new BadStableBinder(); + Stability::markCompilationUnit(iface.get()); // <- for test only return iface; } - static sp vendor() { - sp iface = new BadStabilityTestSub(); - // NO! NO! NO! NO! DO NOT EVERY DO SOMETHING LIKE THIS? - // WHAT ARE YOU CRAZY? IT'S VERY DANGEROUS - Stability::markVndk(iface.get()); // <- BAD, NO! DO NOT COPY + static sp vintf() { + sp iface = new BadStableBinder(); + Stability::markVintf(iface.get()); // <- for test only + return iface; + } + + static sp vendor() { + sp iface = new BadStableBinder(); + Stability::markVndk(iface.get()); // <- for test only return iface; } }; +String16 BadStableBinder::kDescriptor = String16("BadStableBinder.test"); // NO! NO! NO! Do not even think of doing something like this! // This is for testing! If a class like this was actually used in production, // it would ruin everything! -class BadStabilityTester : public BnBinderStabilityTest { +class MyBinderStabilityTest : public BnBinderStabilityTest { public: - Status sendBinder(const sp& /*binder*/) override { + Status sendBinder(const sp& /*binder*/) override { return Status::ok(); } - Status sendAndCallBinder(const sp& binder) override { - Stability::debugLogStability("sendAndCallBinder got binder", IInterface::asBinder(binder)); - return binder->userDefinedTransaction(); + Status sendAndCallBinder(const sp& binder) override { + Stability::debugLogStability("sendAndCallBinder got binder", binder); + return Status::fromExceptionCode(BadStableBinder::doUserTransaction(binder)); } - Status returnNoStabilityBinder(sp* _aidl_return) override { - *_aidl_return = new BadStabilityTestSub(); + Status returnNoStabilityBinder(sp* _aidl_return) override { + *_aidl_return = BadStableBinder::undef(); return Status::ok(); } - Status returnLocalStabilityBinder(sp* _aidl_return) override { - *_aidl_return = BadStabilityTestSub::system(); + Status returnLocalStabilityBinder(sp* _aidl_return) override { + *_aidl_return = BadStableBinder::system(); return Status::ok(); } - Status returnVintfStabilityBinder(sp* _aidl_return) override { - *_aidl_return = BadStabilityTestSub::vintf(); + Status returnVintfStabilityBinder(sp* _aidl_return) override { + *_aidl_return = BadStableBinder::vintf(); return Status::ok(); } - Status returnVendorStabilityBinder(sp* _aidl_return) override { - *_aidl_return = BadStabilityTestSub::vendor(); + Status returnVendorStabilityBinder(sp* _aidl_return) override { + *_aidl_return = BadStableBinder::vendor(); return Status::ok(); } }; -void checkSystemStabilityBinder(const sp& complServer) { - EXPECT_TRUE(complServer->sendBinder(new BadStabilityTestSub()).isOk()); - EXPECT_TRUE(complServer->sendBinder(BadStabilityTestSub::system()).isOk()); - EXPECT_TRUE(complServer->sendBinder(BadStabilityTestSub::vintf()).isOk()); - EXPECT_TRUE(complServer->sendBinder(BadStabilityTestSub::vendor()).isOk()); +TEST(BinderStability, CantCallVendorBinderInSystemContext) { + sp serverBinder = android::defaultServiceManager()->getService(kSystemStabilityServer); + auto server = interface_cast(serverBinder); + + ASSERT_NE(nullptr, server.get()); + ASSERT_NE(nullptr, IInterface::asBinder(server)->remoteBinder()); - EXPECT_TRUE(complServer->sendAndCallBinder(new BadStabilityTestSub()).isOk()); - EXPECT_TRUE(complServer->sendAndCallBinder(BadStabilityTestSub::system()).isOk()); - EXPECT_TRUE(complServer->sendAndCallBinder(BadStabilityTestSub::vintf()).isOk()); + EXPECT_TRUE(server->sendBinder(BadStableBinder::undef()).isOk()); + EXPECT_TRUE(server->sendBinder(BadStableBinder::system()).isOk()); + EXPECT_TRUE(server->sendBinder(BadStableBinder::vintf()).isOk()); + EXPECT_TRUE(server->sendBinder(BadStableBinder::vendor()).isOk()); - // !!! user-defined transaction may not be stable for remote server !!! - EXPECT_FALSE(complServer->sendAndCallBinder(BadStabilityTestSub::vendor()).isOk()); + { + sp binder = BadStableBinder::undef(); + EXPECT_TRUE(server->sendAndCallBinder(binder).isOk()); + EXPECT_TRUE(binder->gotUserTransaction); + } + { + sp binder = BadStableBinder::system(); + EXPECT_TRUE(server->sendAndCallBinder(binder).isOk()); + EXPECT_TRUE(binder->gotUserTransaction); + } + { + sp binder = BadStableBinder::vintf(); + EXPECT_TRUE(server->sendAndCallBinder(binder).isOk()); + EXPECT_TRUE(binder->gotUserTransaction); + } + { + // !!! user-defined transaction may not be stable for remote server !!! + // !!! so, it does not work !!! + sp binder = BadStableBinder::vendor(); + EXPECT_EQ(BAD_TYPE, server->sendAndCallBinder(binder).exceptionCode()); + EXPECT_FALSE(binder->gotUserTransaction); + } - sp out; - EXPECT_TRUE(complServer->returnNoStabilityBinder(&out).isOk()); + sp out; + EXPECT_TRUE(server->returnNoStabilityBinder(&out).isOk()); ASSERT_NE(nullptr, out.get()); - EXPECT_EQ(OK, IInterface::asBinder(out)->pingBinder()); - EXPECT_TRUE(out->userDefinedTransaction().isOk()); + EXPECT_EQ(OK, out->pingBinder()); + EXPECT_EQ(OK, BadStableBinder::doUserTransaction(out)); - EXPECT_TRUE(complServer->returnLocalStabilityBinder(&out).isOk()); + EXPECT_TRUE(server->returnLocalStabilityBinder(&out).isOk()); ASSERT_NE(nullptr, out.get()); - EXPECT_EQ(OK, IInterface::asBinder(out)->pingBinder()); - EXPECT_TRUE(out->userDefinedTransaction().isOk()); + EXPECT_EQ(OK, out->pingBinder()); + EXPECT_EQ(OK, BadStableBinder::doUserTransaction(out)); - EXPECT_TRUE(complServer->returnVintfStabilityBinder(&out).isOk()); + EXPECT_TRUE(server->returnVintfStabilityBinder(&out).isOk()); ASSERT_NE(nullptr, out.get()); - EXPECT_EQ(OK, IInterface::asBinder(out)->pingBinder()); - EXPECT_TRUE(out->userDefinedTransaction().isOk()); + EXPECT_EQ(OK, out->pingBinder()); + EXPECT_EQ(OK, BadStableBinder::doUserTransaction(out)); - EXPECT_TRUE(complServer->returnVendorStabilityBinder(&out).isOk()); + EXPECT_TRUE(server->returnVendorStabilityBinder(&out).isOk()); ASSERT_NE(nullptr, out.get()); // !!! libbinder-defined transaction works !!! - EXPECT_EQ(OK, IInterface::asBinder(out)->pingBinder()); + EXPECT_EQ(OK, out->pingBinder()); // !!! user-defined transaction may not be stable !!! - EXPECT_FALSE(out->userDefinedTransaction().isOk()); + // !!! so, it does not work !!! + EXPECT_EQ(BAD_TYPE, BadStableBinder::doUserTransaction(out)); } -TEST(BinderStability, RemoteNoStabilityServer) { - sp remoteBinder = android::defaultServiceManager()->getService(kNoStabilityServer); - auto remoteServer = interface_cast(remoteBinder); +// This is handwritten so that we can test different stability levels w/o having the AIDL +// compiler assign them. Hand-writing binder interfaces is considered a bad practice +// sanity reasons. YOU SHOULD DEFINE AN AIDL INTERFACE INSTEAD! - ASSERT_NE(nullptr, remoteServer.get()); - ASSERT_NE(nullptr, IInterface::asBinder(remoteServer)->remoteBinder()); - - checkSystemStabilityBinder(remoteServer); +struct NdkBinderStable_DataClass { + bool gotUserTransaction = false; +}; +void* NdkBadStableBinder_Class_onCreate(void* args) { + LOG_ALWAYS_FATAL_IF(args != nullptr, "Takes no args"); + return static_cast(new NdkBinderStable_DataClass); } - -TEST(BinderStability, RemoteLowStabilityServer) { - sp remoteBinder = android::defaultServiceManager()->getService(kCompilationUnitServer); - auto remoteServer = interface_cast(remoteBinder); - - ASSERT_NE(nullptr, remoteServer.get()); - ASSERT_NE(nullptr, IInterface::asBinder(remoteServer)->remoteBinder()); - - checkSystemStabilityBinder(remoteServer); +void NdkBadStableBinder_Class_onDestroy(void* userData) { + delete static_cast(userData); } +NdkBinderStable_DataClass* NdkBadStableBinder_getUserData(AIBinder* binder) { + LOG_ALWAYS_FATAL_IF(binder == nullptr); + void* userData = AIBinder_getUserData(binder); + LOG_ALWAYS_FATAL_IF(userData == nullptr, "null data - binder is remote?"); -TEST(BinderStability, RemoteVintfServer) { - sp remoteBinder = android::defaultServiceManager()->getService(kVintfServer); - auto remoteServer = interface_cast(remoteBinder); + return static_cast(userData); +} +binder_status_t NdkBadStableBinder_Class_onTransact( + AIBinder* binder, transaction_code_t code, const AParcel* /*in*/, AParcel* /*out*/) { - ASSERT_NE(nullptr, remoteServer.get()); - ASSERT_NE(nullptr, IInterface::asBinder(remoteServer)->remoteBinder()); + if (code == BadStableBinder::USER_TRANSACTION) { + ALOGE("ndk binder stability: Got user transaction"); + NdkBadStableBinder_getUserData(binder)->gotUserTransaction = true; + return STATUS_OK; + } - checkSystemStabilityBinder(remoteServer); + return STATUS_UNKNOWN_TRANSACTION; } -class NdkBadStabilityTestSub : public aidl::BnBinderStabilityTestSub { - ScopedAStatus userDefinedTransaction() { - return ScopedAStatus::ok(); - } -}; +static AIBinder_Class* kNdkBadStableBinder = + AIBinder_Class_define(String8(BadStableBinder::kDescriptor).c_str(), + NdkBadStableBinder_Class_onCreate, + NdkBadStableBinder_Class_onDestroy, + NdkBadStableBinder_Class_onTransact); + // for testing only to get around __ANDROID_VNDK__ guard. extern "C" void AIBinder_markVendorStability(AIBinder* binder); // <- BAD DO NOT COPY -TEST(BinderStability, NdkClientOfRemoteServer) { +TEST(BinderStability, NdkCantCallVendorBinderInSystemContext) { SpAIBinder binder = SpAIBinder(AServiceManager_getService( - String8(kCompilationUnitServer).c_str())); + String8(kSystemStabilityServer).c_str())); std::shared_ptr remoteServer = aidl::IBinderStabilityTest::fromBinder(binder); ASSERT_NE(nullptr, remoteServer.get()); - std::shared_ptr vendor = SharedRefBase::make(); - - // TODO: not ideal: binder must be held once it is marked - SpAIBinder vendorBinder = vendor->asBinder(); - AIBinder_markVendorStability(vendorBinder.get()); + SpAIBinder comp = SpAIBinder(AIBinder_new(kNdkBadStableBinder, nullptr /*args*/)); + EXPECT_TRUE(remoteServer->sendBinder(comp).isOk()); + EXPECT_TRUE(remoteServer->sendAndCallBinder(comp).isOk()); + EXPECT_TRUE(NdkBadStableBinder_getUserData(comp.get())->gotUserTransaction); + SpAIBinder vendor = SpAIBinder(AIBinder_new(kNdkBadStableBinder, nullptr /*args*/)); + AIBinder_markVendorStability(vendor.get()); EXPECT_TRUE(remoteServer->sendBinder(vendor).isOk()); EXPECT_FALSE(remoteServer->sendAndCallBinder(vendor).isOk()); + EXPECT_FALSE(NdkBadStableBinder_getUserData(vendor.get())->gotUserTransaction); } class MarksStabilityInConstructor : public BBinder { @@ -234,16 +282,8 @@ int main(int argc, char** argv) { // child process prctl(PR_SET_PDEATHSIG, SIGHUP); - sp noStability = new BadStabilityTester; - android::defaultServiceManager()->addService(kNoStabilityServer, noStability); - - sp compil = new BadStabilityTester; - Stability::markCompilationUnit(compil.get()); - android::defaultServiceManager()->addService(kCompilationUnitServer, compil); - - sp vintf = new BadStabilityTester; - Stability::markVintf(vintf.get()); - android::defaultServiceManager()->addService(kVintfServer, vintf); + sp server = new MyBinderStabilityTest; + android::defaultServiceManager()->addService(kSystemStabilityServer, server); IPCThreadState::self()->joinThreadPool(true); exit(1); // should not reach -- cgit v1.2.3-59-g8ed1b