From 202e0fe34dc3c8dcb1a0ad12faa7f4d5a7c91b2d Mon Sep 17 00:00:00 2001 From: Alberto Leiva Popper Date: Thu, 8 Feb 2024 15:29:35 -0600 Subject: [PATCH] RTR server bind review - Hint SOCK_STREAM on getaddrinfo(), not on socket(). I guess this means it was attempting some redundant binds on failure? - Print the "Attempting to bind socket to address '%s', port '%s'" message in every addrinfo, not just the first one. (Improves log legibility.) - Improve the algorithm that produces the printable version of the address (used to dereference ai_addr->sa_data like the previous commit), and fall back to whatever the user provided if it fails. - Weird inconsistency: Most errors used to cause create_server_socket() to try the next addrinfo candidate, but getsockname() and listen() induced immediate abortion. I've no idea what that was about; getsockname() isn't even a mandatory step. - Attempt to bind all addrinfo candidates; they must all succeed. (It used to stop on the first success.) I'm conflicted with the last point. The old behavior appears to have been inherited from the Linux manual page example, but doesn't make any sense from where I'm standing. If `/etc/hosts` defines 127.0.0.1 localhost ::1 localhost And server.address is "localhost", shouldn't we attempt to bind to both addresses? --- src/rtr/rtr.c | 137 +++++++++++++++++++++----------------------- src/types/address.c | 12 ++-- src/types/address.h | 2 +- 3 files changed, 74 insertions(+), 77 deletions(-) diff --git a/src/rtr/rtr.c b/src/rtr/rtr.c index 135d4040..ae44e1ac 100644 --- a/src/rtr/rtr.c +++ b/src/rtr/rtr.c @@ -110,7 +110,7 @@ init_addrinfo(char const *hostname, char const *service, memset(&hints, 0 , sizeof(hints)); hints.ai_family = AF_UNSPEC; - /* hints.ai_socktype = SOCK_DGRAM; */ + hints.ai_socktype = SOCK_STREAM; hints.ai_flags |= AI_PASSIVE; if (hostname != NULL) @@ -127,7 +127,25 @@ init_addrinfo(char const *hostname, char const *service, return 0; } +static char * +get_best_printable(struct addrinfo *addr, char const *input_addr) +{ + char str[INET6_ADDRSTRLEN]; + + if (sockaddr2str((struct sockaddr_storage *) addr->ai_addr, str)) + return pstrdup(str); + + if (input_addr != NULL) + return pstrdup(input_addr); + + /* Failure is fine; this is just a nice-to-have. */ + return NULL; +} + /* + * We want to listen to all sockets in one thread, + * so don't block. + * * By the way: man 2 poll says * * > The operation of poll() and ppoll() is not affected by the O_NONBLOCK flag. @@ -164,92 +182,67 @@ set_nonblock(int fd) * from the clients. */ static int -create_server_socket(char const *input_addr, char const *hostname, - char const *service) +create_server_socket(char const *input_addr, char const *hostname, char const *port) { - struct addrinfo *addrs; - struct addrinfo *addr; - unsigned long port; - int reuse; - int fd; + struct addrinfo *ais, *ai; struct rtr_server server; - int error; - - reuse = 1; - - error = init_addrinfo(hostname, service, &addrs); - if (error) - return error; - - if (addrs != NULL) - pr_op_info( - "Attempting to bind socket to address '%s', port '%s'.", - (addrs->ai_canonname != NULL) ? addrs->ai_canonname : "any", - service); - - for (addr = addrs; addr != NULL; addr = addr->ai_next) { - fd = socket(addr->ai_family, SOCK_STREAM, 0); - if (fd < 0) { - pr_op_err("socket() failed: %s", strerror(errno)); - continue; + char const *errmsg; + static const int yes = 1; + int err; + + err = init_addrinfo(hostname, port, &ais); + if (err) + return err; + + for (ai = ais; ai != NULL; ai = ai->ai_next) { + server.fd = -1; + server.addr = get_best_printable(ai, input_addr); + pr_op_info("[%s]:%s: Setting up socket...", server.addr, port); + + server.fd = socket(ai->ai_family, ai->ai_socktype, ai->ai_protocol); + if (server.fd < 0) { + err = errno; + errmsg = "Unable to create socket"; + goto fail; } - /* - * We want to listen to all sockets in one thread, - * so don't block. - */ - if (set_nonblock(fd) != 0) { - close(fd); - continue; + if ((err = set_nonblock(server.fd)) != 0) { + errmsg = "Unable to disable blocking on the socket"; + goto fail; } - /* enable SO_REUSEADDR */ - if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &reuse, - sizeof(int)) < 0) { - pr_op_err("setsockopt(SO_REUSEADDR) failed: %s", - strerror(errno)); - close(fd); - continue; + if (setsockopt(server.fd, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof(yes)) < 0) { + err = errno; + errmsg = "Unable to enable SO_REUSEADDR on the socket"; + goto fail; } - if (bind(fd, addr->ai_addr, addr->ai_addrlen) < 0) { - pr_op_err("bind() failed: %s", strerror(errno)); - close(fd); - continue; - } - - if (getsockname(fd, addr->ai_addr, &addr->ai_addrlen) != 0) { - error = errno; - close(fd); - freeaddrinfo(addrs); - pr_op_err("getsockname() failed: %s", strerror(error)); - return error; + if (bind(server.fd, ai->ai_addr, ai->ai_addrlen) < 0) { + err = errno; + errmsg = "Unable to bind the socket"; + goto fail; } - port = (unsigned char)(addr->ai_addr->sa_data[0]) << 8; - port += (unsigned char)(addr->ai_addr->sa_data[1]); - pr_op_info("Success; bound to address '%s', port '%ld'.", - (addr->ai_canonname != NULL) ? addr->ai_canonname : "any", - port); - freeaddrinfo(addrs); - - if (listen(fd, config_get_server_queue()) != 0) { - error = errno; - close(fd); - pr_op_err("listen() failure: %s", strerror(error)); - return error; + if (listen(server.fd, config_get_server_queue()) < 0) { + err = errno; + errmsg = "Unable to start listening on socket"; + goto fail; } - server.fd = fd; - /* Ignore failure; this is just a nice-to-have. */ - server.addr = (input_addr != NULL) ? pstrdup(input_addr) : NULL; + pr_op_info("[%s]:%s: Success.", server.addr, port); server_arraylist_add(&servers, &server); - - return 0; /* Happy path */ } - freeaddrinfo(addrs); - return pr_op_err("None of the addrinfo candidates could be bound."); + freeaddrinfo(ais); + return 0; + +fail: + pr_op_err("[%s]:%s: %s: %s", server.addr, port, errmsg, strerror(err)); + if (server.fd != -1) + close(server.fd); + free(server.addr); + freeaddrinfo(ais); + return err; } static int diff --git a/src/types/address.c b/src/types/address.c index ee1c56d9..911015fb 100644 --- a/src/types/address.c +++ b/src/types/address.c @@ -530,7 +530,7 @@ addr2str6(struct in6_addr const *addr, char *buffer) /** * buffer must length INET6_ADDRSTRLEN. */ -void +bool sockaddr2str(struct sockaddr_storage *sockaddr, char *buffer) { void *addr = NULL; @@ -538,7 +538,7 @@ sockaddr2str(struct sockaddr_storage *sockaddr, char *buffer) if (sockaddr == NULL) { strcpy(buffer, "(null)"); - return; + return false; } switch (sockaddr->ss_family) { @@ -550,10 +550,14 @@ sockaddr2str(struct sockaddr_storage *sockaddr, char *buffer) break; default: strcpy(buffer, "(protocol unknown)"); - return; + return false; } str = inet_ntop(sockaddr->ss_family, addr, buffer, INET6_ADDRSTRLEN); - if (str == NULL) + if (str == NULL) { strcpy(buffer, "(unprintable address)"); + return false; + } + + return true; } diff --git a/src/types/address.h b/src/types/address.h index d695e0d2..749a6565 100644 --- a/src/types/address.h +++ b/src/types/address.h @@ -53,6 +53,6 @@ bool ipv6_covered(struct in6_addr const *, uint8_t, struct in6_addr const *); char const *addr2str4(struct in_addr const *, char *); char const *addr2str6(struct in6_addr const *, char *); -void sockaddr2str(struct sockaddr_storage *, char *buffer); +bool sockaddr2str(struct sockaddr_storage *, char *buffer); #endif /* SRC_TYPES_ADDRESS_H_ */ -- 2.47.2