Libguestfs - Jul 2016 - Re: [PATCH] mllib: Getopt: fix integer parsing

Since we are using gnulib already, make use of xstrtol to parse the
integer arguments to avoid extra suffixes, etc.

Fixes commit 0f7bf8f714898c606e5d5015fff5b7803dcd1aee.
---
 mllib/getopt-c.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/mllib/getopt-c.c b/mllib/getopt-c.c
index 1f129a7..2d3f9b6 100644
--- a/mllib/getopt-c.c
+++ b/mllib/getopt-c.c
@@ -30,6 +30,8 @@
 #include <error.h>
 #include <assert.h>
 
+#include "xstrtol.h"
+
 #include <caml/alloc.h>
 #include <caml/fail.h>
 #include <caml/memory.h>
@@ -117,6 +119,26 @@ do_call1 (value funv, value paramv)
   CAMLreturn0;
 }
 
+static int
+strtoint (const char *arg)
+{
+  long int num;
+
+  if (xstrtol (arg, NULL, 0, &num, NULL) != LONGINT_OK) {
+    fprintf (stderr, _("%s: '%s' is not a numeric value.\n"),
+             guestfs_int_program_name, arg);
+    show_error (EXIT_FAILURE);
+  }
+
+  if (num <= INT_MIN || num >= INT_MAX) {
+    fprintf (stderr, _("%s: %s: integer out of range\n"),
+             guestfs_int_program_name, arg);
+    show_error (EXIT_FAILURE);
+  }
+
+  return (int) num;
+}
+
 value
 guestfs_int_mllib_getopt_parse (value argsv, value specsv, value anon_funv,
value usage_msgv)
 {
@@ -274,21 +296,13 @@ guestfs_int_mllib_getopt_parse (value argsv, value specsv,
value anon_funv, valu
       break;
 
     case 5:  /* Int of string * (int -> unit) */
-      if (sscanf (optarg, "%d", &num) != 1) {
-        fprintf (stderr, _("'%s' is not a numeric value.\n"),
-                 guestfs_int_program_name);
-        show_error (EXIT_FAILURE);
-      }
+      num = strtoint (optarg);
       v = Field (actionv, 1);
       do_call1 (v, Val_int (num));
       break;
 
     case 6:  /* Set_int of string * int ref */
-      if (sscanf (optarg, "%d", &num) != 1) {
-        fprintf (stderr, _("'%s' is not a numeric value.\n"),
-                 guestfs_int_program_name);
-        show_error (EXIT_FAILURE);
-      }
+      num = strtoint (optarg);
       caml_modify (&Field (Field (actionv, 1), 0), Val_int (num));
       break;
 
-- 
2.7.4

On Mon, Jul 18, 2016 at 10:13:30AM +0200, Pino Toscano
wrote:
> Since we are using gnulib already, make use of xstrtol to parse the
> integer arguments to avoid extra suffixes, etc.
> 
> Fixes commit 0f7bf8f714898c606e5d5015fff5b7803dcd1aee.
> ---
>  mllib/getopt-c.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/mllib/getopt-c.c b/mllib/getopt-c.c
> index 1f129a7..2d3f9b6 100644
> --- a/mllib/getopt-c.c
> +++ b/mllib/getopt-c.c
> @@ -30,6 +30,8 @@
>  #include <error.h>
>  #include <assert.h>
>  
> +#include "xstrtol.h"
> +
>  #include <caml/alloc.h>
>  #include <caml/fail.h>
>  #include <caml/memory.h>
> @@ -117,6 +119,26 @@ do_call1 (value funv, value paramv)
>    CAMLreturn0;
>  }
This function needs to return something other than 'int', since on 64
bit OCaml integers (the final destination) are signed 63 bits.  I
think returning 'long' is a better idea, and the receiving 'num'
should also be 'long' (as in my patch).
> +static int
> +strtoint (const char *arg)
> +{
> +  long int num;
> +
> +  if (xstrtol (arg, NULL, 0, &num, NULL) != LONGINT_OK) {
> +    fprintf (stderr, _("%s: '%s' is not a numeric
value.\n"),
> +             guestfs_int_program_name, arg);
> +    show_error (EXIT_FAILURE);
> +  }
> +
> +  if (num <= INT_MIN || num >= INT_MAX) {
> +    fprintf (stderr, _("%s: %s: integer out of range\n"),
> +             guestfs_int_program_name, arg);
> +    show_error (EXIT_FAILURE);
These bounds are not tight enough.  On 32 bit they should check the
range of a 31 bit signed int, and on 64 bit, a 63 bit signed int.

Rich.
> +  }
> +
> +  return (int) num;
> +}
> +
>  value
>  guestfs_int_mllib_getopt_parse (value argsv, value specsv, value
anon_funv, value usage_msgv)
>  {
> @@ -274,21 +296,13 @@ guestfs_int_mllib_getopt_parse (value argsv, value
specsv, value anon_funv, valu
>        break;
>  
>      case 5:  /* Int of string * (int -> unit) */
> -      if (sscanf (optarg, "%d", &num) != 1) {
> -        fprintf (stderr, _("'%s' is not a numeric
value.\n"),
> -                 guestfs_int_program_name);
> -        show_error (EXIT_FAILURE);
> -      }
> +      num = strtoint (optarg);
>        v = Field (actionv, 1);
>        do_call1 (v, Val_int (num));
>        break;
>  
>      case 6:  /* Set_int of string * int ref */
> -      if (sscanf (optarg, "%d", &num) != 1) {
> -        fprintf (stderr, _("'%s' is not a numeric
value.\n"),
> -                 guestfs_int_program_name);
> -        show_error (EXIT_FAILURE);
> -      }
> +      num = strtoint (optarg);
>        caml_modify (&Field (Field (actionv, 1), 0), Val_int (num));
>        break;
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Monday, 18 July 2016 09:38:43 CEST Richard W.M. Jones
wrote:
> On Mon, Jul 18, 2016 at 10:13:30AM +0200, Pino Toscano wrote:
> > Since we are using gnulib already, make use of xstrtol to parse the
> > integer arguments to avoid extra suffixes, etc.
> > 
> > Fixes commit 0f7bf8f714898c606e5d5015fff5b7803dcd1aee.
> > ---
> >  mllib/getopt-c.c | 34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> > 
> > diff --git a/mllib/getopt-c.c b/mllib/getopt-c.c
> > index 1f129a7..2d3f9b6 100644
> > --- a/mllib/getopt-c.c
> > +++ b/mllib/getopt-c.c
> > @@ -30,6 +30,8 @@
> >  #include <error.h>
> >  #include <assert.h>
> >  
> > +#include "xstrtol.h"
> > +
> >  #include <caml/alloc.h>
> >  #include <caml/fail.h>
> >  #include <caml/memory.h>
> > @@ -117,6 +119,26 @@ do_call1 (value funv, value paramv)
> >    CAMLreturn0;
> >  }
> 
> This function needs to return something other than 'int', since on
64
> bit OCaml integers (the final destination) are signed 63 bits.  I
> think returning 'long' is a better idea, and the receiving
'num'
> should also be 'long' (as in my patch).
I still don't understand why we need to handle values bigger than int
(as in C int, i.e. signed 32 bits) at all -- neither it is actually
needed, nor it would be coherent in 32bit vs 64bit builds.

If we really 64bit values as parameters, then let's just create a new
option type marking that.
> 
> > +static int
> > +strtoint (const char *arg)
> > +{
> > +  long int num;
> > +
> > +  if (xstrtol (arg, NULL, 0, &num, NULL) != LONGINT_OK) {
> > +    fprintf (stderr, _("%s: '%s' is not a numeric
value.\n"),
> > +             guestfs_int_program_name, arg);
> > +    show_error (EXIT_FAILURE);
> > +  }
> > +
> > +  if (num <= INT_MIN || num >= INT_MAX) {
> > +    fprintf (stderr, _("%s: %s: integer out of range\n"),
> > +             guestfs_int_program_name, arg);
> > +    show_error (EXIT_FAILURE);
> 
> These bounds are not tight enough.  On 32 bit they should check the
> range of a 31 bit signed int, and on 64 bit, a 63 bit signed int.
Right, so -(2LL<<30) to (2LL<<30)-1 -- updating the patch
accordingly.

Thanks,
-- 
Pino Toscano