ping(8): better "-i wait" edge case handling

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

ping(8): better "-i wait" edge case handling

Scott Cheloha
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?

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.

Reply | Threaded
Open this post in threaded view
|

Re: ping(8): better "-i wait" edge case handling

Claudio Jeker
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

Reply | Threaded
Open this post in threaded view
|

Re: ping(8): better "-i wait" edge case handling

Mark Kettenis
> Date: Sun, 21 Jul 2019 10:50:27 +0200
> From: Claudio Jeker <[hidden email]>
>
> 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
>
>

Reply | Threaded
Open this post in threaded view
|

Re: ping(8): better "-i wait" edge case handling

Scott Cheloha
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 <[hidden email]>
> >
> > 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.

Reply | Threaded
Open this post in threaded view
|

Re: ping(8): better "-i wait" edge case handling

Klemens Nanni-2
On Sun, Jul 21, 2019 at 08:30:23AM -0500, Scott Cheloha wrote:
> 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.
Looks good.

> @@ -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) {
You can check this earlier against integral directly and avoid handling
`interval' in the non-root case.

> + errx(1, "only root may use an interval smaller"
> +    "than one second");
> + }
>   options |= F_INTERVAL;
>   break;
>   case 'L':

> @@ -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.
I think the old "between sending each packet" still reads better.

> +The default is one second.
> +The
> +.Ar interval
> +may contain a fractional portion.
> +Only the superuser may specify a value less than one second.
Here, the old sentence combining both statements in one sentence reads
better to me, but no objections.

>  This option is incompatible with the
>  .Fl f
>  option.
>

Reply | Threaded
Open this post in threaded view
|

Re: ping(8): better "-i wait" edge case handling

Claudio Jeker
In reply to this post by Scott Cheloha
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 <[hidden email]>
> > >
> > > 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

Reply | Threaded
Open this post in threaded view
|

Re: ping(8): better "-i wait" edge case handling

Scott Cheloha
> On Jul 22, 2019, at 01:49, Claudio Jeker <[hidden email]> wrote:

>
>> 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 <[hidden email]>
>>>>
>>>>> 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()?

You mean the ERANGE thing?  That isn't useful here.  The
normal range for a double is far more vast than what a timeval
can represent.  We have to check by hand.