Skip to content

Commit 11da150

Browse files
author
Alice Wang
committed
[native] Restore ServiceManager#getService() to return IBinder
This fixes a crash in 3p libraries. A new API ServiceManager#getService2() has been introduced to work with the Service enum type. Bug: 354674329 Bug: 355187769 Test: atest servicemanager_test Test: atest vm_accessor_test Test: Run the demo app in b/354674329 and check it works Change-Id: If1e0e9bee6dcd3cfceea69bea58ed5fbe431e81d
1 parent 725f828 commit 11da150

8 files changed

Lines changed: 95 additions & 29 deletions

File tree

‎cmds/servicemanager/ServiceManager.cpp‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,16 @@ ServiceManager::~ServiceManager() {
392392
}
393393
}
394394

395-
Status ServiceManager::getService(const std::string& name, os::Service* outService) {
395+
Status ServiceManager::getService(const std::string& name, sp<IBinder>* outBinder) {
396+
SM_PERFETTO_TRACE_FUNC(PERFETTO_TE_PROTO_FIELDS(
397+
PERFETTO_TE_PROTO_FIELD_CSTR(kProtoServiceName, name.c_str())));
398+
399+
*outBinder = tryGetBinder(name, true);
400+
// returns ok regardless of result for legacy reasons
401+
return Status::ok();
402+
}
403+
404+
Status ServiceManager::getService2(const std::string& name, os::Service* outService) {
396405
SM_PERFETTO_TRACE_FUNC(PERFETTO_TE_PROTO_FIELDS(
397406
PERFETTO_TE_PROTO_FIELD_CSTR(kProtoServiceName, name.c_str())));
398407

‎cmds/servicemanager/ServiceManager.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ class ServiceManager : public os::BnServiceManager, public IBinder::DeathRecipie
4444
~ServiceManager();
4545

4646
// getService will try to start any services it cannot find
47-
binder::Status getService(const std::string& name, os::Service* outService) override;
47+
binder::Status getService(const std::string& name, sp<IBinder>* outBinder) override;
48+
binder::Status getService2(const std::string& name, os::Service* outService) override;
4849
binder::Status checkService(const std::string& name, os::Service* outService) override;
4950
binder::Status addService(const std::string& name, const sp<IBinder>& binder,
5051
bool allowIsolated, int32_t dumpPriority) override;

‎cmds/servicemanager/test_sm.cpp‎

Lines changed: 48 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -155,17 +155,23 @@ TEST(AddService, OverwriteExistingService) {
155155
IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk());
156156

157157
Service outA;
158-
EXPECT_TRUE(sm->getService("foo", &outA).isOk());
158+
EXPECT_TRUE(sm->getService2("foo", &outA).isOk());
159159
EXPECT_EQ(serviceA, outA.get<Service::Tag::binder>());
160+
sp<IBinder> outBinderA;
161+
EXPECT_TRUE(sm->getService("foo", &outBinderA).isOk());
162+
EXPECT_EQ(serviceA, outBinderA);
160163

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

166169
Service outB;
167-
EXPECT_TRUE(sm->getService("foo", &outB).isOk());
170+
EXPECT_TRUE(sm->getService2("foo", &outB).isOk());
168171
EXPECT_EQ(serviceB, outB.get<Service::Tag::binder>());
172+
sp<IBinder> outBinderB;
173+
EXPECT_TRUE(sm->getService("foo", &outBinderB).isOk());
174+
EXPECT_EQ(serviceB, outBinderB);
169175
}
170176

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

190196
Service out;
191-
EXPECT_TRUE(sm->getService("foo", &out).isOk());
197+
EXPECT_TRUE(sm->getService2("foo", &out).isOk());
192198
EXPECT_EQ(service, out.get<Service::Tag::binder>());
199+
sp<IBinder> outBinder;
200+
EXPECT_TRUE(sm->getService("foo", &outBinder).isOk());
201+
EXPECT_EQ(service, outBinder);
193202
}
194203

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

