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

[Ronny Chevalier]

2015-03-09 23:09 GMT+01:00 Shawn Landden <shawn at churchofgit.com>:
> 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.

Maybe by using a temporary variable like this:

char *env_str;

[...]

env_str = strappend("REMOTE_ADDR", addr);
if (!env_str) {
        r = -ENOMEM;
        goto fail;
}
our_env[n_env++] = env_str;

And the same with the other one ?

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