merge google/gopacket PR https://github.com/google/gopacket/pull/754 #79
Conversation
… allocation and copy during serialization by https://github.com/cjacobs96, see PR: google/gopacket#754
…ing icmp layer payload not transmitting
| @@ -231,17 +231,28 @@ func (i *ICMPv4) DecodeFromBytes(data []byte, df gopacket.DecodeFeedback) error | |||
| return nil | |||
| } | |||
|
|
|||
| // fix from google/gopacket pull request by https://github.com/cjacobs96 | |||
There was a problem hiding this comment.
This shouldn't appear in godoc.
|
|
||
| startIndex := 8 //start at after header offset | ||
| //copy payload into buffer | ||
| for _, element := range i.Payload { |
There was a problem hiding this comment.
This is incorrect.
According to BaseLayer godoc, i.Payload is the set of bytes contained by (but not part of) this Layer.
If you want to encode payload in ICMPv4 packet, it should be a separate gopacket.Payload layer.
There was a problem hiding this comment.
ok, I guess that makes sense. can this reconcile with the fact that not including a payload produces truncated ICMPv4 ping? I feel like the default behavior should still be a complete ICMPv4, but maybe you mean the logic in constructing the packet should always include Payload layer separately and we're doing it wrong to achieve non-truncated result as users of the library?
There was a problem hiding this comment.
the logic in constructing the packet should always include Payload layer separately
Yes, if you need the payload.
merge google/gopacket pull request, fixing icmp4 layer payload buffer allocation and copy during serialization by https://github.com/cjacobs96, see PR: google/gopacket#754
I needed this bug fixed so my project sneknet could handle emulating a gateway including pinging the gateway (even though it is purely a software device/tuntap)
I did my best to try and create a PR from the original PR, however I think it may not be possible due to the way gopacket/gopacket is forked from google/gopacket, either that or I don't know enough. Not trying to take credit from cjacobs96, he did all the actual work. I just tested it on my machine.
Ping before pull request merge: (payload not present, only header 8 bytes AKA truncated)
ping -s 128 10.0.0.2 -I sneknet_tap0 PING 10.0.0.2 (10.0.0.2) from 10.0.0.1 sneknet_tap0: 128(156) bytes of data. 8 bytes from 10.0.0.2: icmp_seq=1 ttl=64 (truncated) 8 bytes from 10.0.0.2: icmp_seq=2 ttl=64 (truncated) 8 bytes from 10.0.0.2: icmp_seq=3 ttl=64 (truncated) 8 bytes from 10.0.0.2: icmp_seq=4 ttl=64 (truncated) ^C --- 10.0.0.2 ping statistics --- 4 packets transmitted, 4 received, 0% packet loss, time 3067ms rtt min/avg/max/mdev = 9223372036854775.807/0.000/0.000/0.000 msPing after pull request merge:
ping -s 128 10.0.0.2 -I sneknet_tap0 PING 10.0.0.2 (10.0.0.2) from 10.0.0.1 sneknet_tap0: 128(156) bytes of data. 136 bytes from 10.0.0.2: icmp_seq=1 ttl=64 time=0.152 ms 136 bytes from 10.0.0.2: icmp_seq=2 ttl=64 time=0.210 ms 136 bytes from 10.0.0.2: icmp_seq=3 ttl=64 time=0.186 ms ^C --- 10.0.0.2 ping statistics --- 3 packets transmitted, 3 received, 0% packet loss, time 2031ms rtt min/avg/max/mdev = 0.152/0.182/0.210/0.023 ms