tools/misc: fix xenwatchdogd invocation

Commit Message

zithro / Cyril Rébert March 26, 2024, 5:15 a.m. UTC

When xenwatchdogd is invoked with -h/--help (or any non-number), the
argument value is converted to 0, which immediately shutdowns dom0.

So make sure only numbers are passed to the program, otherwise fail.
For the help, use getopt_long as suggested.
While there, reformat the code a bit (s/tabs/spaces/, indentation, etc).

Bug fix only, no functional change intended.

Reported-by: Leigh Brown <[e-mail]>
Suggested-by: Andrew Cooper <[e-mail]>
Signed-off-by: Cyril Rébert / zithro <[e-mail]>
---
- Not sure about the preprocessor stanzas, copied them from xentop.
- A small explanation about what the program does could be helpful,
  like some kind of synopsis ? Purpose, gotchas, etc. I can do the
  writing, but please be specific !
- Built on 4.17 and unstable, tested on 4.17.
---
 tools/misc/xenwatchdogd.c | 77 +++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 20 deletions(-)

Comments

Jan Beulich March 26, 2024, 8:19 a.m. UTC | #1

On 26.03.2024 06:15, zithro / Cyril Rébert wrote:
> --- a/tools/misc/xenwatchdogd.c
> +++ b/tools/misc/xenwatchdogd.c
> @@ -8,26 +8,34 @@
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <signal.h>
> +#include <string.h>
>  #include <stdio.h>
>  
> +#define _GNU_SOURCE

This wants defining first thing in a file (or even on the compiler command
line), to (properly) affect all headers.

> +#include <getopt.h>
> +#if !defined(__GNUC__) && !defined(__GNUG__)

Why __GNUG__ in a C file? And anyway, why is this construct needed? You
don't ...

> +#define __attribute__(arg) /* empty */

... use any attributes down from here.

You mention xentop as where you've taken them from. I view those as
questionable. If in fact you had used any other utility's source for
reference, you wouldn't have encountered such.

