libsysutils: Fix race condition in SocketListener thread.

+ Handle EINTR in accept(), write() and select()
+ Fix a memory leak when deleting the mClients list
+ Fix typo in SocketListener.h

Change-Id: Ie68bb3e2dbefe0dfdaa22a5cd06a42dbc4c0f8aa

2 files changed, 52 insertions, 27 deletions

diff --git a/include/sysutils/SocketListener.h b/include/sysutils/SocketListener.h
index c7edfeb..6592b01 100644
--- a/include/sysutils/SocketListener.h
+++ b/include/sysutils/SocketListener.h
@@ -30,7 +30,7 @@ class SocketListener {
     pthread_t               mThread;
 
 public:
-    SocketListener(const char *socketNames, bool listen);
+    SocketListener(const char *socketName, bool listen);
     SocketListener(int socketFd, bool listen);
 
     virtual ~SocketListener();
diff --git a/libsysutils/src/SocketListener.cpp b/libsysutils/src/SocketListener.cpp
index 1bc06db..677c57d 100644
--- a/libsysutils/src/SocketListener.cpp
+++ b/libsysutils/src/SocketListener.cpp
@@ -54,7 +54,7 @@ SocketListener::~SocketListener() {
         close(mCtrlPipe[1]);
     }
     SocketClientCollection::iterator it;
-    for (it = mClients->begin(); it != mClients->end(); ++it) {
+    for (it = mClients->begin(); it != mClients->end();) {
         delete (*it);
         it = mClients->erase(it);
     }
@@ -96,8 +96,10 @@ int SocketListener::startListener() {
 
 int SocketListener::stopListener() {
     char c = 0;
+    int  rc;
 
-    if (write(mCtrlPipe[1], &c, 1) != 1) {
+    rc = TEMP_FAILURE_RETRY(write(mCtrlPipe[1], &c, 1));
+    if (rc != 1) {
         SLOGE("Error writing to control pipe (%s)", strerror(errno));
         return -1;
     }
@@ -118,7 +120,7 @@ int SocketListener::stopListener() {
     }
 
     SocketClientCollection::iterator it;
-    for (it = mClients->begin(); it != mClients->end(); ++it) {
+    for (it = mClients->begin(); it != mClients->end();) {
         delete (*it);
         it = mClients->erase(it);
     }
@@ -135,11 +137,13 @@ void *SocketListener::threadStart(void *obj) {
 
 void SocketListener::runListener() {
 
+    SocketClientCollection *pendingList = new SocketClientCollection();
+
     while(1) {
         SocketClientCollection::iterator it;
         fd_set read_fds;
         int rc = 0;
-        int max = 0;
+        int max = -1;
 
         FD_ZERO(&read_fds);
 
@@ -154,13 +158,16 @@ void SocketListener::runListener() {
 
         pthread_mutex_lock(&mClientsLock);
         for (it = mClients->begin(); it != mClients->end(); ++it) {
-            FD_SET((*it)->getSocket(), &read_fds);
-            if ((*it)->getSocket() > max)
-                max = (*it)->getSocket();
+            int fd = (*it)->getSocket();
+            FD_SET(fd, &read_fds);
+            if (fd > max)
+                max = fd;
         }
         pthread_mutex_unlock(&mClientsLock);
 
         if ((rc = select(max + 1, &read_fds, NULL, NULL, NULL)) < 0) {
+            if (errno == EINTR)
+                continue;
             SLOGE("select failed (%s)", strerror(errno));
             sleep(1);
             continue;
@@ -171,10 +178,14 @@ void SocketListener::runListener() {
             break;
         if (mListen && FD_ISSET(mSock, &read_fds)) {
             struct sockaddr addr;
-            socklen_t alen = sizeof(addr);
+            socklen_t alen;
             int c;
 
-            if ((c = accept(mSock, &addr, &alen)) < 0) {
+            do {
+                alen = sizeof(addr);
+                c = accept(mSock, &addr, &alen);
+            } while (c < 0 && errno == EINTR);
+            if (c < 0) {
                 SLOGE("accept failed (%s)", strerror(errno));
                 sleep(1);
                 continue;
@@ -184,27 +195,41 @@ void SocketListener::runListener() {
             pthread_mutex_unlock(&mClientsLock);
         }
 
-        do {
-            pthread_mutex_lock(&mClientsLock);
-            for (it = mClients->begin(); it != mClients->end(); ++it) {
-                int fd = (*it)->getSocket();
-                if (FD_ISSET(fd, &read_fds)) {
-                    pthread_mutex_unlock(&mClientsLock);
-                    if (!onDataAvailable(*it)) {
-                        close(fd);
-                        pthread_mutex_lock(&mClientsLock);
-                        delete *it;
-                        it = mClients->erase(it);
-                        pthread_mutex_unlock(&mClientsLock);
+        /* Add all active clients to the pending list first */
+        pendingList->clear();
+        pthread_mutex_lock(&mClientsLock);
+        for (it = mClients->begin(); it != mClients->end(); ++it) {
+            int fd = (*it)->getSocket();
+            if (FD_ISSET(fd, &read_fds)) {
+                pendingList->push_back(*it);
+            }
+        }
+        pthread_mutex_unlock(&mClientsLock);
+
+        /* Process the pending list, since it is owned by the thread,
+         * there is no need to lock it */
+        while (!pendingList->empty()) {
+            /* Pop the first item from the list */
+            it = pendingList->begin();
+            SocketClient* c = *it;
+            pendingList->erase(it);
+            /* Process it, if false is returned, remove and destroy it */
+            if (!onDataAvailable(c)) {
+                /* Remove the client from our array */
+                pthread_mutex_lock(&mClientsLock);
+                for (it = mClients->begin(); it != mClients->end(); ++it) {
+                    if (*it == c) {
+                        mClients->erase(it);
+                        break;
                     }
-                    FD_CLR(fd, &read_fds);
-                    pthread_mutex_lock(&mClientsLock);
-                    continue;
                 }
+                pthread_mutex_unlock(&mClientsLock);
+                /* Destroy the client */
+                delete c;
             }
-            pthread_mutex_unlock(&mClientsLock);
-        } while (0);
+        }
     }
+    delete pendingList;
 }
 
 void SocketListener::sendBroadcast(int code, const char *msg, bool addErrno) {