socket: always pass socket, fd and SocketPeer ownership to service together

Per-connection socket instances we currently maintain three fields
related to the socket: a reference to the Socket unit, the connection fd,
and a reference to the SocketPeer object that counts socket peers.

Let's synchronize their lifetime, i.e. always set them all three
together or unset them together, so that their reference counters stay
synchronous.

THis will in particuar ensure that we'll drop the SocketPeer reference
whenever we leave an active state of the service unit, i.e. at the same
time we close the fd for it.

Fixes: #20685

diff --git a/src/core/service.c b/src/core/service.c
index 17c19a2c4ab54196f23553a2bc3300bf68d79f70..49579f799858fdedfcc4d0ab20cc5baa30fe5ab0 100644 (file)
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -190,6 +190,8 @@ void service_close_socket_fd(Service *s) {
                 socket_connection_unref(SOCKET(UNIT_DEREF(s->accept_socket)));
                 unit_ref_unset(&s->accept_socket);
         }
+
+        s->socket_peer = socket_peer_unref(s->socket_peer);
 }
 
 static void service_stop_watchdog(Service *s) {
@@ -388,7 +390,6 @@ static void service_done(Unit *u) {
         s->usb_function_strings = mfree(s->usb_function_strings);
 
         service_close_socket_fd(s);
-        s->peer = socket_peer_unref(s->peer);
 
         unit_ref_unset(&s->accept_socket);
 
@@ -1237,8 +1238,8 @@ static int service_coldplug(Unit *u) {
 
                         /* Make a best-effort attempt at bumping the connection count */
                         if (socket_acquire_peer(socket, s->socket_fd, &peer) > 0) {
-                                socket_peer_unref(s->peer);
-                                s->peer = peer;
+                                socket_peer_unref(s->socket_peer);
+                                s->socket_peer = peer;
                         }
                 }
         }
@@ -4285,8 +4286,14 @@ static void service_bus_name_owner_change(Unit *u, const char *new_owner) {
         }
 }
 
-int service_set_socket_fd(Service *s, int fd, Socket *sock, bool selinux_context_net) {
-        _cleanup_free_ char *peer = NULL;
+int service_set_socket_fd(
+                Service *s,
+                int fd,
+                Socket *sock,
+                SocketPeer *peer,
+                bool selinux_context_net) {
+
+        _cleanup_free_ char *peer_text = NULL;
         int r;
 
         assert(s);
@@ -4301,22 +4308,23 @@ int service_set_socket_fd(Service *s, int fd, Socket *sock, bool selinux_context
         if (s->socket_fd >= 0)
                 return -EBUSY;
 
+        assert(!s->socket_peer);
+
         if (s->state != SERVICE_DEAD)
                 return -EAGAIN;
 
-        if (getpeername_pretty(fd, true, &peer) >= 0) {
+        if (getpeername_pretty(fd, true, &peer_text) >= 0) {
 
                 if (UNIT(s)->description) {
                         _cleanup_free_ char *a = NULL;
 
-                        a = strjoin(UNIT(s)->description, " (", peer, ")");
+                        a = strjoin(UNIT(s)->description, " (", peer_text, ")");
                         if (!a)
                                 return -ENOMEM;
 
                         r = unit_set_description(UNIT(s), a);
                 }  else
-                        r = unit_set_description(UNIT(s), peer);
-
+                        r = unit_set_description(UNIT(s), peer_text);
                 if (r < 0)
                         return r;
         }
@@ -4326,6 +4334,7 @@ int service_set_socket_fd(Service *s, int fd, Socket *sock, bool selinux_context
                 return r;
 
         s->socket_fd = fd;
+        s->socket_peer = socket_peer_ref(peer);
         s->socket_fd_selinux_context_net = selinux_context_net;
 
         unit_ref_set(&s->accept_socket, UNIT(s), UNIT(sock));

diff --git a/src/core/service.h b/src/core/service.h
index 70ce70fba51f5f0acb72af32fdfe3e30ec8672e1..778551d8441bb293d9be4b44d2018c5c46220284 100644 (file)
--- a/src/core/service.h
+++ b/src/core/service.h
@@ -157,8 +157,11 @@ struct Service {
         DynamicCreds dynamic_creds;
 
         pid_t main_pid, control_pid;
+
+        /* if we are a socket activated service instance, store information of the connection/peer/socket */
         int socket_fd;
-        SocketPeer *peer;
+        SocketPeer *socket_peer;
+        UnitRef accept_socket;
         bool socket_fd_selinux_context_net;
 
         bool permissions_start_only;
@@ -186,8 +189,6 @@ struct Service {
         char *status_text;
         int status_errno;
 
-        UnitRef accept_socket;
-
         sd_event_source *timer_event_source;
         PathSpec *pid_file_pathspec;
 
@@ -226,7 +227,7 @@ static inline usec_t service_get_watchdog_usec(Service *s) {
 
 extern const UnitVTable service_vtable;
 
-int service_set_socket_fd(Service *s, int fd, struct Socket *socket, bool selinux_context_net);
+int service_set_socket_fd(Service *s, int fd, struct Socket *socket, struct SocketPeer *peer, bool selinux_context_net);
 void service_close_socket_fd(Service *s);
 
 const char* service_restart_to_string(ServiceRestart i) _const_;

diff --git a/src/core/socket.c b/src/core/socket.c
index e6d168188a17404792ba3202158c0736396292a6..d9db1edd3c7b307a19d59bb92370397e9b41da51 100644 (file)
--- a/src/core/socket.c
+++ b/src/core/socket.c
@@ -2409,7 +2409,7 @@ static void socket_enter_running(Socket *s, int cfd_in) {
 
                 s->n_accepted++;
 
-                r = service_set_socket_fd(SERVICE(service), cfd, s, s->selinux_context_from_net);
+                r = service_set_socket_fd(SERVICE(service), cfd, s, p, s->selinux_context_from_net);
                 if (ERRNO_IS_DISCONNECT(r))
                         return;
                 if (r < 0)
@@ -2418,8 +2418,6 @@ static void socket_enter_running(Socket *s, int cfd_in) {
                 TAKE_FD(cfd); /* We passed ownership of the fd to the service now. Forget it here. */
                 s->n_connections++;
 
-                SERVICE(service)->peer = TAKE_PTR(p); /* Pass ownership of the peer reference */
-
                 r = manager_add_job(UNIT(s)->manager, JOB_START, service, JOB_REPLACE, NULL, &error, NULL);
                 if (r < 0) {
                         /* We failed to activate the new service, but it still exists. Let's make sure the