On Fri, 17 May 2024 13:30:17 +1000 David Gibson wrote:

When reporting errors, we sometimes want to show a relevant socket address. Doing so by extracting the various relevant fields can be pretty awkward, so introduce a sockaddr_ntop() helper to make it simpler. For now we just have one user in tcp.c, but I have further upcoming patches which can make use of it.

Signed-off-by: David Gibson --- tcp.c | 23 ++++++-------- util.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ util.h | 13 ++++++++ 3 files changed, 116 insertions(+), 14 deletions(-)

Changes since v1: * More careful and documented reasoning about the size of the strings needed for output. * Some more complex logic to calling inet_ntop() into a temporary buffer then copying it

That helper looks neat, and perhaps we'll reuse it somewhere, but... do we really need it? I mean, maybe it's not so elegant, but something like this (lightly "tested" with the main() below) should also do the trick and it's much less code: -- #include #include #include #include #include #include const char *sockaddr_ntop(const void *sa, char *dst, ssize_t size) { sa_family_t family = ((const struct sockaddr *)sa)->sa_family; const struct sockaddr_in6 *a6 = sa; const struct sockaddr_in *a4 = sa; char *p = dst; switch (family) { case AF_INET: if ((size -= sizeof(":9")) < 0) return NULL; if (!inet_ntop(AF_INET, &a4->sin_addr, p, size)) return NULL; break; case AF_INET6: if ((size -= sizeof("[")) < 0) return NULL; if ((size -= snprintf(p++, size, "[")) < (int)sizeof("::]:9")) return NULL; if (!inet_ntop(AF_INET6, &a6->sin6_addr, p, size)) return NULL; } size -= strlen(p); p += strlen(p); if (snprintf(p, size, family == AF_INET6 ? "]:%hu" : ":%hu", family == AF_INET6 ? ntohs(a6->sin6_port) : ntohs(a4->sin_port)) >= size) return NULL; return dst; } int main() { struct sockaddr_in6 test6 = { .sin6_family = AF_INET6 }; struct sockaddr_in test4 = { .sin_family = AF_INET }; struct { struct in6_addr a6; struct in_addr a4; uint16_t p; uint8_t len; } r; char buf[BUFSIZ]; int i; for (i = 0; i < (1 << 18); i++) { getrandom(&r, sizeof(r), 0); test6.sin6_addr = r.a6; test4.sin_addr = r.a4; test4.sin_port = test6.sin6_port = r.p; fprintf(stdout, "%s\n", sockaddr_ntop(&test4, buf, r.len)); fprintf(stdout, "%s\n", sockaddr_ntop(&test6, buf, r.len)); } return 0; } -- if you want to go ahead with the iprintf() helper, however, it's also fine by me.

diff --git a/tcp.c b/tcp.c index 21d0af0..efbbc1c 100644 --- a/tcp.c +++ b/tcp.c @@ -2758,6 +2758,7 @@ static void tcp_tap_conn_from_sock(struct ctx *c, in_port_t dstport, void tcp_listen_handler(struct ctx *c, union epoll_ref ref, const struct timespec *now) { + char sastr[SOCKADDR_STRLEN]; union sockaddr_inany sa; socklen_t sl = sizeof(sa); union flow *flow; @@ -2776,25 +2777,15 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref,

if (IN4_IS_ADDR_UNSPECIFIED(addr) || IN4_IS_ADDR_BROADCAST(addr) || - IN4_IS_ADDR_MULTICAST(addr) || port == 0) { - char str[INET_ADDRSTRLEN]; - - err("Invalid endpoint from TCP accept(): %s:%hu", - inet_ntop(AF_INET, addr, str, sizeof(str)), port); - goto cancel; - } + IN4_IS_ADDR_MULTICAST(addr) || port == 0) + goto bad_endpoint; } else if (sa.sa_family == AF_INET6) { const struct in6_addr *addr = &sa.sa6.sin6_addr; in_port_t port = sa.sa6.sin6_port;

if (IN6_IS_ADDR_UNSPECIFIED(addr) || - IN6_IS_ADDR_MULTICAST(addr) || port == 0) { - char str[INET6_ADDRSTRLEN]; - - err("Invalid endpoint from TCP accept(): %s:%hu", - inet_ntop(AF_INET6, addr, str, sizeof(str)), port); - goto cancel; - } + IN6_IS_ADDR_MULTICAST(addr) || port == 0) + goto bad_endpoint; }

