bus-proxy: don't close local bus fds twice

Clear up how we pass fd owner ship to proxy and bus objects. Document
that ownership is passed of the fds in question even in case of failing
constructors, and that callers should forget about fds pass into the
proxy object.

The alternative would be to duplicate the fds, but given that fds are a
relatively scarce and heavy resource let's better avoid that.

Fixes #1591.

diff --git a/src/bus-proxyd/bus-proxyd.c b/src/bus-proxyd/bus-proxyd.c
index 2bc265d9b491c8a549a9a8608d6612c2f3191748..6a4da0f2e2f81d2edcf79c7b6e0e506e31fcc590 100644 (file)
--- a/src/bus-proxyd/bus-proxyd.c
+++ b/src/bus-proxyd/bus-proxyd.c
@@ -85,11 +85,11 @@ static void *run_client(void *userdata) {
         int r;
 
         r = proxy_new(&p, c->fd, c->fd, arg_address);
+        c->fd = -1;
+
         if (r < 0)
                 goto exit;
 
-        c->fd = -1;
-
         /* set comm to "p$PIDu$UID" and suffix with '*' if truncated */
         r = snprintf(comm, sizeof(comm), "p" PID_FMT "u" UID_FMT, p->local_creds.pid, p->local_creds.uid);
         if (r >= (ssize_t)sizeof(comm))

diff --git a/src/bus-proxyd/proxy.c b/src/bus-proxyd/proxy.c
index 88800f5e7f28e83237533d4024565dc9e1aa5e9d..bc8516f5c66338c074f449f9d9809a68d05d3b05 100644 (file)
--- a/src/bus-proxyd/proxy.c
+++ b/src/bus-proxyd/proxy.c
@@ -100,18 +100,24 @@ static int proxy_create_destination(Proxy *p, const char *destination, const cha
         return 0;
 }
 
-static int proxy_create_local(Proxy *p, int in_fd, int out_fd, bool negotiate_fds) {
-        _cleanup_bus_flush_close_unref_ sd_bus *b = NULL;
+static int proxy_create_local(Proxy *p, bool negotiate_fds) {
         sd_id128_t server_id;
+        sd_bus *b;
         int r;
 
         r = sd_bus_new(&b);
         if (r < 0)
                 return log_error_errno(r, "Failed to allocate bus: %m");
 
-        r = sd_bus_set_fd(b, in_fd, out_fd);
-        if (r < 0)
+        r = sd_bus_set_fd(b, p->local_in, p->local_out);
+        if (r < 0) {
+                sd_bus_unref(b);
                 return log_error_errno(r, "Failed to set fds: %m");
+        }
+
+        /* The fds are now owned by the bus, and we indicate that by
+         * storing the bus object in the proxy object. */
+        p->local_bus = b;
 
         r = sd_bus_get_bus_id(p->destination_bus, &server_id);
         if (r < 0)
@@ -139,8 +145,6 @@ static int proxy_create_local(Proxy *p, int in_fd, int out_fd, bool negotiate_fd
         if (r < 0)
                 return log_error_errno(r, "Failed to start bus client: %m");
 
-        p->local_bus = b;
-        b = NULL;
         return 0;
 }
 
@@ -224,9 +228,17 @@ int proxy_new(Proxy **out, int in_fd, int out_fd, const char *destination) {
         bool is_unix;
         int r;
 
+        /* This takes possession/destroys the file descriptors passed
+         * in even on failure. The caller should hence forget about
+         * the fds in all cases after calling this function and not
+         * close them. */
+
         p = new0(Proxy, 1);
-        if (!p)
+        if (!p) {
+                safe_close(in_fd);
+                safe_close(out_fd);
                 return log_oom();
+        }
 
         p->local_in = in_fd;
         p->local_out = out_fd;
@@ -247,7 +259,7 @@ int proxy_new(Proxy **out, int in_fd, int out_fd, const char *destination) {
         if (r < 0)
                 return r;
 
-        r = proxy_create_local(p, in_fd, out_fd, is_unix);
+        r = proxy_create_local(p, is_unix);
         if (r < 0)
                 return r;
 
@@ -257,6 +269,7 @@ int proxy_new(Proxy **out, int in_fd, int out_fd, const char *destination) {
 
         *out = p;
         p = NULL;
+
         return 0;
 }
 
@@ -273,7 +286,14 @@ Proxy *proxy_free(Proxy *p) {
                 free(activation);
         }
 
-        sd_bus_flush_close_unref(p->local_bus);
+        if (p->local_bus)
+                sd_bus_flush_close_unref(p->local_bus);
+        else {
+                safe_close(p->local_in);
+                if (p->local_out != p->local_in)
+                        safe_close(p->local_out);
+        }
+
         sd_bus_flush_close_unref(p->destination_bus);
         set_free_free(p->owned_names);
         free(p);