Skip to content

Commit

Permalink
Merge "[native] Restore ServiceManager#getService() to return IBinder…
Browse files Browse the repository at this point in the history 
…" into main am: 91cb820

Original change: https://android-review.googlesource.com/c/platform/frameworks/native/+/3194614

Change-Id: I9b0af59f4dfb111d4069fdd7130a06dd49b6719f
Signed-off-by: Automerger Merge Worker <[email protected]>
  • Loading branch information
@smore-lore @android-build-merge-worker-robot
smore-lore authored and android-build-merge-worker-robot committed Jul 26, 2024
2 parents 4b61c23 + 91cb820 commit 075500c
Show file tree
Hide file tree
Showing 8 changed files with 95 additions and 29 deletions.
11 changes: 10 additions & 1 deletion cmds/servicemanager/ServiceManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,16 @@ ServiceManager::~ServiceManager() {
}
}

Status ServiceManager::getService(const std::string& name, os::Service* outService) {
Status ServiceManager::getService(const std::string& name, sp<IBinder>* outBinder) {
SM_PERFETTO_TRACE_FUNC(PERFETTO_TE_PROTO_FIELDS(
PERFETTO_TE_PROTO_FIELD_CSTR(kProtoServiceName, name.c_str())));

*outBinder = tryGetBinder(name, true);
// returns ok regardless of result for legacy reasons
return Status::ok();
}

Status ServiceManager::getService2(const std::string& name, os::Service* outService) {
SM_PERFETTO_TRACE_FUNC(PERFETTO_TE_PROTO_FIELDS(
PERFETTO_TE_PROTO_FIELD_CSTR(kProtoServiceName, name.c_str())));

Expand Down
3 changes: 2 additions & 1 deletion cmds/servicemanager/ServiceManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ class ServiceManager : public os::BnServiceManager, public IBinder::DeathRecipie
~ServiceManager();

// getService will try to start any services it cannot find
binder::Status getService(const std::string& name, os::Service* outService) override;
binder::Status getService(const std::string& name, sp<IBinder>* outBinder) override;
binder::Status getService2(const std::string& name, os::Service* outService) override;
binder::Status checkService(const std::string& name, os::Service* outService) override;
binder::Status addService(const std::string& name, const sp<IBinder>& binder,
bool allowIsolated, int32_t dumpPriority) override;
Expand Down
69 changes: 48 additions & 21 deletions cmds/servicemanager/test_sm.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,17 +155,23 @@ TEST(AddService, OverwriteExistingService) {
IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk());

Service outA;
EXPECT_TRUE(sm->getService("foo", &outA).isOk());
EXPECT_TRUE(sm->getService2("foo", &outA).isOk());
EXPECT_EQ(serviceA, outA.get<Service::Tag::binder>());
sp<IBinder> outBinderA;
EXPECT_TRUE(sm->getService("foo", &outBinderA).isOk());
EXPECT_EQ(serviceA, outBinderA);

// serviceA should be overwritten by serviceB
sp<IBinder> serviceB = getBinder();
EXPECT_TRUE(sm->addService("foo", serviceB, false /*allowIsolated*/,
IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk());

Service outB;
EXPECT_TRUE(sm->getService("foo", &outB).isOk());
EXPECT_TRUE(sm->getService2("foo", &outB).isOk());
EXPECT_EQ(serviceB, outB.get<Service::Tag::binder>());
sp<IBinder> outBinderB;
EXPECT_TRUE(sm->getService("foo", &outBinderB).isOk());
EXPECT_EQ(serviceB, outBinderB);
}

TEST(AddService, NoPermissions) {
Expand All @@ -188,24 +194,30 @@ TEST(GetService, HappyHappy) {
IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk());

Service out;
EXPECT_TRUE(sm->getService("foo", &out).isOk());
EXPECT_TRUE(sm->getService2("foo", &out).isOk());
EXPECT_EQ(service, out.get<Service::Tag::binder>());
sp<IBinder> outBinder;
EXPECT_TRUE(sm->getService("foo", &outBinder).isOk());
EXPECT_EQ(service, outBinder);
}

TEST(GetService, NonExistant) {
auto sm = getPermissiveServiceManager();

Service out;
EXPECT_TRUE(sm->getService("foo", &out).isOk());
EXPECT_TRUE(sm->getService2("foo", &out).isOk());
EXPECT_EQ(nullptr, out.get<Service::Tag::binder>());
sp<IBinder> outBinder;
EXPECT_TRUE(sm->getService("foo", &outBinder).isOk());
EXPECT_EQ(nullptr, outBinder);
}

TEST(GetService, NoPermissionsForGettingService) {
std::unique_ptr<MockAccess> access = std::make_unique<NiceMock<MockAccess>>();

EXPECT_CALL(*access, getCallingContext()).WillRepeatedly(Return(Access::CallingContext{}));
EXPECT_CALL(*access, canAdd(_, _)).WillOnce(Return(true));
EXPECT_CALL(*access, canFind(_, _)).WillOnce(Return(false));
EXPECT_CALL(*access, canFind(_, _)).WillRepeatedly(Return(false));

sp<ServiceManager> sm = sp<NiceMock<MockServiceManager>>::make(std::move(access));

Expand All @@ -214,22 +226,28 @@ TEST(GetService, NoPermissionsForGettingService) {

Service out;
// returns nullptr but has OK status for legacy compatibility
EXPECT_TRUE(sm->getService("foo", &out).isOk());
EXPECT_TRUE(sm->getService2("foo", &out).isOk());
EXPECT_EQ(nullptr, out.get<Service::Tag::binder>());
sp<IBinder> outBinder;
EXPECT_TRUE(sm->getService("foo", &outBinder).isOk());
EXPECT_EQ(nullptr, outBinder);
}

TEST(GetService, AllowedFromIsolated) {
std::unique_ptr<MockAccess> access = std::make_unique<NiceMock<MockAccess>>();

EXPECT_CALL(*access, getCallingContext())
// something adds it
.WillOnce(Return(Access::CallingContext{}))
// next call is from isolated app
.WillOnce(Return(Access::CallingContext{
.uid = AID_ISOLATED_START,
}));
// something adds it
.WillOnce(Return(Access::CallingContext{}))
// next calls is from isolated app
.WillOnce(Return(Access::CallingContext{
.uid = AID_ISOLATED_START,
}))
.WillOnce(Return(Access::CallingContext{
.uid = AID_ISOLATED_START,
}));
EXPECT_CALL(*access, canAdd(_, _)).WillOnce(Return(true));
EXPECT_CALL(*access, canFind(_, _)).WillOnce(Return(true));
EXPECT_CALL(*access, canFind(_, _)).WillRepeatedly(Return(true));

sp<ServiceManager> sm = sp<NiceMock<MockServiceManager>>::make(std::move(access));

Expand All @@ -238,20 +256,26 @@ TEST(GetService, AllowedFromIsolated) {
IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk());

Service out;
EXPECT_TRUE(sm->getService("foo", &out).isOk());
EXPECT_TRUE(sm->getService2("foo", &out).isOk());
EXPECT_EQ(service, out.get<Service::Tag::binder>());
sp<IBinder> outBinder;
EXPECT_TRUE(sm->getService("foo", &outBinder).isOk());
EXPECT_EQ(service, outBinder);
}

TEST(GetService, NotAllowedFromIsolated) {
std::unique_ptr<MockAccess> access = std::make_unique<NiceMock<MockAccess>>();

EXPECT_CALL(*access, getCallingContext())
// something adds it
.WillOnce(Return(Access::CallingContext{}))
// next call is from isolated app
.WillOnce(Return(Access::CallingContext{
.uid = AID_ISOLATED_START,
}));
// something adds it
.WillOnce(Return(Access::CallingContext{}))
// next calls is from isolated app
.WillOnce(Return(Access::CallingContext{
.uid = AID_ISOLATED_START,
}))
.WillOnce(Return(Access::CallingContext{
.uid = AID_ISOLATED_START,
}));
EXPECT_CALL(*access, canAdd(_, _)).WillOnce(Return(true));

// TODO(b/136023468): when security check is first, this should be called first
Expand All @@ -264,8 +288,11 @@ TEST(GetService, NotAllowedFromIsolated) {

Service out;
// returns nullptr but has OK status for legacy compatibility
EXPECT_TRUE(sm->getService("foo", &out).isOk());
EXPECT_TRUE(sm->getService2("foo", &out).isOk());
EXPECT_EQ(nullptr, out.get<Service::Tag::binder>());
sp<IBinder> outBinder;
EXPECT_TRUE(sm->getService("foo", &outBinder).isOk());
EXPECT_EQ(nullptr, outBinder);
}

TEST(ListServices, NoPermissions) {
Expand Down
13 changes: 11 additions & 2 deletions libs/binder/BackendUnifiedServiceManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,19 @@ BackendUnifiedServiceManager::BackendUnifiedServiceManager(const sp<AidlServiceM
sp<AidlServiceManager> BackendUnifiedServiceManager::getImpl() {
return mTheRealServiceManager;
}

binder::Status BackendUnifiedServiceManager::getService(const ::std::string& name,
os::Service* _out) {
sp<IBinder>* _aidl_return) {
os::Service service;
binder::Status status = getService2(name, &service);
*_aidl_return = service.get<os::Service::Tag::binder>();
return status;
}

binder::Status BackendUnifiedServiceManager::getService2(const ::std::string& name,
os::Service* _out) {
os::Service service;
binder::Status status = mTheRealServiceManager->getService(name, &service);
binder::Status status = mTheRealServiceManager->getService2(name, &service);
toBinderService(service, _out);
return status;
}
Expand Down
3 changes: 2 additions & 1 deletion libs/binder/BackendUnifiedServiceManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ class BackendUnifiedServiceManager : public android::os::BnServiceManager {
explicit BackendUnifiedServiceManager(const sp<os::IServiceManager>& impl);

sp<os::IServiceManager> getImpl();
binder::Status getService(const ::std::string& name, os::Service* out) override;
binder::Status getService(const ::std::string& name, sp<IBinder>* _aidl_return) override;
binder::Status getService2(const ::std::string& name, os::Service* out) override;
binder::Status checkService(const ::std::string& name, os::Service* out) override;
binder::Status addService(const ::std::string& name, const sp<IBinder>& service,
bool allowIsolated, int32_t dumpPriority) override;
Expand Down
2 changes: 1 addition & 1 deletion libs/binder/IServiceManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ class ServiceManagerShim : public IServiceManager
// mUnifiedServiceManager->getService so that it can be overridden in ServiceManagerHostShim.
virtual Status realGetService(const std::string& name, sp<IBinder>* _aidl_return) {
Service service;
Status status = mUnifiedServiceManager->getService(name, &service);
Status status = mUnifiedServiceManager->getService2(name, &service);
*_aidl_return = service.get<Service::Tag::binder>();
return status;
}
Expand Down
16 changes: 15 additions & 1 deletion libs/binder/aidl/android/os/IServiceManager.aidl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,23 @@ interface IServiceManager {
* exists for legacy purposes.
*
* Returns null if the service does not exist.
*
* @deprecated TODO(b/355394904): Use getService2 instead.
*/
@UnsupportedAppUsage
Service getService(@utf8InCpp String name);
@nullable IBinder getService(@utf8InCpp String name);

/**
* Retrieve an existing service called @a name from the
* service manager.
*
* This is the same as checkService (returns immediately) but
* exists for legacy purposes.
*
* Returns an enum Service that can be of different types. The
* enum value is null if the service does not exist.
*/
Service getService2(@utf8InCpp String name);

/**
* Retrieve an existing service called @a name from the service
Expand Down
7 changes: 6 additions & 1 deletion libs/binder/servicedispatcher.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,12 @@ int Dispatch(const char* name, const ServiceRetriever& serviceRetriever,
class ServiceManagerProxyToNative : public android::os::BnServiceManager {
public:
ServiceManagerProxyToNative(const sp<android::os::IServiceManager>& impl) : mImpl(impl) {}
android::binder::Status getService(const std::string&, android::os::Service*) override {
android::binder::Status getService(const std::string&,
android::sp<android::IBinder>*) override {
// We can't send BpBinder for regular binder over RPC.
return android::binder::Status::fromStatusT(android::INVALID_OPERATION);
}
android::binder::Status getService2(const std::string&, android::os::Service*) override {
// We can't send BpBinder for regular binder over RPC.
return android::binder::Status::fromStatusT(android::INVALID_OPERATION);
}
Expand Down

0 comments on commit 075500c

Please sign in to comment.