if (tcp_splice_conn_from_sock(c, ref.tcp_listen.pif, @@ -2804,6 +2795,10 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref, tcp_tap_conn_from_sock(c, ref.tcp_listen.port, flow, s, &sa, now); return;

+bad_endpoint: + err("Invalid endpoint from TCP accept(): %s", + sockaddr_ntop(&sa, sastr, sizeof(sastr))); + cancel: flow_alloc_cancel(flow); } diff --git a/util.c b/util.c index 849fa7f..74c5070 100644 --- a/util.c +++ b/util.c @@ -553,3 +553,97 @@ int write_remainder(int fd, const struct iovec *iov, int iovcnt, size_t skip)

return 0; } + +/** iprintf() - Incremental printf helper + * @buf: Buffer into which to format string + * @end: Pointer to one past @buf's allocated space + * @fmt: Format string + * @...: Format arguments + * + * Formats a string into the buffer at @buf, never writing past @end (including + * terminating \0). It returns a pointer to the \0 at the end of the formatted + * string, allowing iprintf() calls to be chained, building up a long string in + * pieces. + * + * Return: On success, a pointer to the \0 at the end of the formatted string. + * NULL if the formatted string would be truncated or on other errors + */ +__attribute__((format(printf, 3, 4))) +static char *iprintf(char *buf, const char *const end, const char *fmt, ...) +{ + char *next = NULL; + va_list ap; + int n; + + va_start(ap, fmt); + n = vsnprintf(buf, end - buf, fmt, ap); + if (n < 0) { + /* Output error */ + errno = EIO; + goto out; + } + + if (n > (end - buf)) { + /* Truncated */ + errno = ENOSPC; + goto out; + } + + next = buf + n; + +out: + va_end(ap); + return next; +} + +/** sockaddr_ntop() - Convert a socket address to text format + * @sa: Socket address + * @dst: output buffer, minimum SOCKADDR_STRLEN bytes + * @size: size of buffer at @dst + * + * Return: On success, a non-null pointer to @dst, NULL on failure + */ +const char *sockaddr_ntop(const void *sa, char *dst, socklen_t size) +{ + sa_family_t family = ((const struct sockaddr *)sa)->sa_family; + const char *const end = dst + size; + char *p = dst; + + switch (family) { + case AF_INET: { + const struct sockaddr_in *sa4 = sa; + + if (!inet_ntop(AF_INET, &sa4->sin_addr, p, end - p)) + return NULL; + p += strlen(p); + + if (!iprintf(p, end, ":%hu", ntohs(sa4->sin_port))) + return NULL; + + break; + } + + case AF_INET6: { + const struct sockaddr_in6 *sa6 = sa; + + if (!(p = iprintf(p, end, "["))) + return NULL; + + if (!inet_ntop(AF_INET6, &sa6->sin6_addr, p, end - p)) + return NULL; + p += strlen(p); + + if (iprintf(p, end, "]:%hu", ntohs(sa6->sin6_port))) + return NULL; + + break; + } + + /* FIXME: Implement AF_UNIX */ + default: + errno = EAFNOSUPPORT; + return NULL; + } + + return dst; +} diff --git a/util.h b/util.h index 264423b..a637d31 100644 --- a/util.h +++ b/util.h @@ -180,6 +180,19 @@ static inline const char *af_name(sa_family_t af) } }

+/* Maximum required string length to format into decimal */ +#define U16_STRLEN 6 /* "65535\0" */ + +/* inet address (- '\0') + port (u16) (- '\0') + ':' + '\0' */ +#define SOCKADDR_INET_STRLEN (INET_ADDRSTRLEN-1 + U16_STRLEN-1 + 2) + +/* inet6 address (- '\0') + port (u16) (- '\0') + '[' + ']' + ':' + '\0' */ +#define SOCKADDR_INET6_STRLEN (INET6_ADDRSTRLEN-1 + U16_STRLEN-1 + 4)

Isn't it actually less error-prone and more readable to do something like the stuff I dropped in d32edee60a93 ("passt: Use INET{,6}_ADDRSTRLEN instead of open coded sizeof"), say: #define SOCKADDR_INET6_STRLEN \ sizeof("[0123:4567:89ab:cdef:0123:4567:89ab:cdef]:65535") ?

+ +#define SOCKADDR_STRLEN MAX(SOCKADDR_INET_STRLEN, SOCKADDR_INET6_STRLEN) + +const char *sockaddr_ntop(const void *sa, char *dst, socklen_t size); + /** * mod_sub() - Modular arithmetic subtraction * @a: Minued, unsigned value < @m

-- Stefano