vconn-unix: Unlink Unix sockets for vconns at close and free memory.

The make_unix_socket() function that Unix vconns use to create their
bindings calls fatal_signal_add_file_to_unlink() to make sure that the
binding socket gets unlinked from the file system if the process is killed
by a fatal signal.  However, this doesn't happen until the process is
actually killed, even if the vconn that owns the socket is actually closed.

This wasn't a problem when the vconn-unix code was written, because all
of the unix vconns were created at process start time and never destroyed
during the normal process runtime.  However, these days the vswitch can
create and destroy unix vconns at runtime depending on the contents of its
configuration file, so it's better to clean up the file system and free
the memory required to keep track of these sockets.

This commit makes unix vconns and pvconns delete their files and free
the memory used to track them when the (p)vconns are closed.

This is only a very minor leak most of the time.

Bug #1817.

diff --git a/lib/vconn-stream.c b/lib/vconn-stream.c
index e2991ddfe12bc23795331ad112fcf7575d002fad..0551c9ebf207245889208a99617b1b85cedb9c1c 100644 (file)
--- a/lib/vconn-stream.c
+++ b/lib/vconn-stream.c
@@ -23,6 +23,7 @@
 #include <string.h>
 #include <sys/types.h>
 #include <unistd.h>
+#include "fatal-signal.h"
 #include "leak-checker.h"
 #include "ofpbuf.h"
 #include "openflow/openflow.h"
@@ -44,6 +45,7 @@ struct stream_vconn
     struct ofpbuf *rxbuf;
     struct ofpbuf *txbuf;
     struct poll_waiter *tx_waiter;
+    char *unlink_path;
 };
 
 static struct vconn_class stream_vconn_class;
@@ -51,10 +53,20 @@ static struct vconn_class stream_vconn_class;
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(10, 25);
 
 static void stream_clear_txbuf(struct stream_vconn *);
+static void maybe_unlink_and_free(char *path);
 
+/* Creates a new vconn named 'name' that will send and receive data on 'fd' and
+ * stores a pointer to the vconn in '*vconnp'.  Initial connection status
+ * 'connect_status' is interpreted as described for vconn_init().
+ *
+ * When '*vconnp' is closed, then 'unlink_path' (if nonnull) will be passed to
+ * fatal_signal_unlink_file_now() and then freed with free().
+ *
+ * Returns 0 if successful, otherwise a positive errno value.  (The current
+ * implementation never fails.) */
 int
 new_stream_vconn(const char *name, int fd, int connect_status,
-                 struct vconn **vconnp)
+                 char *unlink_path, struct vconn **vconnp)
 {
     struct stream_vconn *s;
 
@@ -64,6 +76,7 @@ new_stream_vconn(const char *name, int fd, int connect_status,
     s->txbuf = NULL;
     s->tx_waiter = NULL;
     s->rxbuf = NULL;
+    s->unlink_path = unlink_path;
     *vconnp = &s->vconn;
     return 0;
 }
@@ -83,6 +96,7 @@ stream_close(struct vconn *vconn)
     stream_clear_txbuf(s);
     ofpbuf_delete(s->rxbuf);
     close(s->fd);
+    maybe_unlink_and_free(s->unlink_path);
     free(s);
 }
 
@@ -252,6 +266,7 @@ struct pstream_pvconn
     int fd;
     int (*accept_cb)(int fd, const struct sockaddr *, size_t sa_len,
                      struct vconn **);
+    char *unlink_path;
 };
 
 static struct pvconn_class pstream_pvconn_class;
@@ -263,16 +278,31 @@ pstream_pvconn_cast(struct pvconn *pvconn)
     return CONTAINER_OF(pvconn, struct pstream_pvconn, pvconn);
 }
 
+/* Creates a new pvconn named 'name' that will accept new socket connections on
+ * 'fd' and stores a pointer to the vconn in '*pvconnp'.
+ *
+ * When a connection has been accepted, 'accept_cb' will be called with the new
+ * socket fd 'fd' and the remote address of the connection 'sa' and 'sa_len'.
+ * accept_cb must return 0 if the connection is successful, in which case it
+ * must initialize '*vconnp' to the new vconn, or a positive errno value on
+ * error.  In either case accept_cb takes ownership of the 'fd' passed in.
+ *
+ * When '*pvconnp' is closed, then 'unlink_path' (if nonnull) will be passed to
+ * fatal_signal_unlink_file_now() and freed with free().
+ *
+ * Returns 0 if successful, otherwise a positive errno value.  (The current
+ * implementation never fails.) */
 int
 new_pstream_pvconn(const char *name, int fd,
-                  int (*accept_cb)(int fd, const struct sockaddr *,
-                                   size_t sa_len, struct vconn **),
-                  struct pvconn **pvconnp)
+                  int (*accept_cb)(int fd, const struct sockaddr *sa,
+                                   size_t sa_len, struct vconn **vconnp),
+                  char *unlink_path, struct pvconn **pvconnp)
 {
     struct pstream_pvconn *ps = xmalloc(sizeof *ps);
     pvconn_init(&ps->pvconn, &pstream_pvconn_class, name);
     ps->fd = fd;
     ps->accept_cb = accept_cb;
+    ps->unlink_path = unlink_path;
     *pvconnp = &ps->pvconn;
     return 0;
 }
