> 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.
Using a long double isn't a solution anyway, since we have quite a few
architectures where long double is the same as double.
> > 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 20 Jul 2019 12:46:46 -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;
> > + long double dec, frac, 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 = strtold(optarg, &e);
> > + if (*optarg == '\0' || *e != '\0' || seconds < 0.0)
> > + errx(1, "interval is invalid: %s", optarg);
> > + frac = modfl(seconds, &dec);
> > + if (dec > LLONG_MAX)
> > + errx(1, "interval is too large: %s", optarg);
> > + interval.tv_sec = dec;
> > + interval.tv_usec = frac * 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 20 Jul 2019 12:46:46 -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.
> >
>
> --
> :wq Claudio
>
>
