Fix potential race introduced in Icd7f5f03

Digit wrote:

"You probably don't want to close the socket here without updating
c->socket as well. Otherwise, another thread holding a handle to the
client after the c->decRef() could end up sending a message to a
different socket, if the file descriptor index is reused by another
client in the meantime."

Change-Id: Icdefb5ffc0c7607325d7db761e1f04e5d868bfb7

3 files changed, 23 insertions, 17 deletions

diff --git a/include/sysutils/SocketClient.h b/include/sysutils/SocketClient.h
index ef6dd9be6..d6bb7d5df 100644
--- a/include/sysutils/SocketClient.h
+++ b/include/sysutils/SocketClient.h
@@ -44,7 +44,7 @@ public:
     // SocketListener creates a SocketClient (at refcount 1) and calls
     // decRef() when it's done with the client.
     void incRef();
-    void decRef();
+    bool decRef(); // returns true at 0 (but note: SocketClient already deleted)
 };
 
 typedef android::List<SocketClient *> SocketClientCollection;
diff --git a/libsysutils/src/SocketClient.cpp b/libsysutils/src/SocketClient.cpp
index 6d4dff45d..90ca52e74 100644
--- a/libsysutils/src/SocketClient.cpp
+++ b/libsysutils/src/SocketClient.cpp
@@ -104,20 +104,23 @@ int SocketClient::sendData(const void* data, int len) {
 }
 
 void SocketClient::incRef() {
-  pthread_mutex_lock(&mRefCountMutex);
-  mRefCount++;
-  pthread_mutex_unlock(&mRefCountMutex);
+    pthread_mutex_lock(&mRefCountMutex);
+    mRefCount++;
+    pthread_mutex_unlock(&mRefCountMutex);
 }
 
-void SocketClient::decRef() {
-  bool deleteSelf = false;
-  pthread_mutex_lock(&mRefCountMutex);
-  mRefCount--;
-  if (mRefCount == 0) {
-      deleteSelf = true;
-  }
-  pthread_mutex_unlock(&mRefCountMutex);
-  if (deleteSelf) {
-      delete this;
-  }
+bool SocketClient::decRef() {
+    bool deleteSelf = false;
+    pthread_mutex_lock(&mRefCountMutex);
+    mRefCount--;
+    if (mRefCount == 0) {
+        deleteSelf = true;
+    } else if (mRefCount < 0) {
+        SLOGE("SocketClient refcount went negative!");
+    }
+    pthread_mutex_unlock(&mRefCountMutex);
+    if (deleteSelf) {
+        delete this;
+    }
+    return deleteSelf;
 }
diff --git a/libsysutils/src/SocketListener.cpp b/libsysutils/src/SocketListener.cpp
index dde9b55de..69ed79edf 100644
--- a/libsysutils/src/SocketListener.cpp
+++ b/libsysutils/src/SocketListener.cpp
@@ -225,8 +225,11 @@ void SocketListener::runListener() {
                 }
                 pthread_mutex_unlock(&mClientsLock);
                 /* Destroy the client */
-                close(c->getSocket());
-                c->decRef();
+                int socket = c->getSocket();
+                if (c->decRef()) {
+                    // Note: 'c' is deleted memory at this point.
+                    close(socket);
+                }
             }
         }
     }