d0811f
From 056799e2e147d678e156c5a1fce15b04762f1313 Mon Sep 17 00:00:00 2001
d0811f
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
d0811f
Date: Tue, 1 Sep 2020 23:50:01 +0200
d0811f
Subject: [PATCH 1/3] core/socket: we may get ENOTCONN from
d0811f
 socket_instantiate_service()
d0811f
d0811f
This means that the connection was aborted before we even got to figure out
d0811f
what the service name will be. Let's treat this as a non-event and close the
d0811f
connection fd without any further messages.
d0811f
d0811f
Code last changed in 934ef6a5.
d0811f
Reported-by: Thiago Macieira <thiago.macieira@intel.com>
d0811f
d0811f
With the patch:
d0811f
systemd[1]: foobar.socket: Incoming traffic
d0811f
systemd[1]: foobar.socket: Got ENOTCONN on incoming socket, assuming aborted connection attempt, ignoring.
d0811f
...
d0811f
d0811f
Also, when we get ENOMEM, don't give the hint about missing unit.
d0811f
---
d0811f
 src/core/socket.c | 35 ++++++++++++++++++++++++-----------
d0811f
 1 file changed, 24 insertions(+), 11 deletions(-)
d0811f
d0811f
diff --git a/src/core/socket.c b/src/core/socket.c
d0811f
index ebf5ce3b16..f880040331 100644
d0811f
--- a/src/core/socket.c
d0811f
+++ b/src/core/socket.c
d0811f
@@ -18,6 +18,7 @@
d0811f
 #include "dbus-socket.h"
d0811f
 #include "dbus-unit.h"
d0811f
 #include "def.h"
d0811f
+#include "errno-list.h"
d0811f
 #include "exit-status.h"
d0811f
 #include "fd-util.h"
d0811f
 #include "format-util.h"
d0811f
@@ -1418,11 +1419,12 @@ int socket_load_service_unit(Socket *s, int cfd, Unit **ret) {
d0811f
 
d0811f
         if (cfd >= 0) {
d0811f
                 r = instance_from_socket(cfd, s->n_accepted, &instance);
d0811f
-                if (r == -ENOTCONN)
d0811f
-                        /* ENOTCONN is legitimate if TCP RST was received.
d0811f
-                         * This connection is over, but the socket unit lives on. */
d0811f
+                if (ERRNO_IS_DISCONNECT(r))
d0811f
+                        /* ENOTCONN is legitimate if TCP RST was received. Other socket families might return
d0811f
+                         * different errors. This connection is over, but the socket unit lives on. */
d0811f
                         return log_unit_debug_errno(UNIT(s), r,
d0811f
-                                                    "Got ENOTCONN on incoming socket, assuming aborted connection attempt, ignoring.");
d0811f
+                                                    "Got %s on incoming socket, assuming aborted connection attempt, ignoring.",
d0811f
+                                                    errno_to_name(r));
d0811f
                 if (r < 0)
d0811f
                         return r;
d0811f
         }
d0811f
@@ -2359,8 +2361,8 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
 
d0811f
                 if (!pending) {
d0811f
                         if (!UNIT_ISSET(s->service)) {
d0811f
-                                log_unit_error(UNIT(s), "Service to activate vanished, refusing activation.");
d0811f
-                                r = -ENOENT;
d0811f
+                                r = log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOENT),
d0811f
+                                                         "Service to activate vanished, refusing activation.");
d0811f
                                 goto fail;
d0811f
                         }
d0811f
 
d0811f
@@ -2382,8 +2384,10 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
 
