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