On Sun, Jul 21, 2019 at 08:30:23AM -0500, Scott Cheloha wrote:
> 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 <[email protected]>
> > >
> > > 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?
Can't this just use the overflow detection of strtod()?
> 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.
>
--
:wq Claudio
