On 03.08.2018 12:26, Robert Elz wrote: > 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? > 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