On Sun, Jul 21, 2019 at 11:49:10AM +0200, Mark Kettenis wrote:
> > Date: Sun, 21 Jul 2019 10:50:27 +0200
> > From: Claudio Jeker <cje...@diehard.n-r-g.com>
> >
> > On Sat, Jul 20, 2019 at 08:12:40AM -0500, Scott Cheloha wrote:
> > > There are cases for ping(8)'s "-i wait" option that we don't handle
> > > correctly.
> > >
> > > Negative values smaller than -1:
> > >
> > > $ doas ping -i -0.1 localhost
> > > [no error]
> > >
> > > Positive values less than one microsecond:
> > >
> > > $ doas ping -i 0.0000001 localhost
> > > [no error]
> > >
> > > Large positive values that actually fit into a timeval:
> > >
> > > $ ping -i $(bc -e '2^63 - 1' -e quit) localhost
> > > ping: illegal timing interval 9223372036854775807
> > >
> > > The first two cases are bugs in the input checking. The latter case
> > > is a limitation of IEEE doubles: with only 52 bits of significand you
> > > can't represent the full range of a timeval on a platform with 64-bit
> > > time_t.
> > >
> > > This patch addresses the first two cases with better error checking.
> > > It also tries to handle the latter case a bit better by using IEEE
> > > quads, i.e. long doubles. With 64 bits of significand you can cover
> > > our time_t and the above case works. It isn't ~perfect~, but it's as
> > > close as we can get to perfect without parsing the number by hand as
> > > we do in sleep(1) (cf. bin/sleep/sleep.c).
> > >
> > > With this patch those cases all work as expected:
> > >
> > > $ doas ping -i -0.1 localhost
> > > ping: interval is invalid: -0.1
> > > $ doas ping -i 0.0000001 localhost
> > > ping: interval is too small: 0.0000001
> > > $ ping -i $(bc -e '2^63' -e quit) localhost
> > > ping: interval is too large: 9223372036854775808
> > > $ ping -i $(bc -e '2^63 - 1' -e quit) localhost
> > > PING localhost.local (23.195.69.106): 56 data bytes
> > > 64 bytes from 23.195.69.106: icmp_seq=0 ttl=54 time=18.001 ms
> > > [stalls forever]
> > >
> > > --
> > >
> > > While we're modifying this code, I think "-i interval" is better than
> > > "-i wait". The "i for interval" mnemonic is particularly nice. The
> > > current "wait" argument name sort-of implies a relationship with the
> > > "-w maxwait" option, which is not the case. We're also already using
> > > "timing interval" in these error messages here. I've tweaked the
> > > error messages to look more like the usual strtonum(3) error messages.
> > >
> > > ok?
> >
> > Can't we limit the -i maximum value to something that fits into the double
> > instead of using long double in ping. Nobody will ever try to using ping
> > with a timeout that is longer than the operators expected life time.
Sure. Here's a diff that uses UINT_MAX. The oldest known person
lived to 123. French, if I recall correctly. UINT_MAX gives us 136
years, so we have a bit of room to grow just in case someone beats the
record. And 52 bits of significand is plenty for 32 bits of integral
plus a fractional portion.
> Using a long double isn't a solution anyway, since we have quite a few
> architectures where long double is the same as double.
Oh, hmmm, didn't know that.
--
this diff better?
Index: ping.c
===================================================================
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.237
diff -u -p -r1.237 ping.c
--- ping.c 20 Jul 2019 00:49:54 -0000 1.237
+++ ping.c 21 Jul 2019 13:23:55 -0000
@@ -258,7 +258,7 @@ main(int argc, char *argv[])
char *e, *target, hbuf[NI_MAXHOST], *source = NULL;
char rspace[3 + 4 * NROUTES + 1]; /* record route space */
const char *errstr;
- double intval;
+ double fraction, integral, seconds;
uid_t ouid, uid;
gid_t gid;
u_int rtableid = 0;
@@ -332,17 +332,21 @@ main(int argc, char *argv[])
case 'S': /* deprecated */
source = optarg;
break;
- case 'i': /* wait between sending packets */
- intval = strtod(optarg, &e);
- if (*optarg == '\0' || *e != '\0')
- errx(1, "illegal timing interval %s", optarg);
- if (intval < 1 && ouid)
- errx(1, "only root may use interval < 1s");
- interval.tv_sec = (time_t)intval;
- interval.tv_usec =
- (long)((intval - interval.tv_sec) * 1000000);
- if (interval.tv_sec < 0)
- errx(1, "illegal timing interval %s", optarg);
+ case 'i': /* interval between packets */
+ seconds = strtod(optarg, &e);
+ if (*optarg == '\0' || *e != '\0' || seconds < 0.0)
+ errx(1, "interval is invalid: %s", optarg);
+ fraction = modf(seconds, &integral);
+ if (integral > UINT_MAX)
+ errx(1, "interval is too large: %s", optarg);
+ interval.tv_sec = integral;
+ interval.tv_usec = fraction * 1000000.0;
+ if (!timerisset(&interval))
+ errx(1, "interval is too small: %s", optarg);
+ if (interval.tv_sec < 1 && ouid != 0) {
+ errx(1, "only root may use an interval smaller"
+ "than one second");
+ }
options |= F_INTERVAL;
break;
case 'L':
@@ -2176,13 +2180,13 @@ usage(void)
if (v6flag) {
fprintf(stderr,
"usage: ping6 [-DdEefHLmnqv] [-c count] [-h hoplimit] "
- "[-I sourceaddr]\n\t[-i wait] [-l preload] [-p pattern] "
- "[-s packetsize] [-T toskeyword]\n\t"
- "[-V rtable] [-w maxwait] host\n");
+ "[-I sourceaddr]\n\t[-i interval] [-l preload] "
+ "[-p pattern] [-s packetsize] [-T toskeyword]\n"
+ "\t[-V rtable] [-w maxwait] host\n");
} else {
fprintf(stderr,
- "usage: ping [-DdEefHLnqRv] [-c count] [-I ifaddr]"
- " [-i wait]\n\t[-l preload] [-p pattern] [-s packetsize]"
+ "usage: ping [-DdEefHLnqRv] [-c count] [-I ifaddr] "
+ "[-i interval]\n\t[-l preload] [-p pattern] [-s packetsize]"
#ifndef SMALL
" [-T toskeyword]"
#endif /* SMALL */
Index: ping.8
===================================================================
RCS file: /cvs/src/sbin/ping/ping.8,v
retrieving revision 1.61
diff -u -p -r1.61 ping.8
--- ping.8 10 Nov 2018 23:44:53 -0000 1.61
+++ ping.8 21 Jul 2019 13:23:55 -0000
@@ -69,7 +69,7 @@
.Op Fl DdEefHLnqRv
.Op Fl c Ar count
.Op Fl I Ar ifaddr
-.Op Fl i Ar wait
+.Op Fl i Ar interval
.Op Fl l Ar preload
.Op Fl p Ar pattern
.Op Fl s Ar packetsize
@@ -83,7 +83,7 @@
.Op Fl c Ar count
.Op Fl h Ar hoplimit
.Op Fl I Ar sourceaddr
-.Op Fl i Ar wait
+.Op Fl i Ar interval
.Op Fl l Ar preload
.Op Fl p Ar pattern
.Op Fl s Ar packetsize
@@ -160,13 +160,15 @@ Set the hoplimit.
Specify the interface address to transmit from
on machines with multiple interfaces.
For unicast and multicast pings.
-.It Fl i Ar wait
-Wait
-.Ar wait
-seconds between sending each packet.
-The default is to wait for one second between each packet.
-The wait time may be fractional, but only the superuser may specify
-a value less than one second.
+.It Fl i Ar interval
+Wait at least
+.Ar interval
+seconds between each outgoing packet.
+The default is one second.
+The
+.Ar interval
+may contain a fractional portion.
+Only the superuser may specify a value less than one second.
This option is incompatible with the
.Fl f
option.
