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

[Shawn Landden]

On Mon, Mar 9, 2015 at 1:18 PM, Lennart Poettering <lennart at poettering.net>
wrote:

> 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.
>
> Taking this out would be pretty gruesome because of use of the
post-increment operator.

> >
> > -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
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>



-- 
Shawn Landden
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20150309/2adef8e3/attachment.html>