Wrong payload displayed in responses

[s1198576]

Describe the bug
The ICMP responses displayed are showing the wrong payload, which doesn't change when the size option is specified. You will also see when sweep start and end are specified, all responses show the same payload. You will get failures when the Path MTU is exceeded, showing that in the background the payload is changing.

To Reproduce
See below for the execution of an MTU sweep, with all payloads displaying the same value.

ping('8.8.8.8', sweep_start=1000, sweep_end=1472)
Reply from 8.8.8.8, 76 bytes in 5.48ms
Reply from 8.8.8.8, 76 bytes in 5.53ms
Reply from 8.8.8.8, 76 bytes in 5.68ms
Reply from 8.8.8.8, 76 bytes in 5.66ms
Reply from 8.8.8.8, 76 bytes in 5.43ms
Reply from 8.8.8.8, 76 bytes in 5.48ms
Reply from 8.8.8.8, 76 bytes in 5.65ms
Reply from 8.8.8.8, 76 bytes in 5.43ms
Reply from 8.8.8.8, 76 bytes in 5.62ms
Reply from 8.8.8.8, 76 bytes in 5.68ms
Reply from 8.8.8.8, 76 bytes in 5.45ms
Reply from 8.8.8.8, 76 bytes in 5.51ms
Reply from 8.8.8.8, 76 bytes in 5.56ms

Expected behavior
ICMP response should be of the same size as the request. It is expected that the response will reflect the same payload as specified in the ICMP request.

Screenshots
See copy and past of results in the steps to reproduce section.

Desktop (please complete the following information):

• OS: Red Hat 8.1
• Browser: N/A
• Version: Python 3

Smartphone (please complete the following information):

• Device: N/A
• OS: N/A
• Browser: N/A
• Version: N/A

Additional context

[alessandromaggio]

We need to look further into this. I gave a quick glance at the RFC792 and it does not seem to indicate that the size of the reply should match the size of the ICMP reply should match the size of the ICMP request, so this behavior may be related to the destination host as well.

Can you please repeat the test while also running a WireShark capture at the same time and then share the result, so that we can compare the PCAP file with the pythonping result?

Edit: this may also be aestethic related to showing the incoming payload rather than source payload, which according to your use case is what you would be looking for.

[s1198576]

Sorry for the delay, I've been busy on other things. For quick reference here is the packet capture results from a different system. This ping was performed from a Windows CLI specifying a payload of 1200 bytes. IP+ICMP includes 28 bytes of overhead, so the remaining 14 bytes must be the frame encapsulation, for a total of 1242. This is the size displayed for both the request and reply. Thanks!

[alessandromaggio]

Just switched to displaying the payload of the packet with commit 145af92, however calculating the packet size still seems off. I noticed on Windows the same packet is displayed twice as a response, even if the NIC sees only one packet. We will need to look deeper into this.

[s1198576]

Thank you! Much appreciated. Having this insight is beneficial when determining PMTU, which becomes increasingly important with the addition of network overhead from things like VPNs. Happy New Year!

…

On Wed, Dec 29, 2021 at 9:45 AM Alessandro Maggio ***@***.***> wrote: Just switched to displaying the payload of the packet with commit 145af92 <145af92>, however calculating the packet size still seems off. I noticed on Windows the same packet is displayed twice as a response, even if the NIC sees only one packet. We will need to look deeper into this. — Reply to this email directly, view it on GitHub <#74 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADL67IJR3V356KVISZ7BK23UTMNH5ANCNFSM5HHQNIFQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[alessandromaggio]

I implemented in release 1.1.1, just published. Calculations were just off and this has been fixed, however the previous output format showed the size of the reply packet. It still shows that, but now with correct sizing.
To better address this issue I also added a new out_format option which defaults to legacy and can be set to None to start to use the new format, which will be the default in the future. Useful for ping sweeps.

ping('8.8.8.8', sweep_start=10, sweep_end=20, verbose=True, out_format=None)

status=OK       from=8.8.8.8    ms=5.55         bytes   snt=38  rcv=38
status=OK       from=8.8.8.8    ms=5.38         bytes   snt=39  rcv=39
status=OK       from=8.8.8.8    ms=4.92         bytes   snt=40  rcv=40
status=OK       from=8.8.8.8    ms=4.81         bytes   snt=41  rcv=41
status=OK       from=8.8.8.8    ms=4.71         bytes   snt=42  rcv=42
status=OK       from=8.8.8.8    ms=4.8          bytes   snt=43  rcv=43
status=OK       from=8.8.8.8    ms=4.39         bytes   snt=44  rcv=44
status=OK       from=8.8.8.8    ms=4.82         bytes   snt=45  rcv=45
status=OK       from=8.8.8.8    ms=5.2          bytes   snt=46  rcv=46
status=OK       from=8.8.8.8    ms=5.32         bytes   snt=47  rcv=47
status=OK       from=8.8.8.8    ms=5.14         bytes   snt=48  rcv=48

We will make this default after implementing some more tests and test coverage, in a breaking release. For now we can consider this issue closed!

Happy New Year!

[s1198576]

Thank you so much! Have you considered putting payload as an option in the response list. This would be helpful when using ping sweep and not programmatically setting each payload. If you are interested I can submit an additional request. This is all very helpful for those traversing many long and complex paths where MTU isn’t known and you have PMTU discovery filtered by 3rd parties, ICMP type 3 code 4 - packet too big. Thanks again and happy New Year!

…

On Fri, Dec 31, 2021 at 4:27 AM Alessandro Maggio ***@***.***> wrote: I implemented in release 1.1.1, just published. Calculations were just off and this has been fixed, however the previous output format showed the size of the reply packet. It still shows that, but now with correct sizing. To better address this issue I also added a new out_format option which defaults to legacy and can be set to None to start to use the new format, which will be the default in the future. Useful for ping sweeps. ping('8.8.8.8', sweep_start=10, sweep_end=20, verbose=True, out_format=None) status=OK from=8.8.8.8 ms=5.55 bytes snt=38 rcv=38 status=OK from=8.8.8.8 ms=5.38 bytes snt=39 rcv=39 status=OK from=8.8.8.8 ms=4.92 bytes snt=40 rcv=40 status=OK from=8.8.8.8 ms=4.81 bytes snt=41 rcv=41 status=OK from=8.8.8.8 ms=4.71 bytes snt=42 rcv=42 status=OK from=8.8.8.8 ms=4.8 bytes snt=43 rcv=43 status=OK from=8.8.8.8 ms=4.39 bytes snt=44 rcv=44 status=OK from=8.8.8.8 ms=4.82 bytes snt=45 rcv=45 status=OK from=8.8.8.8 ms=5.2 bytes snt=46 rcv=46 status=OK from=8.8.8.8 ms=5.32 bytes snt=47 rcv=47 status=OK from=8.8.8.8 ms=5.14 bytes snt=48 rcv=48 We will make this default after implementing some more tests and test coverage, in a breaking release. For now we can consider this issue closed! Happy New Year! — Reply to this email directly, view it on GitHub <#74 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADL67IIMIOD4TYDLLZWJXELUTVZRXANCNFSM5HHQNIFQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>