Use a virFreeCallback on virNetSocket to ensure safe release

When unregistering an I/O callback from a virNetSocket object,
there is still a chance that an event may come in on the callback.
In this case it is possible that the virNetSocket might have been
freed already. Make use of a virFreeCallback when registering
the I/O callbacks and hold a reference for the entire time the
callback is set.

* src/rpc/virnetsocket.c: Register a free function for the
  file handle watch
* src/rpc/virnetsocket.h, src/rpc/virnetserverservice.c,
  src/rpc/virnetserverclient.c, src/rpc/virnetclient.c: Add
  a free function for the socket I/O watches

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 4a9eabc8d33f0f899c9830999f3feda87ab0d233..27542a57ac3808f2efe86432ea30176a11054e93 100644 (file)
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -110,6 +110,13 @@ static void virNetClientIncomingEvent(virNetSocketPtr sock,
                                       int events,
                                       void *opaque);
 
+static void virNetClientEventFree(void *opaque)
+{
+    virNetClientPtr client = opaque;
+
+    virNetClientFree(client);
+}
+
 static virNetClientPtr virNetClientNew(virNetSocketPtr sock,
                                        const char *hostname)
 {
@@ -140,11 +147,15 @@ static virNetClientPtr virNetClientNew(virNetSocketPtr sock,
         goto no_memory;
 
     /* Set up a callback to listen on the socket data */
+    client->refs++;
     if (virNetSocketAddIOCallback(client->sock,
                                   VIR_EVENT_HANDLE_READABLE,
                                   virNetClientIncomingEvent,
-                                  client) < 0)
+                                  client,
+                                  virNetClientEventFree) < 0) {
+        client->refs--;
         VIR_DEBUG("Failed to add event watch, disabling events");
+    }
 
     VIR_DEBUG("client=%p refs=%d", client, client->refs);
     return client;

diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 341981f46f6280b8084d40106e53e6b2d281e353..317d59cf559ab92bfe302a4459f5e708eb3d40e4 100644 (file)
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -160,6 +160,13 @@ virNetServerClientCalculateHandleMode(virNetServerClientPtr client) {
     return mode;
 }
 
+static void virNetServerClientEventFree(void *opaque)
+{
+    virNetServerClientPtr client = opaque;
+
+    virNetServerClientFree(client);
+}
+
 /*
  * @server: a locked or unlocked server object
  * @client: a locked client object
@@ -168,12 +175,16 @@ static int virNetServerClientRegisterEvent(virNetServerClientPtr client)
 {
     int mode = virNetServerClientCalculateHandleMode(client);
 
+    client->refs++;
     VIR_DEBUG("Registering client event callback %d", mode);
     if (virNetSocketAddIOCallback(client->sock,
                                   mode,
                                   virNetServerClientDispatchEvent,
-                                  client) < 0)
+                                  client,
+                                  virNetServerClientEventFree) < 0) {
+        client->refs--;
         return -1;
+    }
 
     return 0;
 }

diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
index 8c250e246d05fed1ff8551add06f5a1515f2de74..d5648dc81faf7adca9a31a46bb7bf8e66cb8e1cb 100644 (file)
--- a/src/rpc/virnetserverservice.c
+++ b/src/rpc/virnetserverservice.c
@@ -91,6 +91,14 @@ error:
 }
 
 
+static void virNetServerServiceEventFree(void *opaque)
+{
+    virNetServerServicePtr svc = opaque;
+
+    virNetServerServiceFree(svc);
+}
+
+
 virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename,
                                                  const char *service,
                                                  int auth,
@@ -124,11 +132,15 @@ virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename,
 
         /* IO callback is initially disabled, until we're ready
          * to deal with incoming clients */
+        virNetServerServiceRef(svc);
         if (virNetSocketAddIOCallback(svc->socks[i],
                                       0,
                                       virNetServerServiceAccept,
-                                      svc) < 0)
+                                      svc,
+                                      virNetServerServiceEventFree) < 0) {
+            virNetServerServiceFree(svc);
             goto error;
+        }
     }
 
 
@@ -180,11 +192,15 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path,
 
         /* IO callback is initially disabled, until we're ready
          * to deal with incoming clients */
+        virNetServerServiceRef(svc);
         if (virNetSocketAddIOCallback(svc->socks[i],
                                       0,
                                       virNetServerServiceAccept,
-                                      svc) < 0)
+                                      svc,
+                                      virNetServerServiceEventFree) < 0) {
+            virNetServerServiceFree(svc);
             goto error;
+        }
     }
 
 

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 603650052f9c99a24f4ef905be50ecb7d336ca52..4403fc47ddadf0026f7b60ef58ad9b41dcb37988 100644 (file)
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -58,8 +58,12 @@ struct _virNetSocket {
     pid_t pid;
     int errfd;
     bool client;
+
+    /* Event callback fields */
     virNetSocketIOFunc func;
     void *opaque;
+    virFreeCallback ff;
+
     virSocketAddr localAddr;
     virSocketAddr remoteAddr;
     char *localAddrStr;
@@ -1121,10 +1125,31 @@ static void virNetSocketEventHandle(int watch ATTRIBUTE_UNUSED,
 }
 
 
+static void virNetSocketEventFree(void *opaque)
+{
+    virNetSocketPtr sock = opaque;
+    virFreeCallback ff;
+    void *eopaque;
+
+    virMutexLock(&sock->lock);
+    ff = sock->ff;
+    eopaque = sock->opaque;
+    sock->func = NULL;
+    sock->ff = NULL;
+    sock->opaque = NULL;
+    virMutexUnlock(&sock->lock);
+
+    if (ff)
+        ff(eopaque);
+
+    virNetSocketFree(sock);
+}
+
 int virNetSocketAddIOCallback(virNetSocketPtr sock,
                               int events,
                               virNetSocketIOFunc func,
-                              void *opaque)
+                              void *opaque,
+                              virFreeCallback ff)
 {
     int ret = -1;
 
@@ -1134,16 +1159,19 @@ int virNetSocketAddIOCallback(virNetSocketPtr sock,
         goto cleanup;
     }
 
+    sock->refs++;
     if ((sock->watch = virEventAddHandle(sock->fd,
                                          events,
                                          virNetSocketEventHandle,
                                          sock,
-                                         NULL)) < 0) {
+                                         virNetSocketEventFree)) < 0) {
         VIR_DEBUG("Failed to register watch on socket %p", sock);
+        sock->refs--;
         goto cleanup;
     }
     sock->func = func;
     sock->opaque = opaque;
+    sock->ff = ff;
 
     ret = 0;
 

diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h
index 5f882ac5439afca95a70a625bbb94020cc01a495..e13ab8fb150dba6f3951c34e1547d4c63436b022 100644 (file)
--- a/src/rpc/virnetsocket.h
+++ b/src/rpc/virnetsocket.h
@@ -109,7 +109,8 @@ int virNetSocketAccept(virNetSocketPtr sock,
 int virNetSocketAddIOCallback(virNetSocketPtr sock,
                               int events,
                               virNetSocketIOFunc func,
-                              void *opaque);
+                              void *opaque,
+                              virFreeCallback ff);
 
 void virNetSocketUpdateIOCallback(virNetSocketPtr sock,
                                   int events);