Re: systat(1): improve parsing of delay value
On Thu, Jan 28, 2021 at 09:06:51PM +0100, Martijn van Duren wrote:
> Thanks for checking. Should be fixed below.
OK bluhm@
> Index: main.c
> ===
> RCS file: /cvs/src/usr.bin/systat/main.c,v
> retrieving revision 1.72
> diff -u -p -r1.72 main.c
> --- main.c12 Jan 2020 20:51:08 - 1.72
> +++ main.c28 Jan 2021 20:05:30 -
> @@ -40,9 +40,11 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -73,6 +75,7 @@ charuloadbuf[TIMEPOS];
>
> int ucount(void);
> void usage(void);
> +double strtodnum(const char *, double, double, const char **);
>
> /* command prompt */
>
> @@ -323,9 +326,14 @@ void
> cmd_delay(const char *buf)
> {
> double del;
> - del = atof(buf);
> + const char *errstr;
>
> - if (del > 0) {
> + if (buf[0] == '\0')
> + return;
> + del = strtodnum(buf, 0, UINT32_MAX / 100, );
> + if (errstr != NULL)
> + error("s: \"%s\": delay value is %s", buf, errstr);
> + else {
> udelay = (useconds_t)(del * 100);
> gotsig_alarm = 1;
> naptime = del;
> @@ -414,6 +422,48 @@ gethz(void)
> hz = cinf.hz;
> }
>
> +#define INVALID 1
> +#define TOOSMALL2
> +#define TOOLARGE3
> +
> +double
> +strtodnum(const char *nptr, double minval, double maxval, const char
> **errstrp)
> +{
> + double d = 0;
> + int error = 0;
> + char *ep;
> + struct errval {
> + const char *errstr;
> + int err;
> + } ev[4] = {
> + { NULL, 0 },
> + { "invalid",EINVAL },
> + { "too small", ERANGE },
> + { "too large", ERANGE },
> + };
> +
> + ev[0].err = errno;
> + errno = 0;
> + if (minval > maxval) {
> + error = INVALID;
> + } else {
> + d = strtod(nptr, );
> + if (nptr == ep || *ep != '\0')
> + error = INVALID;
> + else if ((d == -HUGE_VAL && errno == ERANGE) || d < minval)
> + error = TOOSMALL;
> + else if ((d == HUGE_VAL && errno == ERANGE) || d > maxval)
> + error = TOOLARGE;
> + }
> + if (errstrp != NULL)
> + *errstrp = ev[error].errstr;
> + errno = ev[error].err;
> + if (error)
> + d = 0;
> +
> + return (d);
> +}
> +
> int
> main(int argc, char *argv[])
> {
> @@ -421,7 +471,7 @@ main(int argc, char *argv[])
> const char *errstr;
> extern char *optarg;
> extern int optind;
> - double delay = 5;
> + double delay = 5, del;
>
> char *viewstr = NULL;
>
> @@ -475,9 +525,11 @@ main(int argc, char *argv[])
> nflag = 1;
> break;
> case 's':
> - delay = atof(optarg);
> - if (delay <= 0)
> - delay = 5;
> + delay = strtodnum(optarg, 0, UINT32_MAX / 100,
> + );
> + if (errstr != NULL)
> + errx(1, "-s \"%s\": delay value is %s", optarg,
> + errstr);
> break;
> case 'w':
> rawwidth = strtonum(optarg, 1, MAX_LINE_BUF-1, );
> @@ -497,16 +549,16 @@ main(int argc, char *argv[])
> argv += optind;
>
> if (argc == 1) {
> - double del = atof(argv[0]);
> - if (del == 0)
> + del = strtodnum(argv[0], 0, UINT32_MAX / 100, );
> + if (errstr != NULL)
> viewstr = argv[0];
> else
> delay = del;
> } else if (argc == 2) {
> viewstr = argv[0];
> - delay = atof(argv[1]);
> - if (delay <= 0)
> - delay = 5;
> + delay = strtodnum(argv[1], 0, UINT32_MAX / 100, );
> + if (errstr != NULL)
> + errx(1, "\"%s\": delay value is %s", argv[1], errstr);
> }
>
> udelay = (useconds_t)(delay * 100.0);
>
Re: systat(1): improve parsing of delay value
On Tue, 2021-01-26 at 16:40 +0100, Alexander Bluhm wrote:
> On Mon, Jan 25, 2021 at 11:17:04AM +0100, Martijn van Duren wrote:
> > if (argc == 1) {
> > - double del = atof(argv[0]);
> > - if (del == 0)
> > + delay = strtodnum(argv[0], 0, UINT32_MAX / 100,
> > );
> > + if (errstr != NULL)
> > viewstr = argv[0];
> > - else
> > - delay = del;
>
> You need the else. delay should only be changed if parsing was successful.
>
> > } else if (argc == 2) {
> > viewstr = argv[0];
> > - delay = atof(argv[1]);
> > - if (delay <= 0)
> > - delay = 5;
> > + delay = strtodnum(optarg, 0, UINT32_MAX / 100, );
>
> This should be argv[1] instead of optarg.
>
> > + if (errstr != NULL)
> > + errx(1, "-s \"%s\": delay value is %s", optarg,
> > errstr);
> > }
>
> The -s in the error message is wrong. Here delay was passed as argument.
>
> bluhm
>
Thanks for checking. Should be fixed below.
Index: main.c
===
RCS file: /cvs/src/usr.bin/systat/main.c,v
retrieving revision 1.72
diff -u -p -r1.72 main.c
--- main.c 12 Jan 2020 20:51:08 - 1.72
+++ main.c 28 Jan 2021 20:05:30 -
@@ -40,9 +40,11 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include
#include
#include
@@ -73,6 +75,7 @@ char uloadbuf[TIMEPOS];
int ucount(void);
void usage(void);
+double strtodnum(const char *, double, double, const char **);
/* command prompt */
@@ -323,9 +326,14 @@ void
cmd_delay(const char *buf)
{
double del;
- del = atof(buf);
+ const char *errstr;
- if (del > 0) {
+ if (buf[0] == '\0')
+ return;
+ del = strtodnum(buf, 0, UINT32_MAX / 100, );
+ if (errstr != NULL)
+ error("s: \"%s\": delay value is %s", buf, errstr);
+ else {
udelay = (useconds_t)(del * 100);
gotsig_alarm = 1;
naptime = del;
@@ -414,6 +422,48 @@ gethz(void)
hz = cinf.hz;
}
+#defineINVALID 1
+#defineTOOSMALL2
+#defineTOOLARGE3
+
+double
+strtodnum(const char *nptr, double minval, double maxval, const char **errstrp)
+{
+ double d = 0;
+ int error = 0;
+ char *ep;
+ struct errval {
+ const char *errstr;
+ int err;
+ } ev[4] = {
+ { NULL, 0 },
+ { "invalid",EINVAL },
+ { "too small", ERANGE },
+ { "too large", ERANGE },
+ };
+
+ ev[0].err = errno;
+ errno = 0;
+ if (minval > maxval) {
+ error = INVALID;
+ } else {
+ d = strtod(nptr, );
+ if (nptr == ep || *ep != '\0')
+ error = INVALID;
+ else if ((d == -HUGE_VAL && errno == ERANGE) || d < minval)
+ error = TOOSMALL;
+ else if ((d == HUGE_VAL && errno == ERANGE) || d > maxval)
+ error = TOOLARGE;
+ }
+ if (errstrp != NULL)
+ *errstrp = ev[error].errstr;
+ errno = ev[error].err;
+ if (error)
+ d = 0;
+
+ return (d);
+}
+
int
main(int argc, char *argv[])
{
@@ -421,7 +471,7 @@ main(int argc, char *argv[])
const char *errstr;
extern char *optarg;
extern int optind;
- double delay = 5;
+ double delay = 5, del;
char *viewstr = NULL;
@@ -475,9 +525,11 @@ main(int argc, char *argv[])
nflag = 1;
break;
case 's':
- delay = atof(optarg);
- if (delay <= 0)
- delay = 5;
+ delay = strtodnum(optarg, 0, UINT32_MAX / 100,
+ );
+ if (errstr != NULL)
+ errx(1, "-s \"%s\": delay value is %s", optarg,
+ errstr);
break;
case 'w':
rawwidth = strtonum(optarg, 1, MAX_LINE_BUF-1, );
@@ -497,16 +549,16 @@ main(int argc, char *argv[])
argv += optind;
if (argc == 1) {
- double del = atof(argv[0]);
- if (del == 0)
+ del = strtodnum(argv[0], 0, UINT32_MAX / 100, );
+ if (errstr != NULL)
viewstr = argv[0];
else
delay = del;
} else if (argc == 2) {
viewstr = argv[0];
- delay = atof(argv[1]);
- if (delay <= 0)
- delay = 5;
+
Re: systat(1): improve parsing of delay value
On Mon, Jan 25, 2021 at 11:17:04AM +0100, Martijn van Duren wrote:
> if (argc == 1) {
> - double del = atof(argv[0]);
> - if (del == 0)
> + delay = strtodnum(argv[0], 0, UINT32_MAX / 100, );
> + if (errstr != NULL)
> viewstr = argv[0];
> - else
> - delay = del;
You need the else. delay should only be changed if parsing was successful.
> } else if (argc == 2) {
> viewstr = argv[0];
> - delay = atof(argv[1]);
> - if (delay <= 0)
> - delay = 5;
> + delay = strtodnum(optarg, 0, UINT32_MAX / 100, );
This should be argv[1] instead of optarg.
> + if (errstr != NULL)
> + errx(1, "-s \"%s\": delay value is %s", optarg, errstr);
> }
The -s in the error message is wrong. Here delay was passed as argument.
bluhm
Re: systat(1): improve parsing of delay value
On Mon, 2021-01-25 at 10:50 +0100, Martijn van Duren wrote:
> On Mon, 2021-01-25 at 08:15 +, Nick Gasson wrote:
> > Hi,
> >
> > I incorrectly ran "systat netstat -N" instead of "systat -N netstat" and
> > got confused why it wasn't resolving host names. The -N gets parsed with
> > atof to a 0s delay that is then clamped to 5s. The patch below instead
> > prints an error if the delay cannot be parsed. I think the <= 0 case
> > should also produce an error but I left the existing behaviour of
> > setting it to 5s.
>
> Your diff fails for the simple case of "systat", because of your added
> else case. It's also not completely clear to the end user why things
> fail.
>
> I agree that failing loud on unparseable strings and just defaulting
> back to the 5s is just stupid. So here's an updated diff that just
> blatantly steals^Wborrows from strtonum.
>
> OK?
>
> martijn@
>
That one was a little too rushed... It overlooked the internal "s"
command and some minor additional testing shows that currently
large delays are accepted and completely broken because of useconds_t
overflow. I couldn't find something like USECONDS_MAX, so I took the
next best thing: UINT32_MAX, where useconds_t is based on.
This change limits delays to 4294 seconds, which already is the
only reasonable value that would work for systat.
martijn@
Index: main.c
===
RCS file: /cvs/src/usr.bin/systat/main.c,v
retrieving revision 1.72
diff -u -p -r1.72 main.c
--- main.c 12 Jan 2020 20:51:08 - 1.72
+++ main.c 25 Jan 2021 10:16:34 -
@@ -40,9 +40,11 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include
#include
#include
@@ -73,6 +75,7 @@ char uloadbuf[TIMEPOS];
int ucount(void);
void usage(void);
+double strtodnum(const char *, double, double, const char **);
/* command prompt */
@@ -323,9 +326,14 @@ void
cmd_delay(const char *buf)
{
double del;
- del = atof(buf);
+ const char *errstr;
- if (del > 0) {
+ if (buf[0] == '\0')
+ return;
+ del = strtodnum(buf, 0, UINT32_MAX / 100, );
+ if (errstr != NULL)
+ error("s: \"%s\": delay value is %s", buf, errstr);
+ else {
udelay = (useconds_t)(del * 100);
gotsig_alarm = 1;
naptime = del;
@@ -414,6 +422,48 @@ gethz(void)
hz = cinf.hz;
}
+#defineINVALID 1
+#defineTOOSMALL2
+#defineTOOLARGE3
+
+double
+strtodnum(const char *nptr, double minval, double maxval, const char **errstrp)
+{
+ double d = 0;
+ int error = 0;
+ char *ep;
+ struct errval {
+ const char *errstr;
+ int err;
+ } ev[4] = {
+ { NULL, 0 },
+ { "invalid",EINVAL },
+ { "too small", ERANGE },
+ { "too large", ERANGE },
+ };
+
+ ev[0].err = errno;
+ errno = 0;
+ if (minval > maxval) {
+ error = INVALID;
+ } else {
+ d = strtod(nptr, );
+ if (nptr == ep || *ep != '\0')
+ error = INVALID;
+ else if ((d == -HUGE_VAL && errno == ERANGE) || d < minval)
+ error = TOOSMALL;
+ else if ((d == HUGE_VAL && errno == ERANGE) || d > maxval)
+ error = TOOLARGE;
+ }
+ if (errstrp != NULL)
+ *errstrp = ev[error].errstr;
+ errno = ev[error].err;
+ if (error)
+ d = 0;
+
+ return (d);
+}
+
int
main(int argc, char *argv[])
{
@@ -475,9 +525,11 @@ main(int argc, char *argv[])
nflag = 1;
break;
case 's':
- delay = atof(optarg);
- if (delay <= 0)
- delay = 5;
+ delay = strtodnum(optarg, 0, UINT32_MAX / 100,
+ );
+ if (errstr != NULL)
+ errx(1, "-s \"%s\": delay value is %s", optarg,
+ errstr);
break;
case 'w':
rawwidth = strtonum(optarg, 1, MAX_LINE_BUF-1, );
@@ -497,16 +549,14 @@ main(int argc, char *argv[])
argv += optind;
if (argc == 1) {
- double del = atof(argv[0]);
- if (del == 0)
+ delay = strtodnum(argv[0], 0, UINT32_MAX / 100, );
+ if (errstr != NULL)
viewstr = argv[0];
- else
- delay = del;
} else if (argc == 2) {
viewstr = argv[0];
- delay = atof(argv[1]);
- if (delay <= 0)
- delay = 5;
+ delay =
Re: systat(1): improve parsing of delay value
On Mon, 2021-01-25 at 08:15 +, Nick Gasson wrote:
> Hi,
>
> I incorrectly ran "systat netstat -N" instead of "systat -N netstat" and
> got confused why it wasn't resolving host names. The -N gets parsed with
> atof to a 0s delay that is then clamped to 5s. The patch below instead
> prints an error if the delay cannot be parsed. I think the <= 0 case
> should also produce an error but I left the existing behaviour of
> setting it to 5s.
Your diff fails for the simple case of "systat", because of your added
else case. It's also not completely clear to the end user why things
fail.
I agree that failing loud on unparseable strings and just defaulting
back to the 5s is just stupid. So here's an updated diff that just
blatantly steals^Wborrows from strtonum.
OK?
martijn@
Index: main.c
===
RCS file: /cvs/src/usr.bin/systat/main.c,v
retrieving revision 1.72
diff -u -p -r1.72 main.c
--- main.c 12 Jan 2020 20:51:08 - 1.72
+++ main.c 25 Jan 2021 09:48:44 -
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -414,6 +415,48 @@ gethz(void)
hz = cinf.hz;
}
+#defineINVALID 1
+#defineTOOSMALL2
+#defineTOOLARGE3
+
+double
+strtodnum(const char *nptr, double minval, double maxval, const char **errstrp)
+{
+ double d = 0;
+ int error = 0;
+ char *ep;
+ struct errval {
+ const char *errstr;
+ int err;
+ } ev[4] = {
+ { NULL, 0 },
+ { "invalid",EINVAL },
+ { "too small", ERANGE },
+ { "too large", ERANGE },
+ };
+
+ ev[0].err = errno;
+ errno = 0;
+ if (minval > maxval) {
+ error = INVALID;
+ } else {
+ d = strtod(nptr, );
+ if (nptr == ep || *ep != '\0')
+ error = INVALID;
+ else if ((d == -HUGE_VAL && errno == ERANGE) || d < minval)
+ error = TOOSMALL;
+ else if ((d == HUGE_VAL && errno == ERANGE) || d > maxval)
+ error = TOOLARGE;
+ }
+ if (errstrp != NULL)
+ *errstrp = ev[error].errstr;
+ errno = ev[error].err;
+ if (error)
+ d = 0;
+
+ return (d);
+}
+
int
main(int argc, char *argv[])
{
@@ -475,9 +518,9 @@ main(int argc, char *argv[])
nflag = 1;
break;
case 's':
- delay = atof(optarg);
- if (delay <= 0)
- delay = 5;
+ delay = strtodnum(optarg, 0, HUGE_VAL, );
+ if (errstr != NULL)
+ errx(1, "-s %s: delay value is %s", optarg,
errstr);
break;
case 'w':
rawwidth = strtonum(optarg, 1, MAX_LINE_BUF-1, );
@@ -497,16 +540,14 @@ main(int argc, char *argv[])
argv += optind;
if (argc == 1) {
- double del = atof(argv[0]);
- if (del == 0)
+ delay = strtodnum(argv[0], 0, HUGE_VAL, );
+ if (errstr != NULL)
viewstr = argv[0];
- else
- delay = del;
} else if (argc == 2) {
viewstr = argv[0];
- delay = atof(argv[1]);
- if (delay <= 0)
- delay = 5;
+ delay = strtodnum(optarg, 0, HUGE_VAL, );
+ if (errstr != NULL)
+ errx(1, "-s %s: delay value is %s", optarg, errstr);
}
udelay = (useconds_t)(delay * 100.0);
systat(1): improve parsing of delay value
Hi,
I incorrectly ran "systat netstat -N" instead of "systat -N netstat" and
got confused why it wasn't resolving host names. The -N gets parsed with
atof to a 0s delay that is then clamped to 5s. The patch below instead
prints an error if the delay cannot be parsed. I think the <= 0 case
should also produce an error but I left the existing behaviour of
setting it to 5s.
Index: usr.bin/systat/main.c
===
RCS file: /cvs/src/usr.bin/systat/main.c,v
retrieving revision 1.72
diff -u -p -u -p -r1.72 main.c
--- usr.bin/systat/main.c 12 Jan 2020 20:51:08 - 1.72
+++ usr.bin/systat/main.c 25 Jan 2021 08:07:34 -
@@ -415,6 +415,22 @@ gethz(void)
}
int
+parse_delay(const char *str, double *delay)
+{
+ char *endptr = NULL;
+ double value;
+
+ value = strtod(str, );
+ if (*endptr == '\0') {
+ if (value <= 0)
+ value = 5;
+ *delay = value;
+ return 0;
+ } else
+ return 1;
+}
+
+int
main(int argc, char *argv[])
{
char errbuf[_POSIX2_LINE_MAX];
@@ -475,9 +491,8 @@ main(int argc, char *argv[])
nflag = 1;
break;
case 's':
- delay = atof(optarg);
- if (delay <= 0)
- delay = 5;
+ if (parse_delay(optarg, ))
+ errx(1, "-s %s: invalid delay value", optarg);
break;
case 'w':
rawwidth = strtonum(optarg, 1, MAX_LINE_BUF-1, );
@@ -497,16 +512,14 @@ main(int argc, char *argv[])
argv += optind;
if (argc == 1) {
- double del = atof(argv[0]);
- if (del == 0)
+ if (parse_delay(argv[0], ))
viewstr = argv[0];
- else
- delay = del;
} else if (argc == 2) {
viewstr = argv[0];
- delay = atof(argv[1]);
- if (delay <= 0)
- delay = 5;
+ if (parse_delay(argv[1], ))
+ errx(1, "invalid delay value: %s", argv[1]);
+ } else {
+ usage();
}
udelay = (useconds_t)(delay * 100.0);
