diff mbox series

[v2,4/6] tools/misc: xenwatchdogd: add parse_secs()

Message ID 20240329111056.6118-5-leigh@solinno.co.uk (mailing list archive)
State Superseded
Headers show
Series xenwatchdogd bugfixes and enhancements | expand

Commit Message

Leigh Brown March 29, 2024, 11:10 a.m. UTC 
From: Leigh Brown <leigh@solinno.co.uk>

Create a new parse_secs() function to parse the timeout and sleep
parameters. This ensures that non-numeric parameters are not
accidentally treated as numbers.

Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
---
 tools/misc/xenwatchdogd.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

Comments

Anthony PERARD April 9, 2024, 3:14 p.m. UTC | #1 
On Fri, Mar 29, 2024 at 11:10:54AM +0000, leigh@solinno.co.uk wrote:
> diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
> index 2e7f9f51c5..19ec4c5359 100644
> --- a/tools/misc/xenwatchdogd.c
> +++ b/tools/misc/xenwatchdogd.c
> @@ -49,6 +49,18 @@ static void catch_usr1(int sig)
>      done = true;
>  }
>  
> +static int parse_secs(const char *arg, const char *what)
> +{
> +    char *endptr;
> +    unsigned long val;
> +
> +    val = strtoul(arg, &endptr, 0);
> +    if (val > INT_MAX || *endptr != 0)

nit: '\0' would be more accurate       ^ here.
Otherwise, just `*endptr` without "!= 0" would work too.
But comparing a char to a digit feels wrong.

> +	errx(EXIT_FAILURE, "invalid %s: '%s'", what, arg);
> +
> +    return val;
> +}
> +
>  int main(int argc, char **argv)
>  {
>      int id;
> @@ -64,16 +76,11 @@ int main(int argc, char **argv)
>      if (h == NULL)
>  	err(EXIT_FAILURE, "xc_interface_open");
>  
> -    t = strtoul(argv[1], NULL, 0);
> -    if (t == ULONG_MAX)
> -	err(EXIT_FAILURE, "strtoul");
> +    t = parse_secs(argv[optind], "timeout");
>  
>      s = t / 2;
> -    if (argc == 3) {
> -	s = strtoul(argv[2], NULL, 0);
> -	if (s == ULONG_MAX)
> -	    err(EXIT_FAILURE, "strtoul");
> -    }
> +    if (argc == 3)
> +	s = parse_secs(argv[optind], "sleep");

This is parsing the wrong value here, should it be +1 to parse the sleep
argument instead of the timeout argument a second time.

Also, are you sure you want to start using "optind" here? It's a
variable used by getopt(), but we don't use it yet. It almost feel like
"optind" happen to be set to 1 by luck.


Thanks,
diff mbox series

Patch

diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
index 2e7f9f51c5..19ec4c5359 100644
--- a/tools/misc/xenwatchdogd.c
+++ b/tools/misc/xenwatchdogd.c
@@ -49,6 +49,18 @@  static void catch_usr1(int sig)
     done = true;
 }
 
+static int parse_secs(const char *arg, const char *what)
+{
+    char *endptr;
+    unsigned long val;
+
+    val = strtoul(arg, &endptr, 0);
+    if (val > INT_MAX || *endptr != 0)
+	errx(EXIT_FAILURE, "invalid %s: '%s'", what, arg);
+
+    return val;
+}
+
 int main(int argc, char **argv)
 {
     int id;
@@ -64,16 +76,11 @@  int main(int argc, char **argv)
     if (h == NULL)
 	err(EXIT_FAILURE, "xc_interface_open");
 
-    t = strtoul(argv[1], NULL, 0);
-    if (t == ULONG_MAX)
-	err(EXIT_FAILURE, "strtoul");
+    t = parse_secs(argv[optind], "timeout");
 
     s = t / 2;
-    if (argc == 3) {
-	s = strtoul(argv[2], NULL, 0);
-	if (s == ULONG_MAX)
-	    err(EXIT_FAILURE, "strtoul");
-    }
+    if (argc == 3)
+	s = parse_secs(argv[optind], "sleep");
 
     if (signal(SIGHUP, &catch_exit) == SIG_ERR)
 	err(EXIT_FAILURE, "signal");