5 files changed, 110 insertions, 32 deletions
diff --git a/libsysutils/src/FrameworkClient.cpp b/libsysutils/src/FrameworkClient.cpp
index 562dd67..2f37055 100644
--- a/libsysutils/src/FrameworkClient.cpp
+++ b/libsysutils/src/FrameworkClient.cpp
@@ -14,13 +14,15 @@ FrameworkClient::FrameworkClient(int socket) {
 }
 
 int FrameworkClient::sendMsg(const char *msg) {
+    int ret;
     if (mSocket < 0) {
         errno = EHOSTUNREACH;
         return -1;
     }
 
     pthread_mutex_lock(&mWriteMutex);
-    if (write(mSocket, msg, strlen(msg) +1) < 0) {
+    ret = TEMP_FAILURE_RETRY(write(mSocket, msg, strlen(msg) +1));
+    if (ret < 0) {
         SLOGW("Unable to send msg '%s' (%s)", msg, strerror(errno));
     }
     pthread_mutex_unlock(&mWriteMutex);
@@ -28,13 +30,13 @@ int FrameworkClient::sendMsg(const char *msg) {
 }
 
 int FrameworkClient::sendMsg(const char *msg, const char *data) {
-    char *buffer = (char *) alloca(strlen(msg) + strlen(data) + 1);
+    size_t bufflen = strlen(msg) + strlen(data) + 1;
+    char *buffer = (char *) alloca(bufflen);
     if (!buffer) {
         errno = -ENOMEM;
         return -1;
     }
-    strcpy(buffer, msg);
-    strcat(buffer, data);
+    snprintf(buffer, bufflen, "%s%s", msg, data);
     return sendMsg(buffer);
 }
 
diff --git a/libsysutils/src/FrameworkListener.cpp b/libsysutils/src/FrameworkListener.cpp
index d7fd025..3416ceb 100644
--- a/libsysutils/src/FrameworkListener.cpp
+++ b/libsysutils/src/FrameworkListener.cpp
@@ -15,6 +15,7 @@
  */
 #include <errno.h>
 #include <string.h>
+#include <stdlib.h>
 
 #define LOG_TAG "FrameworkListener"
 
diff --git a/libsysutils/src/NetlinkEvent.cpp b/libsysutils/src/NetlinkEvent.cpp
index 86c1f42..c8d3b1f 100644
--- a/libsysutils/src/NetlinkEvent.cpp
+++ b/libsysutils/src/NetlinkEvent.cpp
@@ -56,45 +56,76 @@ void NetlinkEvent::dump() {
     }
 }
 
+/* If the string between 'str' and 'end' begins with 'prefixlen' characters
+ * from the 'prefix' array, then return 'str + prefixlen', otherwise return
+ * NULL.
+ */
+static const char*
+has_prefix(const char* str, const char* end, const char* prefix, size_t prefixlen)
+{
+    if ((end-str) >= (ptrdiff_t)prefixlen && !memcmp(str, prefix, prefixlen))
+        return str + prefixlen;
+    else
+        return NULL;
+}
+
+/* Same as strlen(x) for constant string literals ONLY */
+#define CONST_STRLEN(x)  (sizeof(x)-1)
+
+/* Convenience macro to call has_prefix with a constant string literal  */
+#define HAS_CONST_PREFIX(str,end,prefix)  has_prefix((str),(end),prefix,CONST_STRLEN(prefix))
+
+
 bool NetlinkEvent::decode(char *buffer, int size) {
-    char *s = buffer;
-    char *end;
+    const char *s = buffer;
+    const char *end;
     int param_idx = 0;
     int i;
     int first = 1;
 
+    if (size == 0)
+        return false;
+
+    /* Ensure the buffer is zero-terminated, the code below depends on this */
+    buffer[size-1] = '\0';
+
     end = s + size;
     while (s < end) {
         if (first) {
-            char *p;
-            for (p = s; *p != '@'; p++);
-            p++;
-            mPath = strdup(p);
+            const char *p;
+            /* buffer is 0-terminated, no need to check p < end */
+            for (p = s; *p != '@'; p++) {
+                if (!*p) { /* no '@', should not happen */
+                    return false;
+                }
+            }
+            mPath = strdup(p+1);
             first = 0;
         } else {
-            if (!strncmp(s, "ACTION=", strlen("ACTION="))) {
-                char *a = s + strlen("ACTION=");
+            const char* a;
+            if ((a = HAS_CONST_PREFIX(s, end, "ACTION=")) != NULL) {
                 if (!strcmp(a, "add"))
                     mAction = NlActionAdd;
                 else if (!strcmp(a, "remove"))
                     mAction = NlActionRemove;
                 else if (!strcmp(a, "change"))
                     mAction = NlActionChange;
-            } else if (!strncmp(s, "SEQNUM=", strlen("SEQNUM=")))
-                mSeq = atoi(s + strlen("SEQNUM="));
-            else if (!strncmp(s, "SUBSYSTEM=", strlen("SUBSYSTEM=")))
-                mSubsystem = strdup(s + strlen("SUBSYSTEM="));
-            else
+            } else if ((a = HAS_CONST_PREFIX(s, end, "SEQNUM=")) != NULL) {
+                mSeq = atoi(a);
+            } else if ((a = HAS_CONST_PREFIX(s, end, "SUBSYSTEM=")) != NULL) {
+                mSubsystem = strdup(a);
+            } else if (param_idx < NL_PARAMS_MAX) {
                 mParams[param_idx++] = strdup(s);
+            }
         }
-        s+= strlen(s) + 1;
+        s += strlen(s) + 1;
     }
     return true;
 }
 
 const char *NetlinkEvent::findParam(const char *paramName) {
     size_t len = strlen(paramName);
-    for (int i = 0; mParams[i] && i < NL_PARAMS_MAX; ++i) {
+    for (int i = 0; i < NL_PARAMS_MAX && mParams[i] != NULL; ++i) {
         const char *ptr = mParams[i] + len;
         if (!strncmp(mParams[i], paramName, len) && *ptr == '=')
             return ++ptr;
diff --git a/libsysutils/src/NetlinkListener.cpp b/libsysutils/src/NetlinkListener.cpp
index fb088e1..ddf6537 100644
--- a/libsysutils/src/NetlinkListener.cpp
+++ b/libsysutils/src/NetlinkListener.cpp
@@ -39,7 +39,8 @@ bool NetlinkListener::onDataAvailable(SocketClient *cli)
     struct iovec iov = {mBuffer, sizeof(mBuffer)};
     struct msghdr hdr = {&snl, sizeof(snl), &iov, 1, cred_msg, sizeof(cred_msg), 0};
 
-    if ((count = recvmsg(socket, &hdr, 0)) < 0) {
+    count = TEMP_FAILURE_RETRY(recvmsg(socket, &hdr, 0));
+    if (count < 0) {
         SLOGE("recvmsg failed (%s)", strerror(errno));
         return false;
     }
diff --git a/libsysutils/src/ServiceManager.cpp b/libsysutils/src/ServiceManager.cpp
index 60b161a..41ac1dd 100644
--- a/libsysutils/src/ServiceManager.cpp
+++ b/libsysutils/src/ServiceManager.cpp
@@ -1,4 +1,5 @@
 #include <errno.h>
+#include <string.h>
 
 #include <sysutils/ServiceManager.h>
 
@@ -9,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;
@@ -18,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;
@@ -34,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;
@@ -42,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"))