[PATCH v2] parser: be more picky for integer values

[Pekka Paalanen]

Hi Tiago,

looks much better. Some comments still...


On Wed, 14 Mar 2012 20:08:30 -0300
Tiago Vignatti <tiago.vignatti at intel.com> wrote:

> It was silently accepting "-i=3", "-i=3/2", "--idle-time=*3" and similar
> unwanted type of arguments, not changing anything internally; this
> gives the wrong impression for the user. Now it explicitly ignores.
> 
> Signed-off-by: Tiago Vignatti <tiago.vignatti at intel.com>
> ---
> since v1:
> - use strtol() built-in features for checking - thanks Pekka!
> - updated with a more clear commit message
> 
>  shared/option-parser.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/shared/option-parser.c b/shared/option-parser.c
> index 600f110..6efde9d 100644
> --- a/shared/option-parser.c
> +++ b/shared/option-parser.c
> @@ -25,18 +25,68 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <assert.h>
> +#include <limits.h>
> +#include <errno.h>
>  
>  #include "config-parser.h"
>  
> +static int
> +get_int(const char *value)
> +{
> +	char *end;
> +	long int v;
> +
> +	errno = 0;
> +	v = strtol(value, &end, 0);
> +	if ((errno == ERANGE && (v == LONG_MAX || v == LONG_MIN)) ||
> +	    (errno != 0 && v == 0) ||
> +	    (end == value) ||
> +	    (*end != '\0')) {
> +		fprintf(stderr, "arg %s is not a valid number\n", value);
> +		assert(0);

I don't think assert is a good way to deal with illegal user input,
especially when assert can be a no-op. Would prefer returning an error
and letting the calling application to decide whether to continue or
not.

> +	}
> +
> +	if (v < INT_MIN || v > INT_MAX) {

Sorry, I took the wrong limits, even though they are probably correct.
Should have used INT32_MIN and INT32_MAX from stdint.h instead of
limits.h.

> +		fprintf(stderr, "arg %s doesn't fit into integer\n", value);
> +		assert(0);
> +	}
> +
> +	return (int32_t) v;
> +}
> +
> +static unsigned int
> +get_uint(const char *value)
> +{
> +	char *end;
> +	long int v;
> +
> +	errno = 0;
> +	v = strtol(value, &end, 0);
> +	if ((errno == ERANGE && (v == ULONG_MAX)) ||
> +	    (errno != 0 && v == 0) ||
> +	    (end == value) ||
> +	    (*end != '\0')) {
> +		fprintf(stderr, "arg %s is not a valid number\n", value);
> +		assert(0);
> +	}
> +
> +	if (v < 0 || v > INT_MAX) {

The maximum for unsigned is different to signed: UINT32_MAX

> +		fprintf(stderr, "arg %s doesn't fit into integer\n", value);

"unsigned" might be a nice addition to the message. Otherwise if
'--foo -5' says "doesn't fit into integer", it will be puzzling. ;-)

> +		assert(0);
> +	}
> +
> +	return (uint32_t) v;
> +}


Thanks,
pq