On 03/08/2018 14: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);
        *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

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?

We could split the term, but merely storing the result of htons in it's own variable creates a larger binary for no good reason as i see it.

Considering both methods work and the result of htons is a uint16_t (but is this always guaranteed?) is this just an over-zealous compiler warning?


Reply via email to