From 13aa8ad44570bceef73115cea749b11f30888530 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 17 Mar 2011 15:41:20 -0700 Subject: [PATCH 1/2] SocketClient: add optional reference counting Needed to fix a race in netd. Bug: 3438459 Change-Id: Icd7f5f035510235f733a25c0621479d3e644b152 --- include/sysutils/SocketClient.h | 11 +++++++++++ libsysutils/src/SocketClient.cpp | 21 +++++++++++++++++++++ libsysutils/src/SocketListener.cpp | 4 ++-- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/include/sysutils/SocketClient.h b/include/sysutils/SocketClient.h index 2fcc3311c..ef6dd9be6 100644 --- a/include/sysutils/SocketClient.h +++ b/include/sysutils/SocketClient.h @@ -19,6 +19,10 @@ class SocketClient { /* Peer group ID */ gid_t mGid; + /* Reference count (starts at 1) */ + pthread_mutex_t mRefCountMutex; + int mRefCount; + public: SocketClient(int sock); virtual ~SocketClient() {} @@ -34,6 +38,13 @@ public: // Sending binary data: int sendData(const void *data, int len); + + // Optional reference counting. Reference count starts at 1. If + // it's decremented to 0, it deletes itself. + // SocketListener creates a SocketClient (at refcount 1) and calls + // decRef() when it's done with the client. + void incRef(); + void decRef(); }; typedef android::List SocketClientCollection; diff --git a/libsysutils/src/SocketClient.cpp b/libsysutils/src/SocketClient.cpp index a6aed265d..6d4dff45d 100644 --- a/libsysutils/src/SocketClient.cpp +++ b/libsysutils/src/SocketClient.cpp @@ -15,8 +15,10 @@ SocketClient::SocketClient(int socket) , mPid(-1) , mUid(-1) , mGid(-1) + , mRefCount(1) { pthread_mutex_init(&mWriteMutex, NULL); + pthread_mutex_init(&mRefCountMutex, NULL); struct ucred creds; socklen_t szCreds = sizeof(creds); @@ -100,3 +102,22 @@ int SocketClient::sendData(const void* data, int len) { pthread_mutex_unlock(&mWriteMutex); return 0; } + +void SocketClient::incRef() { + 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; + } +} diff --git a/libsysutils/src/SocketListener.cpp b/libsysutils/src/SocketListener.cpp index 611d5fe5f..dde9b55de 100644 --- a/libsysutils/src/SocketListener.cpp +++ b/libsysutils/src/SocketListener.cpp @@ -55,7 +55,7 @@ SocketListener::~SocketListener() { } SocketClientCollection::iterator it; for (it = mClients->begin(); it != mClients->end();) { - delete (*it); + (*it)->decRef(); it = mClients->erase(it); } delete mClients; @@ -226,7 +226,7 @@ void SocketListener::runListener() { pthread_mutex_unlock(&mClientsLock); /* Destroy the client */ close(c->getSocket()); - delete c; + c->decRef(); } } } From 3549b0dc2829184f9911d27a6ab0cf39b19764f1 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 17 Mar 2011 17:14:46 -0700 Subject: [PATCH 2/2] 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 --- include/sysutils/SocketClient.h | 2 +- libsysutils/src/SocketClient.cpp | 31 ++++++++++++++++-------------- libsysutils/src/SocketListener.cpp | 7 +++++-- 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 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); + } } } }