On 03.08.2018 15:02, Martin Husemann wrote: > On Fri, Aug 03, 2018 at 02:47:53PM +0200, Kamil Rytarowski wrote: >>> 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); > >> 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. > > But we do not do this here. > >> This change points to the compiler that the code is safe. > > What exactly makes the code safe now? If ntohs(p->ip.ip_len) < > (sizeof(p->ip) + sizeof(p->udp)) then we are now in even more serious > trouble. > The change was indicating to the compiler that code is safe, without changing the algorithm. > Does splitting the term help? > > uint16_t hdr_size = sizeof(p->ip) - sizeof(p->udp); + > uint16_t pkt_size = ntohs(p->ip.ip_len); > KASSERT(pkt_size > hdr_size); > *len = pkt_size > hdr_size ? pkt_size-hdr_size : 0; > > or something like that? > This looks like a safer approach, but I will defer it to Roy to decide what to do. Previously we have agreed with the (size_t) case. > Martin >
Attachment:
signature.asc
Description: OpenPGP digital signature