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.
> 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
