Bug 151023 - [patch] ping6(8) exits before all ICMPv6 echo replies are received
Summary: [patch] ping6(8) exits before all ICMPv6 echo replies are received
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 7.2-RELEASE
Hardware: Any Any
: Normal Affects Only Me
Assignee: Xin LI
URL:
Keywords: patch-ready
Depends on:
Blocks:
 
Reported: 2010-09-28 10:00 UTC by Tianjie Mao
Modified: 2019-11-30 06:26 UTC (History)
6 users (show)

See Also:


Attachments
file.diff (982 bytes, patch)
2010-09-28 10:00 UTC, Tianjie Mao
no flags Details | Diff
A patch to add timeout and waittime options which ping(8) supports. (5.32 KB, patch)
2014-09-21 16:04 UTC, Hiroki Sato
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tianjie Mao 2010-09-28 10:00:23 UTC
An interval timer (itimer) allows a process to do repeated work at a given frequency.  If an -i or -f option is specified, the FreeBSD ping6(8) will use this value to call setitimer(2) to register the timer, then the signal handler is called at every expiry of the timer, sending an ICMPv6 packet each time.

However, the KAME version of ping6 utility immediately exits the loop after knowing that it is the last packet to send, which causes unread replies from a remote host to be ignored.

This is a bug and can be fixed by setting itimer to zero as soon as the last packet is sent, then continue the loop to see if any replies are received.

Fix: Apply the attached patch to src/sbin/ping6/ping6.c and the problem will be fixed.

Tested against CVS revision 1.39 but this is supposed to work on older versions as well.

Patch attached with submission follows:
How-To-Repeat: Suppose there is an average RTT seconds round-trip delay between v6host1 and v6host2.
INT is the interval (seconds) between packets.
CNT is the total number of packets to send.

Run as root on v6host1:
v6host1# ping6 -i $INT -c $CNT -q v6host2

Then the reply might look like this (0.01 interval, 245.816 ms delay, 100 packets):

--- ipv6.sjtu.edu.cn ping6 statistics ---
100 packets transmitted, 76 packets received, 24.0% packet loss
round-trip min/avg/max/std-dev = 244.446/245.816/247.826/0.705 ms

After re-adjusting $INT and switching to another destination host with a different RTT, one would simply conclude with the following formula:

estimated-packets-lost = RTT / INT
Comment 1 Xin LI freebsd_committer 2014-07-28 08:22:31 UTC
MFC reminder.
Comment 2 commit-hook freebsd_committer 2014-07-28 08:23:10 UTC
A commit references this bug:

Author: delphij
Date: Mon Jul 28 08:22:08 UTC 2014
New revision: 269180
URL: http://svnweb.freebsd.org/changeset/base/269180

Log:
  When interval is set to very small value with limited amount of packets,
  ping6(8) would quit before the remote side gets a chance to respond.

  Solve this by resetting the itimer when we have reached the maximum packet
  number have reached, but let the other handling to continue.

  PR:		bin/151023
  Submitted by:	tjmao at tjmao.net
  MFC after:	2 weeks

Changes:
  head/sbin/ping6/ping6.c
Comment 3 commit-hook freebsd_committer 2014-08-11 06:54:41 UTC
A commit references this bug:

Author: delphij
Date: Mon Aug 11 06:54:07 UTC 2014
New revision: 269800
URL: http://svnweb.freebsd.org/changeset/base/269800

Log:
  MFC r269180:

  When interval is set to very small value with limited amount of packets,
  ping6(8) would quit before the remote side gets a chance to respond.

  Solve this by resetting the itimer when we have reached the maximum packet
  number have reached, but let the other handling to continue.

  PR:		bin/151023
  Submitted by:	tjmao at tjmao.net

Changes:
_U  stable/10/
  stable/10/sbin/ping6/ping6.c
Comment 4 commit-hook freebsd_committer 2014-08-11 07:01:43 UTC
A commit references this bug:

Author: delphij
Date: Mon Aug 11 07:00:57 UTC 2014
New revision: 269802
URL: http://svnweb.freebsd.org/changeset/base/269802

Log:
  MFC r269180:

  When interval is set to very small value with limited amount of packets,
  ping6(8) would quit before the remote side gets a chance to respond.

  Solve this by resetting the itimer when we have reached the maximum packet
  number have reached, but let the other handling to continue.

  PR:        bin/151023
  Submitted by:    tjmao at tjmao.net

Changes:
_U  stable/9/sbin/ping6/
  stable/9/sbin/ping6/ping6.c
Comment 5 commit-hook freebsd_committer 2014-08-11 07:01:44 UTC
A commit references this bug:

Author: delphij
Date: Mon Aug 11 07:01:29 UTC 2014
New revision: 269803
URL: http://svnweb.freebsd.org/changeset/base/269803

