libsysutils: Fix wait loop in ServiceManager::start and ::stop

Also check the service name length.

Change-Id: Iffb82aa9e71dd96c85c05c4e2016930f4847c1e8

1 files changed, 54 insertions, 12 deletions

diff --git a/libsysutils/src/ServiceManager.cpp b/libsysutils/src/ServiceManager.cpp
index 1ba6ef0..41ac1dd 100644
--- a/libsysutils/src/ServiceManager.cpp
+++ b/libsysutils/src/ServiceManager.cpp
@@ -10,7 +10,39 @@
 ServiceManager::ServiceManager() {
 }
 
+/* The service name should not exceed SERVICE_NAME_MAX to avoid
+ * some weird things. This is due to the fact that:
+ *
+ * - Starting a service is done by writing its name to the "ctl.start"
+ *   system property. This triggers the init daemon to actually start
+ *   the service for us.
+ *
+ * - Stopping the service is done by writing its name to "ctl.stop"
+ *   in a similar way.
+ *
+ * - Reading the status of a service is done by reading the property
+ *   named "init.svc.<name>"
+ *
+ * If strlen(<name>) > (PROPERTY_KEY_MAX-1)-9, then you can start/stop
+ * the service by writing to ctl.start/stop, but you won't be able to
+ * read its state due to the truncation of "init.svc.<name>" into a
+ * zero-terminated buffer of PROPERTY_KEY_MAX characters.
+ */
+#define SERVICE_NAME_MAX  (PROPERTY_KEY_MAX-10)
+
+/* The maximum amount of time to wait for a service to start or stop,
+ * in micro-seconds (really an approximation) */
+#define  SLEEP_MAX_USEC     2000000  /* 2 seconds */
+
+/* The minimal sleeping interval between checking for the service's state
+ * when looping for SLEEP_MAX_USEC */
+#define  SLEEP_MIN_USEC      200000  /* 200 msec */
+
 int ServiceManager::start(const char *name) {
+    if (strlen(name) > SERVICE_NAME_MAX) {
+        SLOGE("Service name '%s' is too long", name);
+        return 0;
+    }
     if (isRunning(name)) {
         SLOGW("Service '%s' is already running", name);
         return 0;
@@ -19,13 +51,14 @@ int ServiceManager::start(const char *name) {
     SLOGD("Starting service '%s'", name);
     property_set("ctl.start", name);
 
-    int count = 200;
-    while(count--) {
-        sched_yield();
+    int count = SLEEP_MAX_USEC;
+    while(count > 0) {
+        usleep(SLEEP_MIN_USEC);
+        count -= SLEEP_MIN_USEC;
         if (isRunning(name))
             break;
     }
-    if (!count) {
+    if (count <= 0) {
         SLOGW("Timed out waiting for service '%s' to start", name);
         errno = ETIMEDOUT;
         return -1;
@@ -35,6 +68,10 @@ int ServiceManager::start(const char *name) {
 }
 
 int ServiceManager::stop(const char *name) {
+    if (strlen(name) > SERVICE_NAME_MAX) {
+        SLOGE("Service name '%s' is too long", name);
+        return 0;
+    }
     if (!isRunning(name)) {
         SLOGW("Service '%s' is already stopped", name);
         return 0;
@@ -43,28 +80,33 @@ int ServiceManager::stop(const char *name) {
     SLOGD("Stopping service '%s'", name);
     property_set("ctl.stop", name);
 
-    int count = 200;
-    while(count--) {
-        sched_yield();
+    int count = SLEEP_MAX_USEC;
+    while(count > 0) {
+        usleep(SLEEP_MIN_USEC);
+        count -= SLEEP_MIN_USEC;
         if (!isRunning(name))
             break;
     }
 
-    if (!count) {
+    if (count <= 0) {
         SLOGW("Timed out waiting for service '%s' to stop", name);
         errno = ETIMEDOUT;
         return -1;
     }
-    SLOGD("Sucessfully stopped '%s'", name);
+    SLOGD("Successfully stopped '%s'", name);
     return 0;
 }
 
 bool ServiceManager::isRunning(const char *name) {
     char propVal[PROPERTY_VALUE_MAX];
-    char propName[255];
-
-    snprintf(propName, sizeof(propVal), "init.svc.%s", name);
+    char propName[PROPERTY_KEY_MAX];
+    int  ret;
 
+    ret = snprintf(propName, sizeof(propName), "init.svc.%s", name);
+    if (ret > (int)sizeof(propName)-1) {
+        SLOGD("Service name '%s' is too long", name);
+        return false;
+    }
 
     if (property_get(propName, propVal, NULL)) {
         if (!strcmp(propVal, "running"))