Repository navigation
Conversation
|
@pavlix @kerolasa @jsynacek @nmeyerhans @vapier @nmav @paul-demo Feel free to review. |
ddec23e to
c6d9c11
Compare
Yes: Paul Swirhun, paulswirhun@gmail.com I'd like one of the owners to take responsibility for checking this bug/fix; I don't want or need any credit I just want it fixed! Thanks, |
nmeyerhans
left a comment
There was a problem hiding this comment.
This looks good to me. The behavior when processing truncated packets is restored.
We might consider whether we want to report the expected packet length in the output when we log a truncated packet, but that doesn't need to happen here.
I am curious about the issue that #67 was trying to fix in the first place. I wonder if it was an actual observed problem or something theoretical. I'd love to see a repro case for it.
715f832 to
0ec5658
Compare
Thanks for a review!
+1, yep, I'd add it as a separate PR.
That'd be great to have. @locasity care to post more info or even a reproducer. |
…ided" This reverts commit f7710a1. Commit broke report of truncated packets: $ ping -c2 -s100 google.com PING google.com (142.250.185.238) 100(128) bytes of data. Running ping from both s20161105 (which does not contain f7710a1) and reverted f7710a1 on master reports truncated packets: $ ping -c2 -s100 google.com PING google.com (142.250.185.238) 100(128) bytes of data. 76 bytes from fra16s53-in-f14.1e100.net (142.250.185.238): icmp_seq=1 ttl=116 (truncated) 76 bytes from fra16s53-in-f14.1e100.net (142.250.185.238): icmp_seq=2 ttl=116 (truncated) There was unreachable code in gather_statistics() because contains_pattern_in_payload() added in f7710a1 always found a mismatch first. Due that all of these did not work: * updating counters for statistics generation * keeping track of timestamps and time-of-flight using the first section of the payload * checking for duplicate replies and report them * printing basic info about the reply * printing "(truncated)" if the reply was truncated * checking the checksum * validating the rest of the payload (bytes 17 and above) against the ICMP request that was sent, and report any differences Fixes: f7710a1 ("Add strict pattern matching on response when pattern was provided") Closes: iputils#320 Reported-by: Paul Swirhun <paulswirhun@gmail.com> Suggested-by: Paul Swirhun <paulswirhun@gmail.com> Reviewed-by: Noah Meyerhans <noahm@debian.org> Signed-off-by: Petr Vorel <pvorel@suse.cz>
|
Thanks for looping me in (@locasity here) IIRC the intention of the pattern matching code was to only be called if requested using the "strict mode" flag. This was in response to a production issue. The production code was a perl script shelling out to run ping and traceroute concurrently at a large scale. At that scale we were seeing traceroute responses being misconstrued for ping responses once every few days (due to pid warp around), leading to a "hung" ping process. Repro instructions from when I reported it in 2016 are here- https://bugs.launchpad.net/ubuntu/+source/iputils/+bug/1551020 |
|
@Sunjeet thanks for info. I tested your reproducer mentioned in https://bugs.launchpad.net/ubuntu/+source/iputils/+bug/1551020, but it's obviously problematic to some specific host you didn't mention. Thus I decide to revert your implementation to get back truncate packets. Please, if the problem you tried to solve persists, report it again with host, thus we can reproduce it and write test for it. |
|
@pevik sounds good to me |
This reverts commit f7710a1.
Commit broke report of truncated packets:
Running ping from both s20161105 (which does not contain f7710a1) and
reverted f7710a1 on master reports truncated packets:
"truncated" statistics never happend and
there was unreachable code in gather_statistics() because contains_pattern_in_payload() added in
f7710a1 always found a mismatch first and nothing got printed.
There was unreachable code in gather_statistics() because
contains_pattern_in_payload() added in f7710a1 always found a mismatch
first. Due that all of these did not work:
of the payload
ICMP request that was sent, and report any differences
Fixes: f7710a1 ("Add strict pattern matching on response when pattern was provided")
Closes: #320
Reported-by: paul-demo
Reviewed-by: Petr Vorel pvorel@suse.cz