Log:
  MFC r269180:

  When interval is set to very small value with limited amount of packets,
  ping6(8) would quit before the remote side gets a chance to respond.

  Solve this by resetting the itimer when we have reached the maximum packet
  number have reached, but let the other handling to continue.

  PR:        bin/151023
  Submitted by:    tjmao at tjmao.net

Changes:
_U  stable/8/sbin/ping6/
  stable/8/sbin/ping6/ping6.c
Comment 6 Xin LI freebsd_committer 2014-08-11 07:03:07 UTC
Mark as resolved.  I don't have much time to work on this further right now, Bruce Evans have some recommendations that would be great to get addressed:

http://lists.freebsd.org/pipermail/svn-src-head/2014-July/060928.html
Comment 7 Hiroki Sato freebsd_committer 2014-09-20 18:40:02 UTC
The committed patch in r269180 is wrong because ping6 can enter an infinite loop when -c is specified and the last packet is not received.  I will revert the change for now.
Comment 8 commit-hook freebsd_committer 2014-09-20 18:49:49 UTC
A commit references this bug:

Author: hrs
Date: Sat Sep 20 18:48:51 UTC 2014
New revision: 271909
URL: http://svnweb.freebsd.org/changeset/base/271909

Log:
  Revert changes in r269180.  It could cause -c N option to enter an
  infinite loop if no reply packet is received.

  PR:	151023

Changes:
  head/sbin/ping6/ping6.c
Comment 9 commit-hook freebsd_committer 2014-09-20 19:54:56 UTC
A commit references this bug:

Author: hrs
Date: Sat Sep 20 19:54:19 UTC 2014
New revision: 271910
URL: http://svnweb.freebsd.org/changeset/base/271910

Log:
  Fix a problem that reply packets are not received when -i T option is set
  and (T < RTT).

  - Use select(2) for timeout instead of interval timer. Remove poll(2) support.
  - Use sigaction(2) instead of signal(3).
  - Exit in SIGINT handler when two signals are received and doing reverse DNS
    lookup as ping(8) does.
  - Remove redundant variables used for getaddrinfo(3).

  PR:	151023

Changes:
  head/sbin/ping6/Makefile
  head/sbin/ping6/ping6.c
Comment 10 Hiroki Sato freebsd_committer 2014-09-20 20:00:02 UTC
I committed a fix in r271910.  Please test it and let me know if the problem still persists or not.
Comment 11 John Hay freebsd_committer 2014-09-21 08:13:15 UTC
r271910 does not hang when no reply packets are received, but it does wait a lot longer then the old pre r269180 code did, new code first:

root@zibbi:/home/jhay/ping6 # /usr/bin/time ./ping6 -c 1 2001:4200:7000:1::90
PING6(56=40+8+8 bytes) 2001:4200:7000:1:d6ae:52ff:fed1:9981 --> 2001:4200:7000:1::90

--- 2001:4200:7000:1::90 ping6 statistics ---
1 packets transmitted, 0 packets received, 100.0% packet loss
       11.05 real         0.00 user         0.00 sys
root@zibbi:/home/jhay/ping6 # /usr/bin/time /sbin/ping6 -c 1 2001:4200:7000:1::90
PING6(56=40+8+8 bytes) 2001:4200:7000:1:d6ae:52ff:fed1:9981 --> 2001:4200:7000:1::90

--- 2001:4200:7000:1::90 ping6 statistics ---
1 packets transmitted, 0 packets received, 100.0% packet loss
        1.00 real         0.00 user         0.00 sys
root@zibbi:/home/jhay/ping6 #

The new code waits 11 seconds for a ping6 -c 1, while the old code only waited
1 second. Adding -w 1 makes it even shorter:

root@zibbi:/home/jhay/ping6 # /usr/bin/time ./ping6 -c 1 -w 1 2001:4200:7000:1::90
ping6: hostname nor servname provided, or not known
        0.01 real         0.00 user         0.01 sys
root@zibbi:/home/jhay/ping6 #

