From e2d0fa6feb3797246c8bfda3db45a2f5b62e1b5b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Apr 2019 19:36:40 +0200 Subject: [PATCH 1/6] lgtm: complain about accept() [people should use accept4() instead, due to O_CLOEXEC] --- .lgtm/cpp-queries/PotentiallyDangerousFunction.ql | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.lgtm/cpp-queries/PotentiallyDangerousFunction.ql b/.lgtm/cpp-queries/PotentiallyDangerousFunction.ql index 96712cf1c6..865330430d 100644 --- a/.lgtm/cpp-queries/PotentiallyDangerousFunction.ql +++ b/.lgtm/cpp-queries/PotentiallyDangerousFunction.ql @@ -41,6 +41,9 @@ predicate potentiallyDangerousFunction(Function f, string message) { ) or ( f.getQualifiedName() = "strerror" and message = "Call to strerror() is not thread-safe. Use strerror_r() or printf()'s %m format string instead." + ) or ( + f.getQualifiedName() = "accept" and + message = "Call to accept() is not O_CLOEXEC-safe. Use accept4() instead." ) } From 7ebd758cf65ad2d44546768fabca23740460b648 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Apr 2019 19:37:36 +0200 Subject: [PATCH 2/6] tree-wide: voidify some socket calls --- src/activate/activate.c | 4 ++-- src/socket-proxy/socket-proxyd.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/activate/activate.c b/src/activate/activate.c index 0b96344038..1f1a9802ed 100644 --- a/src/activate/activate.c +++ b/src/activate/activate.c @@ -274,8 +274,8 @@ static int do_accept(const char* name, char **argv, char **envp, int fd) { if (fd_accepted < 0) return log_error_errno(errno, "Failed to accept connection on fd:%d: %m", fd); - getsockname_pretty(fd_accepted, &local); - getpeername_pretty(fd_accepted, true, &peer); + (void) getsockname_pretty(fd_accepted, &local); + (void) getpeername_pretty(fd_accepted, true, &peer); log_info("Connection from %s to %s", strna(peer), strna(local)); return fork_and_exec_process(name, argv, envp, fd_accepted); diff --git a/src/socket-proxy/socket-proxyd.c b/src/socket-proxy/socket-proxyd.c index fc7e4958be..ee0c6c0be6 100644 --- a/src/socket-proxy/socket-proxyd.c +++ b/src/socket-proxy/socket-proxyd.c @@ -470,7 +470,7 @@ static int accept_cb(sd_event_source *s, int fd, uint32_t revents, void *userdat if (errno != -EAGAIN) log_warning_errno(errno, "Failed to accept() socket: %m"); } else { - getpeername_pretty(nfd, true, &peer); + (void) getpeername_pretty(nfd, true, &peer); log_debug("New connection from %s", strna(peer)); r = add_connection_socket(context, nfd); From 916a9ec7c6ad0e58a44457f9ea832f16350f5b74 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Apr 2019 19:38:37 +0200 Subject: [PATCH 3/6] errno-util: add three more error codes to ERRNO_IS_DISCONNECT() This is based on the recommendations in accept4() and the listed error codes there. --- src/basic/errno-util.h | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/basic/errno-util.h b/src/basic/errno-util.h index 615b6a806a..f2677c7c19 100644 --- a/src/basic/errno-util.h +++ b/src/basic/errno-util.h @@ -32,11 +32,23 @@ static inline int negative_errno(void) { * * Hint #2: The kernel sends e.g., EHOSTUNREACH or ENONET to userspace in some ICMP error cases. See the * icmp_err_convert[] in net/ipv4/icmp.c in the kernel sources */ -#define ERRNO_IS_DISCONNECT(r) \ - IN_SET(abs(r), \ - ENOTCONN, ECONNRESET, ECONNREFUSED, ECONNABORTED, EPIPE, \ - ENETUNREACH, EHOSTUNREACH, ENOPROTOOPT, EHOSTDOWN, \ - ENONET, ESHUTDOWN) +static inline bool ERRNO_IS_DISCONNECT(int r) { + return IN_SET(abs(r), + ECONNABORTED, + ECONNREFUSED, + ECONNRESET, + EHOSTDOWN, + EHOSTUNREACH, + ENETDOWN, + ENETRESET, + ENETUNREACH, + ENONET, + ENOPROTOOPT, + ENOTCONN, + EPIPE, + EPROTO, + ESHUTDOWN); +} /* Resource exhaustion, could be our fault or general system trouble */ #define ERRNO_IS_RESOURCE(r) \ From 7f000106012f2e641b5fd0604f6668e61a0f44c7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Apr 2019 19:39:12 +0200 Subject: [PATCH 4/6] errno-util: rework ERRNO_IS_RESOURCE() from macro into static inline function No technical reason, except that later on we want to add a new ERRNO_IS() which uses the parameter twice and where we want to avoid double evaluation, and where we'd like to keep things in the same style. --- src/basic/errno-util.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/basic/errno-util.h b/src/basic/errno-util.h index f2677c7c19..ebb204128e 100644 --- a/src/basic/errno-util.h +++ b/src/basic/errno-util.h @@ -51,5 +51,9 @@ static inline bool ERRNO_IS_DISCONNECT(int r) { } /* Resource exhaustion, could be our fault or general system trouble */ -#define ERRNO_IS_RESOURCE(r) \ - IN_SET(abs(r), ENOMEM, EMFILE, ENFILE) +static inline bool ERRNO_IS_RESOURCE(int r) { + return IN_SET(abs(r), + EMFILE, + ENFILE, + ENOMEM); +} From fb0302ddbc47f61349286abfe7fbaa12c7069e25 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Apr 2019 19:40:40 +0200 Subject: [PATCH 5/6] errno-util: add new ERRNO_IS_ACCEPT_AGAIN() test This is modelled after the existing ERRNO_IS_RESOURCES() and in particular ERRNO_IS_DISCONNECT(). It returns true for all transient network errors that should be handled like EAGAIN whenever we call accept() or accept4(). This is per documentation in the accept(2) man page that explicitly says to do so in the its "RETURN VALUE" section. The error list we cover is a bit more comprehensive, and based on existing code of ours. For example EINTR is included too (since we need that to cover cases where we call accept()/accept4() on a blocking socket), and of course ERRNO_IS_DISCONNECT() is a bit more comprehensive than the list in the man page too. --- src/basic/errno-util.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/basic/errno-util.h b/src/basic/errno-util.h index ebb204128e..d7a5ea771f 100644 --- a/src/basic/errno-util.h +++ b/src/basic/errno-util.h @@ -50,6 +50,16 @@ static inline bool ERRNO_IS_DISCONNECT(int r) { ESHUTDOWN); } +/* Transient errors we might get on accept() that we should ignore. As per error handling comment in + * the accept(2) man page. */ +static inline bool ERRNO_IS_ACCEPT_AGAIN(int r) { + return ERRNO_IS_DISCONNECT(r) || + IN_SET(abs(r), + EAGAIN, + EINTR, + EOPNOTSUPP); +} + /* Resource exhaustion, could be our fault or general system trouble */ static inline bool ERRNO_IS_RESOURCE(int r) { return IN_SET(abs(r), From 4ff9bc2ea6e5daf08b5c9de17c0dcf16c3b7379e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Apr 2019 19:50:53 +0200 Subject: [PATCH 6/6] tree-wide: port users over to use new ERRNO_IS_ACCEPT_AGAIN() call --- src/activate/activate.c | 6 +++- src/basic/socket-util.c | 13 +++---- src/core/dbus.c | 3 ++ src/core/socket.c | 26 +++++++------- src/journal-remote/journal-remote.c | 56 ++++++++++++++++------------- src/journal/journald-stream.c | 3 +- src/resolve/resolved-dns-stub.c | 3 +- src/resolve/resolved-llmnr.c | 3 +- src/socket-proxy/socket-proxyd.c | 2 +- src/udev/udev-ctrl.c | 8 +++-- 10 files changed, 72 insertions(+), 51 deletions(-) diff --git a/src/activate/activate.c b/src/activate/activate.c index 1f1a9802ed..473366fb20 100644 --- a/src/activate/activate.c +++ b/src/activate/activate.c @@ -271,8 +271,12 @@ static int do_accept(const char* name, char **argv, char **envp, int fd) { _cleanup_close_ int fd_accepted = -1; fd_accepted = accept4(fd, NULL, NULL, 0); - if (fd_accepted < 0) + if (fd_accepted < 0) { + if (ERRNO_IS_ACCEPT_AGAIN(errno)) + return 0; + return log_error_errno(errno, "Failed to accept connection on fd:%d: %m", fd); + } (void) getsockname_pretty(fd_accepted, &local); (void) getpeername_pretty(fd_accepted, true, &peer); diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index 3d929f5418..904bafb76f 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -15,6 +15,7 @@ #include #include "alloc-util.h" +#include "errno-util.h" #include "escape.h" #include "fd-util.h" #include "fileio.h" @@ -1237,22 +1238,22 @@ int flush_accept(int fd) { continue; return -errno; - - } else if (r == 0) + } + if (r == 0) return 0; cfd = accept4(fd, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC); if (cfd < 0) { - if (errno == EINTR) - continue; - if (errno == EAGAIN) return 0; + if (ERRNO_IS_ACCEPT_AGAIN(errno)) + continue; + return -errno; } - close(cfd); + safe_close(cfd); } } diff --git a/src/core/dbus.c b/src/core/dbus.c index 74b1c51c1d..8ae5d173f0 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -622,6 +622,9 @@ static int bus_on_connection(sd_event_source *s, int fd, uint32_t revents, void nfd = accept4(fd, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC); if (nfd < 0) { + if (ERRNO_IS_ACCEPT_AGAIN(errno)) + return 0; + log_warning_errno(errno, "Failed to accept private connection, ignoring: %m"); return 0; } diff --git a/src/core/socket.c b/src/core/socket.c index 95976ed44d..836e513d49 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -2856,17 +2856,10 @@ static int socket_accept_do(Socket *s, int fd) { assert(s); assert(fd >= 0); - for (;;) { - cfd = accept4(fd, NULL, NULL, SOCK_NONBLOCK); - if (cfd < 0) { - if (errno == EINTR) - continue; - - return -errno; - } - - break; - } + cfd = accept4(fd, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC); + if (cfd < 0) + /* Convert transient network errors into clean and well-defined EAGAIN */ + return ERRNO_IS_ACCEPT_AGAIN(errno) ? -EAGAIN : -errno; return cfd; } @@ -2904,6 +2897,8 @@ static int socket_accept_in_cgroup(Socket *s, SocketPort *p, int fd) { pair[0] = safe_close(pair[0]); cfd = socket_accept_do(s, fd); + if (cfd == -EAGAIN) /* spurious accept() */ + _exit(EXIT_SUCCESS); if (cfd < 0) { log_unit_error_errno(UNIT(s), cfd, "Failed to accept connection socket: %m"); _exit(EXIT_FAILURE); @@ -2928,6 +2923,10 @@ static int socket_accept_in_cgroup(Socket *s, SocketPort *p, int fd) { return r; } + /* If we received no fd, we got EIO here. If this happens with a process exit code of EXIT_SUCCESS + * this is a spurious accept(), let's convert that back to EAGAIN here. */ + if (cfd == -EIO) + return -EAGAIN; if (cfd < 0) return log_unit_error_errno(UNIT(s), cfd, "Failed to receive connection socket: %m"); @@ -2935,6 +2934,8 @@ static int socket_accept_in_cgroup(Socket *s, SocketPort *p, int fd) { shortcut: cfd = socket_accept_do(s, fd); + if (cfd == -EAGAIN) /* spurious accept(), skip it silently */ + return -EAGAIN; if (cfd < 0) return log_unit_error_errno(UNIT(s), cfd, "Failed to accept connection socket: %m"); @@ -2954,7 +2955,6 @@ static int socket_dispatch_io(sd_event_source *source, int fd, uint32_t revents, log_unit_debug(UNIT(p->socket), "Incoming traffic"); if (revents != EPOLLIN) { - if (revents & EPOLLHUP) log_unit_error(UNIT(p->socket), "Got POLLHUP on a listening socket. The service probably invoked shutdown() on it, and should better not do that."); else @@ -2967,6 +2967,8 @@ static int socket_dispatch_io(sd_event_source *source, int fd, uint32_t revents, socket_address_can_accept(&p->address)) { cfd = socket_accept_in_cgroup(p->socket, p, fd); + if (cfd == -EAGAIN) /* Spurious accept() */ + return 0; if (cfd < 0) goto fail; diff --git a/src/journal-remote/journal-remote.c b/src/journal-remote/journal-remote.c index 1da32c5f85..04c66a29ce 100644 --- a/src/journal-remote/journal-remote.c +++ b/src/journal-remote/journal-remote.c @@ -13,6 +13,7 @@ #include "alloc-util.h" #include "def.h" +#include "errno-util.h" #include "escape.h" #include "fd-util.h" #include "journal-file.h" @@ -466,14 +467,23 @@ static int dispatch_blocking_source_event(sd_event_source *event, return journal_remote_handle_raw_source(event, source->importer.fd, EPOLLIN, journal_remote_server_global); } -static int accept_connection(const char* type, int fd, - SocketAddress *addr, char **hostname) { - int fd2, r; +static int accept_connection( + const char* type, + int fd, + SocketAddress *addr, + char **hostname) { + + _cleanup_close_ int fd2 = -1; + int r; log_debug("Accepting new %s connection on fd:%d", type, fd); fd2 = accept4(fd, &addr->sockaddr.sa, &addr->size, SOCK_NONBLOCK|SOCK_CLOEXEC); - if (fd2 < 0) + if (fd2 < 0) { + if (ERRNO_IS_ACCEPT_AGAIN(errno)) + return -EAGAIN; + return log_error_errno(errno, "accept() on fd:%d failed: %m", fd); + } switch(socket_address_family(addr)) { case AF_INET: @@ -482,18 +492,12 @@ static int accept_connection(const char* type, int fd, char *b; r = socket_address_print(addr, &a); - if (r < 0) { - log_error_errno(r, "socket_address_print(): %m"); - close(fd2); - return r; - } + if (r < 0) + return log_error_errno(r, "socket_address_print(): %m"); r = socknameinfo_pretty(&addr->sockaddr, addr->size, &b); - if (r < 0) { - log_error_errno(r, "Resolving hostname failed: %m"); - close(fd2); - return r; - } + if (r < 0) + return log_error_errno(r, "Resolving hostname failed: %m"); log_debug("Accepted %s %s connection from %s", type, @@ -501,22 +505,22 @@ static int accept_connection(const char* type, int fd, a); *hostname = b; + return TAKE_FD(fd2); + } - return fd2; - }; default: - log_error("Rejected %s connection with unsupported family %d", - type, socket_address_family(addr)); - close(fd2); - - return -EINVAL; + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Rejected %s connection with unsupported family %d", + type, socket_address_family(addr)); } } -static int dispatch_raw_connection_event(sd_event_source *event, - int fd, - uint32_t revents, - void *userdata) { +static int dispatch_raw_connection_event( + sd_event_source *event, + int fd, + uint32_t revents, + void *userdata) { + RemoteServer *s = userdata; int fd2; SocketAddress addr = { @@ -526,6 +530,8 @@ static int dispatch_raw_connection_event(sd_event_source *event, char *hostname = NULL; fd2 = accept_connection("raw", fd, &addr, &hostname); + if (fd2 == -EAGAIN) + return 0; if (fd2 < 0) return fd2; diff --git a/src/journal/journald-stream.c b/src/journal/journald-stream.c index aa2db68634..24d4ac30b2 100644 --- a/src/journal/journald-stream.c +++ b/src/journal/journald-stream.c @@ -13,6 +13,7 @@ #include "alloc-util.h" #include "dirent-util.h" #include "env-file.h" +#include "errno-util.h" #include "escape.h" #include "fd-util.h" #include "fileio.h" @@ -605,7 +606,7 @@ static int stdout_stream_new(sd_event_source *es, int listen_fd, uint32_t revent fd = accept4(s->stdout_fd, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC); if (fd < 0) { - if (errno == EAGAIN) + if (ERRNO_IS_ACCEPT_AGAIN(errno)) return 0; return log_error_errno(errno, "Failed to accept stdout connection: %m"); diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index 65c809490f..ce994a7ee0 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ +#include "errno-util.h" #include "fd-util.h" #include "missing_network.h" #include "resolved-dns-stub.h" @@ -462,7 +463,7 @@ static int on_dns_stub_stream(sd_event_source *s, int fd, uint32_t revents, void cfd = accept4(fd, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC); if (cfd < 0) { - if (IN_SET(errno, EAGAIN, EINTR)) + if (ERRNO_IS_ACCEPT_AGAIN(errno)) return 0; return -errno; diff --git a/src/resolve/resolved-llmnr.c b/src/resolve/resolved-llmnr.c index b7c37f1132..61e50343e5 100644 --- a/src/resolve/resolved-llmnr.c +++ b/src/resolve/resolved-llmnr.c @@ -3,6 +3,7 @@ #include #include +#include "errno-util.h" #include "fd-util.h" #include "resolved-llmnr.h" #include "resolved-manager.h" @@ -289,7 +290,7 @@ static int on_llmnr_stream(sd_event_source *s, int fd, uint32_t revents, void *u cfd = accept4(fd, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC); if (cfd < 0) { - if (IN_SET(errno, EAGAIN, EINTR)) + if (ERRNO_IS_ACCEPT_AGAIN(errno)) return 0; return -errno; diff --git a/src/socket-proxy/socket-proxyd.c b/src/socket-proxy/socket-proxyd.c index ee0c6c0be6..abbbc9f2d6 100644 --- a/src/socket-proxy/socket-proxyd.c +++ b/src/socket-proxy/socket-proxyd.c @@ -467,7 +467,7 @@ static int accept_cb(sd_event_source *s, int fd, uint32_t revents, void *userdat nfd = accept4(fd, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC); if (nfd < 0) { - if (errno != -EAGAIN) + if (!ERRNO_IS_ACCEPT_AGAIN(errno)) log_warning_errno(errno, "Failed to accept() socket: %m"); } else { (void) getpeername_pretty(nfd, true, &peer); diff --git a/src/udev/udev-ctrl.c b/src/udev/udev-ctrl.c index 59ce5c1f80..c3dc213aa8 100644 --- a/src/udev/udev-ctrl.c +++ b/src/udev/udev-ctrl.c @@ -20,6 +20,7 @@ #include "sd-event.h" #include "alloc-util.h" +#include "errno-util.h" #include "fd-util.h" #include "format-util.h" #include "io-util.h" @@ -261,9 +262,10 @@ static int udev_ctrl_event_handler(sd_event_source *s, int fd, uint32_t revents, sock = accept4(fd, NULL, NULL, SOCK_CLOEXEC|SOCK_NONBLOCK); if (sock < 0) { - if (errno != EINTR) - log_error_errno(errno, "Failed to accept ctrl connection: %m"); - return 0; + if (ERRNO_IS_ACCEPT_AGAIN(errno)) + return 0; + + return log_error_errno(errno, "Failed to accept ctrl connection: %m"); } /* check peer credential of connection */