> +#endif
> +
>  xc_interface *h;
>  int id = 0;
>  
>  void daemonize(void)
>  {
>      switch (fork()) {
> -    case -1:
> -	err(1, "fork");
> -    case 0:
> -	break;
> -    default:
> -	exit(0);
> +        case -1:
> +            err(1, "fork");
> +        case 0:
> +            break;
> +        default:
> +            exit(0);
>      }

The case labels were properly indented, weren't they? And all other re-
indentation you do wants splitting from the functional change.

> @@ -52,39 +60,68 @@ void catch_usr1(int sig)
>  
>  int main(int argc, char **argv)
>  {
> +
>      int t, s;

What would this new blank line be good for?

>      int ret;
> +    

I'd even question this one.

> +    char *usage = "usage: %s <timeout> <sleep>";

static const char[]

> +    int opt, optind = 0;
> +
> +    struct option lopts[] = {

static const

> +        { "help",          no_argument,       NULL, 'h' },
> +    };
> +    const char *sopts = "h";
> +
> +    while ((opt = getopt_long(argc, argv, sopts, lopts, &optind)) != -1) {
> +        switch (opt) {
> +        case '?':
> +        case 'h':
> +            errx(1, usage, argv[0]);

This isn't an error and hence wants to produce an exit code of 0.

> +            break;
> +        default:
> +            errx(1, usage, argv[0]);
> +        }

Please be consistent with break statements: Either you omit them
uniformly when a noreturn function is call, or you put them there
in all cases.

> +    }
>  
>      if (argc < 2)
> -	errx(1, "usage: %s <timeout> <sleep>", argv[0]);
> +        errx(1, usage, argv[0]);
>  
>      daemonize();
>  
>      h = xc_interface_open(NULL, NULL, 0);
>      if (h == NULL)
> -	err(1, "xc_interface_open");
> +        err(1, "xc_interface_open");
> +
> +    t = strtoul(argv[1], &argv[1], 0);
> +    
> +    // argv1 NaN

NaN is a term normally used for floating point values only.

> +    if ( *argv[1] != '\0' )
> +        errx(1, "Error: timeout must be a number, got '%s'", argv[1]);

This still doesn't guarantee a numeric: An empty string as argument
would still yield a value of 0 if I'm not mistaken. As would a
string consisting of just blanks.

> -    t = strtoul(argv[1], NULL, 0);
>      if (t == ULONG_MAX)
> -	err(1, "strtoul");
> +        err(1, "strtoul");
>  
>      s = t / 2;
>      if (argc == 3) {
> -	s = strtoul(argv[2], NULL, 0);
> -	if (s == ULONG_MAX)
> -	    err(1, "strtoul");
> +        s = strtoul(argv[2], &argv[2], 0);
> +        // argv2 NaN
> +        if ( *argv[2] != '\0' ){
> +            errx(1, "Error: sleep must be a number, got '%s'", argv[2]);
> +        }

Like above you don't really need braces here. If, however, you add
them, the opening one wants to be separated by a blank from the
closing parenthesis. And following other style in this file, there
do not want to be blanks immediately inside the parentheses.

Jan

Patch

diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
index 254117b554..6ef5eaf45c 100644
--- a/tools/misc/xenwatchdogd.c
+++ b/tools/misc/xenwatchdogd.c
@@ -8,26 +8,34 @@ 
 #include <stdlib.h>
 #include <unistd.h>
 #include <signal.h>
+#include <string.h>
 #include <stdio.h>
 
+#define _GNU_SOURCE
+#include <getopt.h>
+#if !defined(__GNUC__) && !defined(__GNUG__)
+#define __attribute__(arg) /* empty */
+#endif
+
 xc_interface *h;
 int id = 0;
 
 void daemonize(void)
 {
     switch (fork()) {
-    case -1:
-	err(1, "fork");
-    case 0:
-	break;
-    default:
-	exit(0);
+        case -1:
+            err(1, "fork");
+        case 0:
+            break;
+        default:
+            exit(0);
     }
+
     umask(0);
     if (setsid() < 0)
-	err(1, "setsid");
+        err(1, "setsid");
     if (chdir("/") < 0)
-	err(1, "chdir /");
+        err(1, "chdir /");
     if (freopen("/dev/null", "r", stdin) == NULL)
         err(1, "reopen stdin");
     if(freopen("/dev/null", "w", stdout) == NULL)
@@ -52,39 +60,68 @@  void catch_usr1(int sig)
 
 int main(int argc, char **argv)
 {
+
     int t, s;
     int ret;
+    
+    char *usage = "usage: %s <timeout> <sleep>";
+    int opt, optind = 0;
+
+    struct option lopts[] = {
+        { "help",          no_argument,       NULL, 'h' },
+    };
+    const char *sopts = "h";
+
+    while ((opt = getopt_long(argc, argv, sopts, lopts, &optind)) != -1) {
+        switch (opt) {
+        case '?':
+        case 'h':
+            errx(1, usage, argv[0]);
+            break;
+        default:
+            errx(1, usage, argv[0]);
+        }
+    }
 
     if (argc < 2)
-	errx(1, "usage: %s <timeout> <sleep>", argv[0]);
+        errx(1, usage, argv[0]);
 
     daemonize();
 
     h = xc_interface_open(NULL, NULL, 0);
     if (h == NULL)
-	err(1, "xc_interface_open");
+        err(1, "xc_interface_open");
+
+    t = strtoul(argv[1], &argv[1], 0);
+    
+    // argv1 NaN
+    if ( *argv[1] != '\0' )
+        errx(1, "Error: timeout must be a number, got '%s'", argv[1]);
 
-    t = strtoul(argv[1], NULL, 0);
     if (t == ULONG_MAX)
-	err(1, "strtoul");
+        err(1, "strtoul");
 
     s = t / 2;
     if (argc == 3) {
-	s = strtoul(argv[2], NULL, 0);
-	if (s == ULONG_MAX)
-	    err(1, "strtoul");
+        s = strtoul(argv[2], &argv[2], 0);
+        // argv2 NaN
+        if ( *argv[2] != '\0' ){
+            errx(1, "Error: sleep must be a number, got '%s'", argv[2]);
+        }
+        if (s == ULONG_MAX)
+            err(1, "strtoul");
     }
 
     if (signal(SIGHUP, &catch_exit) == SIG_ERR)
-	err(1, "signal");
+        err(1, "signal");
     if (signal(SIGINT, &catch_exit) == SIG_ERR)
-	err(1, "signal");
+        err(1, "signal");
     if (signal(SIGQUIT, &catch_exit) == SIG_ERR)
-	err(1, "signal");
+        err(1, "signal");
     if (signal(SIGTERM, &catch_exit) == SIG_ERR)
-	err(1, "signal");
+        err(1, "signal");
     if (signal(SIGUSR1, &catch_usr1) == SIG_ERR)
-	err(1, "signal");
+        err(1, "signal");
 
     id = xc_watchdog(h, 0, t);
     if (id <= 0)