root@zibbi:/home/jhay/ping6 # fgrep '$FreeBSD:' ping6.c 
__FBSDID("$FreeBSD: head/sbin/ping6/ping6.c 271910 2014-09-20 19:54:19Z hrs $");
root@zibbi:/home/jhay/ping6 #
Comment 12 Hiroki Sato freebsd_committer 2014-09-21 12:06:52 UTC
(In reply to John Hay from comment #11)
> r271910 does not hang when no reply packets are received, but it does wait a
> lot longer then the old pre r269180 code did, new code first:

 The original code is intended to wait for 10 seconds if no reply packet
 is received.  It waits for 2 * RTT if at least one reply is received.
 It is compatible with ping(8); you can check it by "ping -c N".
 The old behavior of ping6(8) did not work as intended due to a bug.

> The new code waits 11 seconds for a ping6 -c 1, while the old code only
> waited
> 1 second. Adding -w 1 makes it even shorter:

 In ping(8), 10 seconds wait can be shorten by using waittime (-w) option.
 However, -w of ping6(8) is not an option for waittime.  Actually
 ping6(8) does not have such an option.  Is it better to implement
 an waittime option in ping6(8)?
Comment 13 Hiroki Sato freebsd_committer 2014-09-21 16:04:34 UTC
Created attachment 147528 [details]
A patch to add timeout and waittime options which ping(8) supports.

This is a patch to implement timeout and waittime options.  In ping(8) they are "-t timeout" and "-W waittime", but in ping6(8) both are already used for other purposes.  This patch uses "-x waittime" and "-X timeout".
Comment 14 John Hay freebsd_committer 2014-09-22 18:44:23 UTC
I have tried your patch and -X is working like I expect:

root@zibbi:/home/jhay/ping6 # /usr/bin/time ./ping6 -c 1 -X 2 2001:4200:7000:1::90
PING6(56=40+8+8 bytes) 2001:4200:7000:1:d6ae:52ff:fed1:9981 --> 2001:4200:7000:1::90

--- 2001:4200:7000:1::90 ping6 statistics ---
1 packets transmitted, 0 packets received, 100.0% packet loss
        2.02 real         0.00 user         0.00 sys
root@zibbi:/home/jhay/ping6 #

But -x seems to add 1 second to the -x millisecond value or do I misunderstand it?

root@zibbi:/home/jhay/ping6 # /usr/bin/time ./ping6 -c 1 -x 500 2001:4200:7000:1::90
PING6(56=40+8+8 bytes) 2001:4200:7000:1:d6ae:52ff:fed1:9981 --> 2001:4200:7000:1::90

--- 2001:4200:7000:1::90 ping6 statistics ---
1 packets transmitted, 0 packets received, 100.0% packet loss
        1.53 real         0.00 user         0.00 sys
root@zibbi:/home/jhay/ping6 # /usr/bin/time ./ping6 -c 1 -x 1500 2001:4200:7000:1::90
PING6(56=40+8+8 bytes) 2001:4200:7000:1:d6ae:52ff:fed1:9981 --> 2001:4200:7000:1::90

--- 2001:4200:7000:1::90 ping6 statistics ---
1 packets transmitted, 0 packets received, 100.0% packet loss
        2.54 real         0.00 user         0.00 sys
root@zibbi:/home/jhay/ping6 #
Comment 15 Hiroki Sato freebsd_committer 2014-10-10 03:29:28 UTC
(In reply to John Hay from comment #14)
> But -x seems to add 1 second to the -x millisecond value or do I
> misunderstand it?

 Correct.  ping(8) uses second for -i and millisecond for -W, so
 I used the same units for -x and -X.
Comment 16 Andrey V. Elsukov freebsd_committer 2014-12-22 14:34:55 UTC
Hi,

Note that r269180 still not reverted in stable/* branches.
Comment 17 Glen Barber freebsd_committer 2015-07-08 18:32:03 UTC
To originators/assignees of this PR:

A commit to the tree references this PR, however the PR is still in a non-closed state.

Please review this PR and close as appropriate, or if closing the PR requires a merge to stable/10, please let re@ know as soon as possible.

Thank you.

Glen
Comment 18 Enji Cooper freebsd_committer 2015-12-20 21:24:18 UTC
(In reply to Andrey V. Elsukov from comment #16)

Yes, it's reverted on stable/10, but not stable/8 and stable/9 :/..:

------------------------------------------------------------------------
r272871 | hrs | 2014-10-09 16:48:20 -0700 (Thu, 09 Oct 2014) | 4 lines

MFC r271909:
  Revert changes in r269180.  It could cause -c N option to enter an
  infinite loop if no reply packet is received.
Comment 19 Eitan Adler freebsd_committer freebsd_triage 2018-05-23 10:27:24 UTC
batch change of PRs untouched in 2018 marked "in progress" back to open.
Comment 20 Mike Appleby 2018-08-17 18:37:02 UTC
Can this PR be closed? If not, what work remains?

It seems the proposed patches have landed in the following commits:

head      base r273211, base r271910
stable/11 base r273211, base r271910
stable/10 base r285820

Regarding comment #18 and comment #16, above: the broken patch that was committed in base r269180 has been reverted in the following commits.

head      base r271909
stable/11 base r271909
stable/10 base r272871
stable/9  base r298843

As far as I can tell, stable/8 was the only stable branch where the broken patch was merged (in base r269803), but never reverted.