198207
Service out;
199-
EXPECT_TRUE(sm->getService("foo", &out).isOk());
208+
EXPECT_TRUE(sm->getService2("foo", &out).isOk());
200209
EXPECT_EQ(nullptr, out.get<Service::Tag::binder>());
210+
sp<IBinder> outBinder;
211+
EXPECT_TRUE(sm->getService("foo", &outBinder).isOk());
212+
EXPECT_EQ(nullptr, outBinder);
201213
}
202214

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

206218
EXPECT_CALL(*access, getCallingContext()).WillRepeatedly(Return(Access::CallingContext{}));
207219
EXPECT_CALL(*access, canAdd(_, _)).WillOnce(Return(true));
208-
EXPECT_CALL(*access, canFind(_, _)).WillOnce(Return(false));
220+
EXPECT_CALL(*access, canFind(_, _)).WillRepeatedly(Return(false));
209221

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

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

215227
Service out;
216228
// returns nullptr but has OK status for legacy compatibility
217-
EXPECT_TRUE(sm->getService("foo", &out).isOk());
229+
EXPECT_TRUE(sm->getService2("foo", &out).isOk());
218230
EXPECT_EQ(nullptr, out.get<Service::Tag::binder>());
231+
sp<IBinder> outBinder;
232+
EXPECT_TRUE(sm->getService("foo", &outBinder).isOk());
233+
EXPECT_EQ(nullptr, outBinder);
219234
}
220235

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

224239
EXPECT_CALL(*access, getCallingContext())
225-
// something adds it
226-
.WillOnce(Return(Access::CallingContext{}))
227-
// next call is from isolated app
228-
.WillOnce(Return(Access::CallingContext{
229-
.uid = AID_ISOLATED_START,
230-
}));
240+
// something adds it
241+
.WillOnce(Return(Access::CallingContext{}))
242+
// next calls is from isolated app
243+
.WillOnce(Return(Access::CallingContext{
244+
.uid = AID_ISOLATED_START,
245+
}))
246+
.WillOnce(Return(Access::CallingContext{
247+
.uid = AID_ISOLATED_START,
248+
}));
231249
EXPECT_CALL(*access, canAdd(_, _)).WillOnce(Return(true));
232-
EXPECT_CALL(*access, canFind(_, _)).WillOnce(Return(true));
250+
EXPECT_CALL(*access, canFind(_, _)).WillRepeatedly(Return(true));
233251

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

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

240258
Service out;
241-
EXPECT_TRUE(sm->getService("foo", &out).isOk());
259+
EXPECT_TRUE(sm->getService2("foo", &out).isOk());
242260
EXPECT_EQ(service, out.get<Service::Tag::binder>());
261+
sp<IBinder> outBinder;
262+
EXPECT_TRUE(sm->getService("foo", &outBinder).isOk());
263+
EXPECT_EQ(service, outBinder);
243264
}
244265

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

248269
EXPECT_CALL(*access, getCallingContext())
249-
// something adds it
250-
.WillOnce(Return(Access::CallingContext{}))
251-
// next call is from isolated app
252-
.WillOnce(Return(Access::CallingContext{
253-
.uid = AID_ISOLATED_START,
254-
}));
270+
// something adds it
271+
.WillOnce(Return(Access::CallingContext{}))
272+
// next calls is from isolated app
273+
.WillOnce(Return(Access::CallingContext{
274+
.uid = AID_ISOLATED_START,
275+
}))
276+
.WillOnce(Return(Access::CallingContext{
277+
.uid = AID_ISOLATED_START,
278+
}));
255279
EXPECT_CALL(*access, canAdd(_, _)).WillOnce(Return(true));
256280

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

265289
Service out;
266290
// returns nullptr but has OK status for legacy compatibility
267-
EXPECT_TRUE(sm->getService("foo", &out).isOk());
291+
EXPECT_TRUE(sm->getService2("foo", &out).isOk());
268292
EXPECT_EQ(nullptr, out.get<Service::Tag::binder>());
293+
sp<IBinder> outBinder;
294+
EXPECT_TRUE(sm->getService("foo", &outBinder).isOk());
295+
EXPECT_EQ(nullptr, outBinder);
269296
}
270297

