Re: CVS commit: src/external/bsd/dhcpcd/dist/src

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Fri, 3 Aug 2018 02:17:33 +0000
    From:        "Kamil Rytarowski" <kamil%netbsd.org@localhost>
    Message-ID:  <20180803021733.B2002FBEC%cvs.NetBSD.org@localhost>

  | GCC with -fsanitize=undefiend detects a potential overflow in the code.
  | Cast the return value of ntohs(3) to size_t.

I don't understand the point of this change, and I believe it to
be wrong and misguided.

Further if there ever was a potential problem from this line ...

	*len = ntohs(p->ip.ip_len) - sizeof(p->ip) - sizeof(p->udp);
then
	*len = (size_t)ntohs(p->ip.ip_len) - sizeof(p->ip) - sizeof(p->udp);

will not fix it.   The possible problem being if ip_len < 28 (the sum of the
two sizeof's).    While I didn't check this code, that possibility is usually
verified elsewhere, and even if not, adding the cast changes nothing, the
result is still bogus (that is, at best, the change could be masking a real
bug, though that's unlikely.)

This looks to be (somehow) determining that the result of ntohs(ip_len)
might somehow be negative (or something, I'm not really sure what is
being believed is happening) but the ntohs() result is (in all cases here)
a uint16_t (either because the result is literally the arg, which is that type,
or because it is the result of bswap16() which is declared that way.)

The sizeof() ops make the expression at least a size_t - so unless size_t
and uint16_t are the same (which might happen on a pdp-11 or similar,
though even there it is unlikely I suspect) the narrower one needs to be
promoted - whether the ntohs() result is implicitly promoted to size_t, or
explicitly cast cannot make any difference, can it?

So what exactly is being fixed here?  Which behaviour is supposedly undefined?

kre