d0811f
                 if (s->max_connections_per_source > 0) {
d0811f
                         r = socket_acquire_peer(s, cfd, &p);
d0811f
-                        if (r < 0)
d0811f
-                                goto refuse;
d0811f
+                        if (ERRNO_IS_DISCONNECT(r))
d0811f
+                                goto notconn;
d0811f
+                        if (r < 0) /* We didn't have enough resources to acquire peer information, let's fail. */
d0811f
+                                goto fail;
d0811f
                         if (r > 0 && p->n_ref > s->max_connections_per_source) {
d0811f
                                 _cleanup_free_ char *t = NULL;
d0811f
 
d0811f
@@ -2397,6 +2401,8 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
                 }
d0811f
 
d0811f
                 r = socket_instantiate_service(s, cfd);
d0811f
+                if (ERRNO_IS_DISCONNECT(r))
d0811f
+                        goto notconn;
d0811f
                 if (r < 0)
d0811f
                         goto fail;
d0811f
 
d0811f
@@ -2406,6 +2412,8 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
                 s->n_accepted++;
d0811f
 
d0811f
                 r = service_set_socket_fd(service, cfd, s, s->selinux_context_from_net);
d0811f
+                if (ERRNO_IS_DISCONNECT(r))
d0811f
+                        goto notconn;
d0811f
                 if (r < 0)
d0811f
                         goto fail;
d0811f
 
d0811f
@@ -2430,13 +2438,18 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
 
d0811f
 refuse:
d0811f
         s->n_refused++;
d0811f
+notconn:
d0811f
         safe_close(cfd);
d0811f
         return;
d0811f
 
d0811f
 fail:
d0811f
-        log_unit_warning(UNIT(s), "Failed to queue service startup job (Maybe the service file is missing or not a %s unit?): %s",
d0811f
-                         cfd >= 0 ? "template" : "non-template",
d0811f
-                         bus_error_message(&error, r));
d0811f
+        if (ERRNO_IS_RESOURCE(r))
d0811f
+                log_unit_warning(UNIT(s), "Failed to queue service startup job: %s",
d0811f
+                                 bus_error_message(&error, r));
d0811f
+        else
d0811f
+                log_unit_warning(UNIT(s), "Failed to queue service startup job (Maybe the service file is missing or not a %s unit?): %s",
d0811f
+                                 cfd >= 0 ? "template" : "non-template",
d0811f
+                                 bus_error_message(&error, r));
d0811f
 
d0811f
         socket_enter_stop_pre(s, SOCKET_FAILURE_RESOURCES);
d0811f
         safe_close(cfd);
d0811f
-- 
d0811f
2.26.2
d0811f
d0811f
d0811f
From 86f9af3eb8bea0bea86bb027cb341e6b13beecb5 Mon Sep 17 00:00:00 2001
d0811f
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
d0811f
Date: Wed, 2 Sep 2020 18:04:10 +0200
d0811f
Subject: [PATCH 2/3] core/socket: fold socket_instantiate_service() into
d0811f
 socket_enter_running()
d0811f
d0811f
socket_instantiate_service() was doing unit_ref_set(), and the caller was
d0811f
immediately doing unit_ref_unset(). After we get rid of this, it doesn't seem
d0811f
worth it to have two functions.
d0811f
---
d0811f
 src/core/socket.c | 39 ++++++++++-----------------------------
d0811f
 1 file changed, 10 insertions(+), 29 deletions(-)
d0811f
d0811f
diff --git a/src/core/socket.c b/src/core/socket.c
d0811f
index f880040331..5e128d9fef 100644
d0811f
--- a/src/core/socket.c
d0811f
+++ b/src/core/socket.c
d0811f
@@ -206,27 +206,6 @@ static int socket_arm_timer(Socket *s, usec_t usec) {
d0811f
         return 0;
d0811f
 }
d0811f
 
d0811f
-static int socket_instantiate_service(Socket *s, int cfd) {
d0811f
-        Unit *service;
d0811f
-        int r;
d0811f
-
d0811f
-        assert(s);
d0811f
-        assert(cfd >= 0);
d0811f
-
d0811f
-        /* This fills in s->service if it isn't filled in yet. For Accept=yes sockets we create the next
d0811f
-         * connection service here. For Accept=no this is mostly a NOP since the service is figured out at
d0811f
-         * load time anyway. */
d0811f
-
d0811f
-        r = socket_load_service_unit(s, cfd, &service);
d0811f
-        if (r < 0)
d0811f
-                return r;
d0811f
-
d0811f
-        unit_ref_set(&s->service, UNIT(s), service);
d0811f
-
d0811f
-        return unit_add_two_dependencies(UNIT(s), UNIT_BEFORE, UNIT_TRIGGERS, service,
d0811f
-                                         false, UNIT_DEPENDENCY_IMPLICIT);
d0811f
-}
d0811f
-
d0811f
 static bool have_non_accept_socket(Socket *s) {
d0811f
         SocketPort *p;
d0811f
 
d0811f
@@ -2374,7 +2353,7 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
                 socket_set_state(s, SOCKET_RUNNING);
d0811f
         } else {
d0811f
                 _cleanup_(socket_peer_unrefp) SocketPeer *p = NULL;
d0811f
-                Service *service;
d0811f
+                Unit *service;
d0811f
 
d0811f
                 if (s->n_connections >= s->max_connections) {
d0811f
                         log_unit_warning(UNIT(s), "Too many incoming connections (%u), dropping connection.",
d0811f
@@ -2400,18 +2379,20 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
                         }
d0811f
                 }
d0811f
 
d0811f
-                r = socket_instantiate_service(s, cfd);
d0811f
+                r = socket_load_service_unit(s, cfd, &service);
d0811f
                 if (ERRNO_IS_DISCONNECT(r))
d0811f
                         goto notconn;
d0811f
                 if (r < 0)
d0811f
                         goto fail;
d0811f
 
d0811f
-                service = SERVICE(UNIT_DEREF(s->service));
d0811f
-                unit_ref_unset(&s->service);
d0811f
+                r = unit_add_two_dependencies(UNIT(s), UNIT_BEFORE, UNIT_TRIGGERS, service,
d0811f
+                                              false, UNIT_DEPENDENCY_IMPLICIT);
d0811f
+                if (r < 0)
d0811f
+                        goto fail;
d0811f
 
d0811f
                 s->n_accepted++;
d0811f
 
d0811f
-                r = service_set_socket_fd(service, cfd, s, s->selinux_context_from_net);
d0811f
+                r = service_set_socket_fd(SERVICE(service), cfd, s, s->selinux_context_from_net);
d0811f
                 if (ERRNO_IS_DISCONNECT(r))
d0811f
                         goto notconn;
d0811f
                 if (r < 0)
d0811f
@@ -2420,13 +2401,13 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
                 TAKE_FD(cfd); /* We passed ownership of the fd to the service now. Forget it here. */
d0811f
                 s->n_connections++;
d0811f
 
d0811f
-                service->peer = TAKE_PTR(p); /* Pass ownership of the peer reference */
d0811f
+                SERVICE(service)->peer = TAKE_PTR(p); /* Pass ownership of the peer reference */
d0811f
 
d0811f
-                r = manager_add_job(UNIT(s)->manager, JOB_START, UNIT(service), JOB_REPLACE, NULL, &error, NULL);
d0811f
+                r = manager_add_job(UNIT(s)->manager, JOB_START, service, JOB_REPLACE, NULL, &error, NULL);
d0811f
                 if (r < 0) {
d0811f
                         /* We failed to activate the new service, but it still exists. Let's make sure the
d0811f
                          * service closes and forgets the connection fd again, immediately. */
d0811f
-                        service_close_socket_fd(service);
d0811f
+                        service_close_socket_fd(SERVICE(service));
d0811f
                         goto fail;
d0811f
                 }
d0811f
 
d0811f
-- 
d0811f
2.26.2
d0811f
d0811f
d0811f
From b7e9403a4c6220478980555ef40905d030b307f5 Mon Sep 17 00:00:00 2001
d0811f
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
d0811f
Date: Wed, 2 Sep 2020 18:17:14 +0200
d0811f
Subject: [PATCH 3/3] core/socket: use _cleanup_ to close the connection fd
d0811f
d0811f
Removing the gotos would lead to a lot of duplicated code, so I left them
d0811f
as they were.
d0811f
---
d0811f
 src/core/socket.c | 22 ++++++++++------------
d0811f
 1 file changed, 10 insertions(+), 12 deletions(-)
d0811f
d0811f
diff --git a/src/core/socket.c b/src/core/socket.c
d0811f
index 5e128d9fef..a77a297cf5 100644
d0811f
--- a/src/core/socket.c
d0811f
+++ b/src/core/socket.c
d0811f
@@ -2296,13 +2296,14 @@ static void flush_ports(Socket *s) {
d0811f
         }
d0811f
 }
d0811f
 
d0811f
-static void socket_enter_running(Socket *s, int cfd) {
d0811f
+static void socket_enter_running(Socket *s, int cfd_in) {
d0811f
+        /* Note that this call takes possession of the connection fd passed. It either has to assign it
d0811f
+         * somewhere or close it. */
d0811f
+        _cleanup_close_ int cfd = cfd_in;
d0811f
+
d0811f
         _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
d0811f
         int r;
d0811f
 
d0811f
-        /* Note that this call takes possession of the connection fd passed. It either has to assign it somewhere or
d0811f
-         * close it. */
d0811f
-
d0811f
         assert(s);
d0811f
 
d0811f
         /* We don't take connections anymore if we are supposed to shut down anyway */
d0811f
@@ -2312,9 +2313,8 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
 
d0811f
                 if (cfd >= 0)
d0811f
                         goto refuse;
d0811f
-                else
d0811f
-                        flush_ports(s);
d0811f
 
d0811f
+                flush_ports(s);
d0811f
                 return;
d0811f
         }
d0811f
 
d0811f
@@ -2364,7 +2364,7 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
                 if (s->max_connections_per_source > 0) {
d0811f
                         r = socket_acquire_peer(s, cfd, &p);
d0811f
                         if (ERRNO_IS_DISCONNECT(r))
d0811f
-                                goto notconn;
d0811f
+                                return;
d0811f
                         if (r < 0) /* We didn't have enough resources to acquire peer information, let's fail. */
d0811f
                                 goto fail;
d0811f
                         if (r > 0 && p->n_ref > s->max_connections_per_source) {
d0811f
@@ -2381,7 +2381,7 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
 
d0811f
                 r = socket_load_service_unit(s, cfd, &service);
d0811f
                 if (ERRNO_IS_DISCONNECT(r))
d0811f
-                        goto notconn;
d0811f
+                        return;
d0811f
                 if (r < 0)
d0811f
                         goto fail;
d0811f
 
d0811f
@@ -2394,7 +2394,7 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
 
d0811f
                 r = service_set_socket_fd(SERVICE(service), cfd, s, s->selinux_context_from_net);
d0811f
                 if (ERRNO_IS_DISCONNECT(r))
d0811f
-                        goto notconn;
d0811f
+                        return;
d0811f
                 if (r < 0)
d0811f
                         goto fail;
d0811f
 
d0811f
@@ -2415,12 +2415,11 @@ static void socket_enter_running(Socket *s, int cfd) {
d0811f
                 unit_add_to_dbus_queue(UNIT(s));
d0811f
         }
d0811f
 
d0811f
+        TAKE_FD(cfd);
d0811f
         return;
d0811f
 
d0811f
 refuse:
d0811f
         s->n_refused++;
d0811f
-notconn:
d0811f
-        safe_close(cfd);
d0811f
         return;
d0811f
 
d0811f
 fail:
d0811f
@@ -2433,7 +2432,6 @@ fail:
d0811f
                                  bus_error_message(&error, r));
d0811f
 
d0811f
         socket_enter_stop_pre(s, SOCKET_FAILURE_RESOURCES);
d0811f
-        safe_close(cfd);
d0811f
 }
d0811f
 
d0811f
 static void socket_run_next(Socket *s) {
d0811f
-- 
d0811f
2.26.2
d0811f