[systemd-devel] [PATCH] add REMOTE_ADDR and REMOTE_PORT for Accept=yes

[Lennart Poettering]

On Mon, 09.03.15 13:09, Shawn Landden (shawn at churchofgit.com) wrote:

> +        if (UNIT_DEREF(s->accept_socket)) {
> +                union sockaddr_union sa;
> +                socklen_t salen = sizeof(sa);
> +
> +                r = getpeername(s->socket_fd, &sa.sa, &salen);
> +                if (r < 0) {
> +                        r = -errno;
> +                        goto fail;
> +                }
> +
> +                if (sa.sa.sa_family == AF_INET ||
> +                    sa.sa.sa_family == AF_INET6) {
> +                        _cleanup_free_ char *addr = NULL;
> +                        uint16_t port = (uint16_t)sockaddr_port(&sa);

We try to avoid invoking functions in the same lines as we declare
variables.

Also, even though this cannot fail in this case, I think it would be
nicer, to make port an int, and check for < 0 and return an error in
that case...

> +
> +                        r = sockaddr_pretty(&sa.sa, salen, true, false, &addr);
> +                        if (r < 0)
> +                                goto fail;
> +
> +                        if (!(our_env[n_env++] = strappend("REMOTE_ADDR=", addr))) {

In newer code we try to avoid making assignments and doing if checks
in the same line.

>  
> -int sockaddr_pretty(const struct sockaddr *_sa, socklen_t salen, bool translate_ipv6, char **ret) {
> +int sockaddr_port(const union sockaddr_union *sa) {
> +        assert_return(sa->sa.sa_family == AF_INET6 ||
> +                      sa->sa.sa_family == AF_INET,
> +                      -ENOTSUP);

assert_return() is a macro to use only for cases where there's a clear
programming error of the caller. But I am pretty sure that this is not
the case here. If it gets called on an non-AF_INET/AF_INET6 socket
then this should fail without logging.

Also I think an error code of EAFNOSUPPORT would be more appropriate.

Hmm, and I think this should probably take an actual "struct
sockaddr", rather than a union sockaddr_union...

> @@ -475,42 +485,40 @@ int sockaddr_pretty(const struct sockaddr *_sa, socklen_t salen, bool translate_
>  
>          switch (sa->sa.sa_family) {
>  
> -        case AF_INET: {
> -                uint32_t a;
> -
> -                a = ntohl(sa->in.sin_addr.s_addr);
> -
> -                if (asprintf(&p,
> -                             "%u.%u.%u.%u:%u",
> -                             a >> 24, (a >> 16) & 0xFF, (a >> 8) & 0xFF, a & 0xFF,
> -                             ntohs(sa->in.sin_port)) < 0)
> -                        return -ENOMEM;
> -
> -                break;
> -        }
> -
> +        case AF_INET:
>          case AF_INET6: {
> -                static const unsigned char ipv4_prefix[] = {
> -                        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xFF, 0xFF
> -                };
> -
> -                if (translate_ipv6 && memcmp(&sa->in6.sin6_addr, ipv4_prefix, sizeof(ipv4_prefix)) == 0) {
> -                        const uint8_t *a = sa->in6.sin6_addr.s6_addr+12;
> +                char a[MAX(INET6_ADDRSTRLEN, INET_ADDRSTRLEN)];
> +                const char *addr;
> +                bool ipv4_mapped = false;
> +
> +                if (inet_ntop(sa->sa.sa_family,
> +                              /* this field of the API is kinda braindead,
> +                               * should take head of struct so it can be passed the union...*/
> +                              sa->sa.sa_family == AF_INET6 ?
> +                                &sa->in6.sin6_addr :
> +                                &sa->in.sin_addr,
> +                              a, sizeof(a)) == NULL)
> +                      return -ENOMEM;
> +
> +                /* glibc inet_ntop() presents v4-mapped addresses in ::ffff:a.b.c.d form */
> +                if (translate_ipv6 && sa->sa.sa_family == AF_INET6 && strchr(a, '.')) {
> +                        ipv4_mapped = true;
> +                        addr = strempty(startswith(a, "::ffff:"));

I think it would be a lot nicer to check the raw IP prefix as before,
and only then format things, then first formatting and then looking at
the string again. 

We shouldn't bind us too closely to the precise formatting. I mean, if
one day libc decides to format the thing as ipv6 again, or with
uppercase hex chars, we shouldn't break.

> +                } else
> +                        addr = &a[0];
> +
> +                if (include_port) {
> +                        uint16_t port = (uint16_t)sockaddr_port(sa);

No function calls in variable declarations, please. Same rule as above applies.
Lennart

-- 
Lennart Poettering, Red Hat