Re: [PATCH] lib/parser.c: avoid overflow in match_number()

[Randy Dunlap <rdunlap@xenotime.net>]

On 08/09/2012 01:03 PM, Alex Elder wrote:

> The result of converting an integer value to another signed integer
> type that's unable to represent the original value is implementation
> defined.  (See notes in section 6.3.1.3 of the C standard.)
> 
> In match_number(), the result of simple_strtol() (which returns type
> long) is assigned to a value of type int.
> 
> Instead, handle the result of simple_strtol() in a well-defined way,
> and return -ERANGE if the result won't fit in the int variable used
> to hold the parsed result.
> 
> No current callers pay attention to the particular error value
> returned, so this additional return code shouldn't do any harm.
> 
> Signed-off-by: Alex Elder <elder@inktank.com>


Makes sense to me, but I wonder who will merge it.

I'm Cc-ing a couple of possibilities.

> ---
>  lib/parser.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> Index: b/lib/parser.c
> ===================================================================
> --- a/lib/parser.c
> +++ b/lib/parser.c
> @@ -122,13 +122,14 @@ int match_token(char *s, const match_tab
>   *
>   * Description: Given a &substring_t and a base, attempts to parse the substring
>   * as a number in that base. On success, sets @result to the integer represented
> - * by the string and returns 0. Returns either -ENOMEM or -EINVAL on failure.
> + * by the string and returns 0. Returns -ENOMEM, -EINVAL, or -ERANGE on failure.
>   */
>  static int match_number(substring_t *s, int *result, int base)
>  {
>      char *endp;
>      char *buf;
>      int ret;
> +    long val;
>      size_t len = s->to - s->from;
> 
>      buf = kmalloc(len + 1, GFP_KERNEL);
> @@ -136,10 +137,15 @@ static int match_number(substring_t *s,
>          return -ENOMEM;
>      memcpy(buf, s->from, len);
>      buf[len] = '\0';
> -    *result = simple_strtol(buf, &endp, base);
> +
>      ret = 0;
> +    val = simple_strtol(buf, &endp, base);
>      if (endp == buf)
>          ret = -EINVAL;
> +    else if (val < (long) INT_MIN || val > (long) INT_MAX)
> +        ret = -ERANGE;
> +    else
> +        *result = (int) val;
>      kfree(buf);
>      return ret;
>  }
> -- 


-- 
~Randy