Fixed wrong icmp header length

[kylezhou]

ping_receive_ipv4() and ping_send_one_ipv4() incorrectly used struct icmp, whereas struct icmphdr should be used. struct icmp includes a 20-byte icmp_dun after icmp_hun, which causes the echo request to include an extra 20-byte zeros between icmp header and payload.

[octo]

Hi @kylezhou, thank you very much for your patch! Unfortunately, struct icmphdr is not portable: on FreeBSD it is four bytes long: type, code and checksum. On Linux it is eight bytes long, because it also includes a four byte union, e.g. for the echo ID and sequence number. Maybe it would be easiest if we defined these struct ourselves …

I'm curious why I don't see the extra bytes on the network though. Is the kernel being smart on us?

Best regards,
—octo

[kylezhou]

Hi @octo, I was not aware of the difference in BSD. You are right struct icmphdr is not a good choice. It even does not exist on osx. Should we simply use the macro ICMP_MINLEN in place of sizeof(struct icmphdr), which is always defined to 8 in all platforms I checked.

[kylezhou]

I tested the unchanged code on both an Ubuntu PC and an ARM router device, both show the extra 20 bytes. The icmp_dun field in struct icmp is the culprit. Did you do wireshark/tcpdump?

[octo]

Yeah, I compiled a clean master on an Ubuntu box with the eight byte struct icmphdr, captured the packets with tcpdump and visualized with Wireshark. No extra bytes between header and payload … ICMP_MINLEN looks promising – other tools such a Busybox and traceroute appear to be using it as well.

[octo]

Err, the eight byte struct icmphdr is of course not relevant to this bug: it used the large struct icmp with the icmp_dun union.

[kylezhou]

sorry, octo. I might misunderstood. when you did tcpdump, you used struct icmphdr or struct icmp?

[octo]

I used the vanilla master branch, i.e. with struct icmp and the offset that should have been too large.

I've replaced the sizeof (struct icmphdr) with ICMP_MINLEN now and cleaned up some other code while I was at it. Thanks again for fixing this!

Best regards,
—octo