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
diff --git a/include/sysutils/SocketClient.h b/include/sysutils/SocketClient.h
index ef6dd9b..d6bb7d5 100644
--- a/include/sysutils/SocketClient.h
+++ b/include/sysutils/SocketClient.h
@@ -44,7 +44,7 @@
// 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 6d4dff4..90ca52e 100644
--- a/libsysutils/src/SocketClient.cpp
+++ b/libsysutils/src/SocketClient.cpp
@@ -104,20 +104,23 @@
}
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 dde9b55..69ed79e 100644
--- a/libsysutils/src/SocketListener.cpp
+++ b/libsysutils/src/SocketListener.cpp
@@ -225,8 +225,11 @@
}
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);
+ }
}
}
}