@@ -282,6 +312,7 @@ pstream_close(struct pvconn *pvconn)
 {
     struct pstream_pvconn *ps = pstream_pvconn_cast(pvconn);
     close(ps->fd);
+    maybe_unlink_and_free(ps->unlink_path);
     free(ps);
 }
 
@@ -327,3 +358,13 @@ static struct pvconn_class pstream_pvconn_class = {
     pstream_accept,
     pstream_wait
 };
+\f
+/* Helper functions. */
+static void
+maybe_unlink_and_free(char *path)
+{
+    if (path) {
+        fatal_signal_unlink_file_now(path);
+        free(path);
+    }
+}

diff --git a/lib/vconn-stream.h b/lib/vconn-stream.h
index fd3d8bd46dca3f70a0ceaf85d94907b96fb802de..91904fff5136f5ee5b1c841058bc45bf2e21a3dc 100644 (file)
--- a/lib/vconn-stream.h
+++ b/lib/vconn-stream.h
@@ -26,10 +26,11 @@ struct pvconn;
 struct sockaddr;
 
 int new_stream_vconn(const char *name, int fd, int connect_status,
-                     struct vconn **vconnp);
+                     char *unlink_path, struct vconn **vconnp);
 int new_pstream_pvconn(const char *name, int fd,
                       int (*accept_cb)(int fd, const struct sockaddr *,
                                        size_t sa_len, struct vconn **),
+                      char *unlink_path,
                       struct pvconn **pvconnp);
 
 #endif /* vconn-stream.h */

diff --git a/lib/vconn-tcp.c b/lib/vconn-tcp.c
index 44f49f10d5b6279fd60818995abecc632a56e38b..b4e52343576bb05988911985943cb6503a015964 100644 (file)
--- a/lib/vconn-tcp.c
+++ b/lib/vconn-tcp.c
@@ -58,7 +58,7 @@ new_tcp_vconn(const char *name, int fd, int connect_status,
         return errno;
     }
 
-    retval = new_stream_vconn(name, fd, connect_status, vconnp);
+    retval = new_stream_vconn(name, fd, connect_status, NULL, vconnp);
     if (!retval) {
         struct vconn *vconn = *vconnp;
         vconn_set_remote_ip(vconn, remote->sin_addr.s_addr);
@@ -108,7 +108,7 @@ ptcp_open(const char *name UNUSED, char *suffix, struct pvconn **pvconnp)
     if (fd < 0) {
         return -fd;
     } else {
-        return new_pstream_pvconn("ptcp", fd, ptcp_accept, pvconnp);
+        return new_pstream_pvconn("ptcp", fd, ptcp_accept, NULL, pvconnp);
     }
 }
 

diff --git a/lib/vconn-unix.c b/lib/vconn-unix.c
index e2b6f7a3704bb79a1bb00d5586a9890c30ad4a84..f24c84640b2a0bfbabc99cf58accc3d415ded6a0 100644 (file)
--- a/lib/vconn-unix.c
+++ b/lib/vconn-unix.c
@@ -47,20 +47,21 @@ static int
 unix_open(const char *name, char *suffix, struct vconn **vconnp)
 {
     const char *connect_path = suffix;
-    char bind_path[128];
+    char *bind_path;
     int fd;
 
-    sprintf(bind_path, "/tmp/vconn-unix.%ld.%d",
-            (long int) getpid(), n_unix_sockets++);
+    bind_path = xasprintf("/tmp/vconn-unix.%ld.%d",
+                          (long int) getpid(), n_unix_sockets++);
     fd = make_unix_socket(SOCK_STREAM, true, false, bind_path, connect_path);
     if (fd < 0) {
         VLOG_ERR("%s: connection to %s failed: %s",
                  bind_path, connect_path, strerror(-fd));
+        free(bind_path);
         return -fd;
     }
 
     return new_stream_vconn(name, fd, check_connection_completion(fd),
-                            vconnp);
+                            bind_path, vconnp);
 }
 
 struct vconn_class unix_vconn_class = {
@@ -102,7 +103,8 @@ punix_open(const char *name UNUSED, char *suffix, struct pvconn **pvconnp)
         return error;
     }
 
-    return new_pstream_pvconn("punix", fd, punix_accept, pvconnp);
+    return new_pstream_pvconn("punix", fd, punix_accept,
+                              xstrdup(suffix), pvconnp);
 }
 
 static int
@@ -118,7 +120,7 @@ punix_accept(int fd, const struct sockaddr *sa, size_t sa_len,
     } else {
         strcpy(name, "unix");
     }
-    return new_stream_vconn(name, fd, 0, vconnp);
+    return new_stream_vconn(name, fd, 0, NULL, vconnp);
 }
 
 struct pvconn_class punix_pvconn_class = {