Re: [PATCH v2 0/3] daemon: fix network address handling bugs

[Junio C Hamano <gitster@pobox.com>]

"Sebastien Tardif via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Fix three related issues in daemon.c's network address handling:
>
> IPv6 address corruption in lookup_hostname(): getaddrinfo() is called with
> AF_UNSPEC hints, so it may return IPv6 results. However, the code
> unconditionally casts ai_addr to sockaddr_in and passes AF_INET to
> inet_ntop(). On IPv6-only hosts, this reads from the wrong struct offset,
> producing garbage IP addresses. Fixed by checking ai_family and handling
> both AF_INET and AF_INET6.
>
> IPv6 address truncation in ip2str(): The sockaddr struct size (ai_addrlen)
> is passed as the output buffer size to inet_ntop(). For IPv6,
> sizeof(sockaddr_in6) is 28 bytes but INET6_ADDRSTRLEN is 46, so long IPv6
> addresses are silently truncated. Fixed by passing sizeof(ip) instead, and
> dropping the now-unused len parameter.
>
> NULL pointer in execute() logging: REMOTE_PORT environment variable is used
> in a format string without a NULL check (only REMOTE_ADDR was checked). If
> REMOTE_PORT is unset, NULL is passed to printf's %s, which is undefined
> behavior. Fixed by using a fallback string.
>
> Changes since v1:
>
>  * Split the single patch into three separate commits, one per fix, per
>    Patrick's review.

This, and all the other items in this list, are differences between
the version before v1 and v2, isn't it?  It is OK to pretend that
the pre-v1 version v0 didn't officially exist, but it would be
helpful to see the inter-version improvements for *this* version.

Indeed, range-diff tells us that the commit log improvement is the
only change since the previous iteration.

> Range-diff vs v1:
>
>  1:  b2d8143811 = 1:  b2d8143811 daemon: fix IPv6 address corruption in lookup_hostname()
>  2:  5c01ec3cad = 2:  5c01ec3cad daemon: fix IPv6 address truncation in ip2str()
>  3:  1b2f9d1a07 ! 3:  e312735716 daemon: guard NULL REMOTE_PORT in execute() logging
>      @@ Metadata
>        ## Commit message ##
>           daemon: guard NULL REMOTE_PORT in execute() logging
>       
>      -    The REMOTE_PORT environment variable is used in a format string
>      -    without a NULL check, while REMOTE_ADDR is checked. If REMOTE_PORT
>      -    is unset, NULL is passed to printf's %s, which is undefined behavior.
>      +    REMOTE_ADDR and REMOTE_PORT are both set by the same code path in
>      +    handle(), so neither should be NULL independently. However, the
>      +    existing code checks REMOTE_ADDR before the loginfo() call but not
>      +    REMOTE_PORT. If REMOTE_PORT were unset, NULL would be passed to
>      +    printf's %s, which is undefined behavior.

This is easier to read than the previous, but it is unclear what the
change is trying to achieve.  You first say if addr is set port can
never be unset.  So by checking addr before calling loginfo(), the
code effectively is ensuring that addr and port are set.  

 (1) The word "However" in "However the existing code checks" does
     not make much sense to me (I would think "Therefore" is less
     confusing, but if what you first said is correct, then it is
     quite obvious and can be left unsaid).

 (2) It is unclear why "If REMOTE_PORT were unset NULL would be ..."
     needs to be brought up.  Yes, you are not supposed to pass NULL
     to printf that expects "%s" to format it.  But isn't the whole
     point of checking that addr is not NULL because the caller
     knows that loginfo() accesses both, and the caller also knows
     that if addr is not NULL, port will never be NULL?  Or is this
     comment about something other than loginfo() where port is used
     without checking neither addr or port?  Then it would not make
     much sense to bring up "addr is checked before calling
     loginfo()".

IOW, the sentence structure got vastly improved than the previous
round, but it made it clearer that what these sentences say is
unclear ;-).

>      -    Add a fallback string for the NULL case.
>      +    Add a fallback string for the NULL case, matching the existing
>      +    REMOTE_ADDR guard for consistency.

I tried to find if there is any existing case (addr ? addr : "") to
match, but I didn't find any.  Probably that is because it is not
needed (instead the code does "if (addr) ..." to protect itself).

I think the only valid justification you could give to this change
is to say that even though the current code is perfectly fine as-is
(i.e. as you said, addr and port are both exported at the same time
so it will never happen that addr is non NULL and port is NULL),
somebody who is not so careful can break that arrangement in the
future, and it is a prudent thing to double check that port is not
NULL before using will future-proof this part of the code.

Thanks.