Allow fixing race condition in lazy HAL.
This implements the updated interface for IServiceManager.
Interfaces are directly retrieved from their corresponding locations
and now servers are allowed to unregister themselves before shutting
down.
Bug: 123318663
Test: hidl_test, manual
Change-Id: Ia700d356d67a7c779262243225af5570d4c2817f
Merged-In: Ia700d356d67a7c779262243225af5570d4c2817f
diff --git a/HidlService.cpp b/HidlService.cpp
index ed39ed7..8a3a442 100644
--- a/HidlService.cpp
+++ b/HidlService.cpp
@@ -34,6 +34,9 @@
mPid = pid;
mClientCallbacks.clear();
+ mHasClients = false;
+ mGuaranteeClient = false;
+ mNoClientsCounter = false;
sendRegistrationNotifications();
}
@@ -104,25 +107,24 @@
return found;
}
-void HidlService::handleClientCallbacks(bool isCalledOnInterval) {
+ssize_t HidlService::handleClientCallbacks(bool isCalledOnInterval) {
using ::android::hardware::toBinder;
using ::android::hardware::BpHwBinder;
using ::android::hardware::IBinder;
- if (mClientCallbacks.empty()) return;
- if (mService == nullptr) return;
+ if (mService == nullptr) return -1;
// this justifies the bp cast below, no in-process HALs need this
- if (!mService->isRemote()) return;
+ if (!mService->isRemote()) return -1;
sp<IBinder> binder = toBinder(mService);
- if (binder == nullptr) return;
+ if (binder == nullptr) return -1;
sp<BpHwBinder> bpBinder = static_cast<BpHwBinder*>(binder.get());
ssize_t count = bpBinder->getNodeStrongRefCount();
// binder driver doesn't support this feature
- if (count == -1) return;
+ if (count == -1) return count;
bool hasClients = count > 1; // this process holds a strong count
@@ -141,6 +143,7 @@
}
mGuaranteeClient = false;
+ return count;
}
void HidlService::guaranteeClient() {
diff --git a/HidlService.h b/HidlService.h
index bb21d51..d7ef3b6 100644
--- a/HidlService.h
+++ b/HidlService.h
@@ -52,7 +52,10 @@
void addClientCallback(const sp<IClientCallback>& callback);
bool removeClientCallback(const sp<IClientCallback>& callback);
- void handleClientCallbacks(bool isCalledOnInterval);
+
+ // return is number of clients (-1 means this is not implemented)
+ // count includes one held by hwservicemanager
+ ssize_t handleClientCallbacks(bool isCalledOnInterval);
// when giving out a handle to a client, but the kernel might not know this yet
void guaranteeClient();
diff --git a/ServiceManager.cpp b/ServiceManager.cpp
index ea97136..165757d 100644
--- a/ServiceManager.cpp
+++ b/ServiceManager.cpp
@@ -76,6 +76,19 @@
}
}
+HidlService* ServiceManager::lookup(const std::string& fqName, const std::string& name) {
+ auto ifaceIt = mServiceMap.find(fqName);
+ if (ifaceIt == mServiceMap.end()) {
+ return nullptr;
+ }
+
+ PackageInterfaceMap &ifaceMap = ifaceIt->second;
+
+ HidlService *hidlService = ifaceMap.lookup(name);
+
+ return hidlService;
+}
+
void ServiceManager::serviceDied(uint64_t cookie, const wp<IBase>& who) {
bool serviceRemoved = false;
switch (cookie) {
@@ -220,25 +233,15 @@
return nullptr;
}
- auto ifaceIt = mServiceMap.find(fqName);
- if (ifaceIt == mServiceMap.end()) {
- tryStartService(fqName, hidlName);
- return nullptr;
- }
-
- PackageInterfaceMap &ifaceMap = ifaceIt->second;
-
- // may be modified in post-command task
- HidlService *hidlService = ifaceMap.lookup(name);
-
+ HidlService* hidlService = lookup(fqName, name);
if (hidlService == nullptr) {
- tryStartService(fqName, hidlName);
+ tryStartService(fqName, name);
return nullptr;
}
sp<IBase> service = hidlService->getService();
if (service == nullptr) {
- tryStartService(fqName, hidlName);
+ tryStartService(fqName, name);
return nullptr;
}
@@ -502,42 +505,53 @@
return service->removeListener(callback);
}
-Return<bool> ServiceManager::registerClientCallback(const sp<IBase>& server,
+Return<bool> ServiceManager::registerClientCallback(const hidl_string& hidlFqName,
+ const hidl_string& hidlName,
+ const sp<IBase>& server,
const sp<IClientCallback>& cb) {
if (server == nullptr || cb == nullptr) return false;
+ const std::string fqName = hidlFqName;
+ const std::string name = hidlName;
+
// only the server of the interface can register a client callback
pid_t pid = IPCThreadState::self()->getCallingPid();
-
- HidlService* registered = nullptr;
-
- forEachExistingService([&] (HidlService *service) {
- if (service->getPid() != pid) {
- return true; // continue
- }
-
- if (interfacesEqual(service->getService(), server)) {
- service->addClientCallback(cb);
- registered = service;
- return false; // break
- }
- return true; // continue
- });
-
- if (registered != nullptr) {
- bool linkRet = cb->linkToDeath(this, kClientCallbackDiedCookie).withDefault(false);
- if (!linkRet) {
- LOG(ERROR) << "Could not link to death for registerClientCallback";
- unregisterClientCallback(server, cb);
- registered = nullptr;
- }
+ auto context = mAcl.getContext(pid);
+ if (!mAcl.canAdd(fqName, context, pid)) {
+ return false;
}
- if (registered != nullptr) {
- registered->handleClientCallbacks(false /* isCalledOnInterval */);
+ HidlService* registered = lookup(fqName, name);
+
+ if (registered == nullptr) {
+ return false;
}
- return registered != nullptr;
+ // sanity
+ if (registered->getPid() != pid) {
+ LOG(WARNING) << "Only a server can register for client callbacks (for " << fqName
+ << "/" << name << ")";
+ return false;
+ }
+
+ sp<IBase> service = registered->getService();
+
+ if (!interfacesEqual(service, server)) {
+ LOG(WARNING) << "Tried to register client callback for " << fqName << "/" << name
+ << " but a different service is registered under this name.";
+ return false;
+ }
+
+ bool linkRet = cb->linkToDeath(this, kClientCallbackDiedCookie).withDefault(false);
+ if (!linkRet) {
+ LOG(ERROR) << "Could not link to death for registerClientCallback";
+ return false;
+ }
+
+ registered->addClientCallback(cb);
+ registered->handleClientCallbacks(false /* isCalledOnInterval */);
+
+ return true;
}
Return<bool> ServiceManager::unregisterClientCallback(const sp<IBase>& server,
@@ -591,6 +605,65 @@
return Void();
}
+Return<bool> ServiceManager::tryUnregister(const hidl_string& hidlFqName,
+ const hidl_string& hidlName,
+ const sp<IBase>& service) {
+ const std::string fqName = hidlFqName;
+ const std::string name = hidlName;
+
+ if (service == nullptr) {
+ return false;
+ }
+
+ pid_t pid = IPCThreadState::self()->getCallingPid();
+ auto context = mAcl.getContext(pid);
+
+ if (!mAcl.canAdd(fqName, context, pid)) {
+ return false;
+ }
+
+ HidlService* registered = lookup(fqName, name);
+
+ // sanity
+ if (registered->getPid() != pid) {
+ LOG(WARNING) << "Only a server can unregister itself (for " << fqName
+ << "/" << name << ")";
+ return false;
+ }
+
+ sp<IBase> server = registered->getService();
+
+ if (!interfacesEqual(service, server)) {
+ LOG(WARNING) << "Tried to unregister for " << fqName << "/" << name
+ << " but a different service is registered under this name.";
+ return false;
+ }
+
+ int clients = registered->handleClientCallbacks(false /* isCalledOnInterval */);
+
+ // clients < 0: feature not implemented or other error. Assume clients.
+ // Otherwise:
+ // - kernel driver will hold onto one refcount (during this transaction)
+ // - hwservicemanager has a refcount (guaranteed by this transaction)
+ // So, if clients > 2, then at least one other service on the system must hold a refcount.
+ if (clients < 0 || clients > 2) {
+ // client callbacks are either disabled or there are other clients
+ LOG(INFO) << "Tried to unregister for " << fqName << "/" << name
+ << " but there are clients: " << clients;
+ return false;
+ }
+
+ // will remove entire parent hierarchy
+ bool success = removeService(service, &name /*restrictToInstanceName*/);
+
+ if (registered->getService() != nullptr) {
+ LOG(ERROR) << "Bad state. Unregistration failed for " << fqName << "/" << name << ".";
+ return false;
+ }
+
+ return success;
+}
+
Return<void> ServiceManager::debugDump(debugDump_cb _cb) {
pid_t pid = IPCThreadState::self()->getCallingPid();
if (!mAcl.canList(pid)) {
diff --git a/ServiceManager.h b/ServiceManager.h
index 8cfaeb7..17d7143 100644
--- a/ServiceManager.h
+++ b/ServiceManager.h
@@ -53,7 +53,9 @@
const sp<IServiceNotification>& callback) override;
// Methods from ::android::hidl::manager::V1_2::IServiceManager follow.
- Return<bool> registerClientCallback(const sp<IBase>& server,
+ Return<bool> registerClientCallback(const hidl_string& fqName,
+ const hidl_string& name,
+ const sp<IBase>& server,
const sp<IClientCallback>& cb) override;
Return<bool> unregisterClientCallback(const sp<IBase>& server,
const sp<IClientCallback>& cb) override;
@@ -62,6 +64,9 @@
const hidl_vec<hidl_string>& chain) override;
Return<void> listManifestByInterface(const hidl_string& fqInstanceName,
listManifestByInterface_cb _hidl_cb) override;
+ Return<bool> tryUnregister(const hidl_string& fqName,
+ const hidl_string& name,
+ const sp<IBase>& service) override;
void handleClientCallbacks();
@@ -86,6 +91,8 @@
void forEachServiceEntry(std::function<bool(const HidlService *)> f) const;
void forEachServiceEntry(std::function<bool(HidlService *)> f);
+ HidlService* lookup(const std::string& fqName, const std::string& name);
+
using InstanceMap = std::map<
std::string, // instance name e.x. "manager"
std::unique_ptr<HidlService>