On 03.08.2018 12:26, Robert Elz wrote:
>     Date:        Fri, 3 Aug 2018 02:17:33 +0000
>     From:        "Kamil Rytarowski" <ka...@netbsd.org>
>     Message-ID:  <20180803021733.b2002f...@cvs.netbsd.org>
>   | 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?

It was a build time error generated by GCC. The compiler detected that
both sizeof() could be large enough to overflow the result returned from
 ntohs(3). And overflowing a signed integer is Undefined Behavior.

This change points to the compiler that the code is safe.

> kre

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to