271298
TEST(ListServices, NoPermissions) {

‎libs/binder/BackendUnifiedServiceManager.cpp‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,19 @@ BackendUnifiedServiceManager::BackendUnifiedServiceManager(const sp<AidlServiceM
3333
sp<AidlServiceManager> BackendUnifiedServiceManager::getImpl() {
3434
return mTheRealServiceManager;
3535
}
36+
3637
binder::Status BackendUnifiedServiceManager::getService(const ::std::string& name,
37-
os::Service* _out) {
38+
sp<IBinder>* _aidl_return) {
39+
os::Service service;
40+
binder::Status status = getService2(name, &service);
41+
*_aidl_return = service.get<os::Service::Tag::binder>();
42+
return status;
43+
}
44+
45+
binder::Status BackendUnifiedServiceManager::getService2(const ::std::string& name,
46+
os::Service* _out) {
3847
os::Service service;
39-
binder::Status status = mTheRealServiceManager->getService(name, &service);
48+
binder::Status status = mTheRealServiceManager->getService2(name, &service);
4049
toBinderService(service, _out);
4150
return status;
4251
}

‎libs/binder/BackendUnifiedServiceManager.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@ class BackendUnifiedServiceManager : public android::os::BnServiceManager {
2626
explicit BackendUnifiedServiceManager(const sp<os::IServiceManager>& impl);
2727

2828
sp<os::IServiceManager> getImpl();
29-
binder::Status getService(const ::std::string& name, os::Service* out) override;
29+
binder::Status getService(const ::std::string& name, sp<IBinder>* _aidl_return) override;
30+
binder::Status getService2(const ::std::string& name, os::Service* out) override;
3031
binder::Status checkService(const ::std::string& name, os::Service* out) override;
3132
binder::Status addService(const ::std::string& name, const sp<IBinder>& service,
3233
bool allowIsolated, int32_t dumpPriority) override;

‎libs/binder/IServiceManager.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ class ServiceManagerShim : public IServiceManager
143143
// mUnifiedServiceManager->getService so that it can be overridden in ServiceManagerHostShim.
144144
virtual Status realGetService(const std::string& name, sp<IBinder>* _aidl_return) {
145145
Service service;
146-
Status status = mUnifiedServiceManager->getService(name, &service);
146+
Status status = mUnifiedServiceManager->getService2(name, &service);
147147
*_aidl_return = service.get<Service::Tag::binder>();
148148
return status;
149149
}

‎libs/binder/aidl/android/os/IServiceManager.aidl‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,23 @@ interface IServiceManager {
6060
* exists for legacy purposes.
6161
*
6262
* Returns null if the service does not exist.
63+
*
64+
* @deprecated TODO(b/355394904): Use getService2 instead.
6365
*/
6466
@UnsupportedAppUsage
65-
Service getService(@utf8InCpp String name);
67+
@nullable IBinder getService(@utf8InCpp String name);
68+
69+
/**
70+
* Retrieve an existing service called @a name from the
71+
* service manager.
72+
*
73+
* This is the same as checkService (returns immediately) but
74+
* exists for legacy purposes.
75+
*
76+
* Returns an enum Service that can be of different types. The
77+
* enum value is null if the service does not exist.
78+
*/
79+
Service getService2(@utf8InCpp String name);
6680

6781
/**
6882
* Retrieve an existing service called @a name from the service

‎libs/binder/servicedispatcher.cpp‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,12 @@ int Dispatch(const char* name, const ServiceRetriever& serviceRetriever,
118118
class ServiceManagerProxyToNative : public android::os::BnServiceManager {
119119
public:
120120
ServiceManagerProxyToNative(const sp<android::os::IServiceManager>& impl) : mImpl(impl) {}
121-
android::binder::Status getService(const std::string&, android::os::Service*) override {
121+
android::binder::Status getService(const std::string&,
122+
android::sp<android::IBinder>*) override {
123+
// We can't send BpBinder for regular binder over RPC.
124+
return android::binder::Status::fromStatusT(android::INVALID_OPERATION);
125+
}
126+
android::binder::Status getService2(const std::string&, android::os::Service*) override {
122127
// We can't send BpBinder for regular binder over RPC.
123128
return android::binder::Status::fromStatusT(android::INVALID_OPERATION);
124129
}

0 commit comments

Comments
 (0)