[PATCH] tools/xentop: reject invalid --delay and --iterations arguments

[Matthias Goergens <matthias.goergens@xxxxxxxxx>]

xentop parses -d/--delay and -i/--iterations with atoi(), so invalid
input is silently accepted with surprising results: a fractional delay
such as "-d 2.5" is truncated to 2; "-d abc" parses as 0, which in
batch mode turns the output loop into a busy loop; "-d -1" wraps to an
effective delay of about 136 years; and "-i 0" (or any unparsable
iterations count) decrements an unsigned counter from zero, running
for about 2^32 iterations.  atoi() also has undefined behaviour on
out-of-range input.

Parse both options with strtoull() instead, and reject anything that
is not a plain decimal integer in range: a sign, a fractional part,
trailing junk or overflow now produce an error and exit rather than a
silently wrong value.

Compatibility considerations: "--delay 0" remains accepted, since
updating as fast as possible is a plausible deliberate choice and
works today; "--iterations 0" is rejected, since running the loop
2^32 times cannot be what the caller meant.  The interactive 'D'
prompt already validates its input and is unchanged.  The only
previously useful invocation this breaks is a fractional delay, which
now fails loudly instead of silently rounding down - which is the
point of the change.

A patch documenting the --delay truncation was posted in 2010 but
never applied:
Link: 
https://lore.kernel.org/xen-devel/01ea26d2420e3562eb30.1292604768@xxxxxxxxxxxxxxxxxxxxxxxxxx/

Signed-off-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>
---
Tested by compiling with -Wall -Wextra (no new warnings) and by running
the parse helper, extracted verbatim from the patched file, against a
19-case input matrix covering both the accepted and the rejected inputs
listed above.  Not run against a live Xen host: the change is confined
to command line parsing, ahead of any hypervisor interaction.

Happy to add a CHANGELOG.md entry under "Changed" if that is wanted for
a tools CLI change of this size.

 docs/man/xentop.1.pod |  5 +++--
 tools/xentop/xentop.c | 27 +++++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/docs/man/xentop.1.pod b/docs/man/xentop.1.pod
index db64ceb..13f3f13 100644
--- a/docs/man/xentop.1.pod
+++ b/docs/man/xentop.1.pod
@@ -27,7 +27,7 @@ output version information and exit
 
 =item B<-d>, B<--delay>=I<SECONDS>
 
-seconds between updates (default 3)
+seconds between updates (default 3); must be a non-negative integer
 
 =item B<-n>, B<--networks>
 
@@ -55,7 +55,8 @@ output data in batch mode (to stdout)
 
 =item B<-i>, B<--iterations>=I<ITERATIONS>
 
-maximum number of iterations xentop should produce before ending
+maximum number of iterations xentop should produce before ending; must
+be a positive integer
 
 =item B<-z>, B<--dom0-first>
 
diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
index addb1c7..c7fb4ca 100644
--- a/tools/xentop/xentop.c
+++ b/tools/xentop/xentop.c
@@ -23,6 +23,7 @@
 
 #include <ctype.h>
 #include <errno.h>
+#include <limits.h>
 #include <math.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -1297,6 +1298,28 @@ static void signal_exit_handler(int sig)
        signal_exit = 1;
 }
 
+/* Parse a numeric command line argument as a plain decimal integer no
+ * smaller than min_val.  Anything else - a sign, a fractional part,
+ * trailing junk, overflow - is fatal, rather than being silently
+ * accepted as a wrong value the way atoi() would.
+ */
+static unsigned int parse_uint_arg(const char *name, const char *arg,
+                                  unsigned int min_val)
+{
+       unsigned long long val;
+       char *end;
+
+       errno = 0;
+       if (isdigit((unsigned char)arg[0])) {
+               val = strtoull(arg, &end, 10);
+               if (!errno && !*end && val >= min_val && val <= UINT_MAX)
+                       return val;
+       }
+       fprintf(stderr, "xentop: invalid %s argument '%s': expected a %s 
decimal integer\n",
+               name, arg, min_val ? "positive" : "non-negative");
+       exit(1);
+}
+
 int main(int argc, char **argv)
 {
        int opt, optind = 0;
@@ -1347,7 +1370,7 @@ int main(int argc, char **argv)
                        show_vcpus = 1;
                        break;
                case 'd':
-                       delay = atoi(optarg);
+                       delay = parse_uint_arg("--delay", optarg, 0);
                        break;
                case 'b':
                        batch = 1;
@@ -1356,7 +1379,7 @@ int main(int argc, char **argv)
                        show_pcpus = 1;
                        break;
                case 'i':
-                       iterations = atoi(optarg);
+                       iterations = parse_uint_arg("--iterations", optarg, 1);
                        loop = 0;
                        break;
                case 'f':
-- 
2.55.0