On Fri, 17 May 2024 13:30:17 +1000
David Gibson <david(a)gibson.dropbear.id.au> 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 <david(a)gibson.dropbear.id.au>
> ---
> 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 <arpa/inet.h>
#include <limits.h>
#include <stdint.h>
#include <stdio.h>
#include <string.h>
#include <sys/random.h>
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