Re: [PATCH] lib/xstrtol.c: Fix xstrtol() on EINVAL (invalid base)

[Alejandro Colomar]

On Wed, Jul 24, 2024 at 09:27:14PM GMT, Alejandro Colomar wrote:
> POSIX leaves the value of *endptr unspecified if the base is invalid;
> it is not unmodified.
> 
> glibc and musl libc leave the object unmodified.  However, the BSDs and
> Bionic libc modify it to be nptr.
> 
> That, combined with the fact that POSIX allows implementations of
> strtol(3) to set EINVAL when no digits are found, makes it impossible to
> portably check if the base was valid after the call.
> 
> This bug was introduced after I incorrectly mentioned that strtol(3)
> left *endptr unmodified; that's an implementation-defined behavior of
> glibc and musl.
> 
> Fixes: 64ddc975e72c (2024-07-18, "xstrtol: document and stray less from 
> strtol")
> Fixes: 16b33e664942 (2024-07-19, "xstrtol: be more robust against odd 
> failures")
> Link: <https://mail-index.netbsd.org/tech-misc/2024/07/20/msg000412.html>

I meant this link instead:

<https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=58461>

> Reported-by: Robert Elz <kre@munnari.OZ.AU>
> Cc: Paul Eggert <eggert@cs.ucla.edu>
> Cc: Bruno Haible <bruno@clisp.org>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
> ---
> 
> Hi Paul, Bruno,
> 
> I haven't tested the patch.  Please check thoroughly.  I'm tired of
> strtol(3).  :)
> 
> This was discussed in a NetBSD report about their strtoi(3)
> implementation not being portable.  I CCed Paul in a message there.
> 
> Have a lovely day!
> Alex
> 
>  lib/xstrtol.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/xstrtol.c b/lib/xstrtol.c
> index 797d3e4dee..ccbfa82924 100644
> --- a/lib/xstrtol.c
> +++ b/lib/xstrtol.c
> @@ -71,9 +71,14 @@ strtol_error
>  __xstrtol (char const *nptr, char **endptr, int base,
>             __strtol_t *val, char const *valid_suffixes)
>  {
> -  char *t_ptr = nullptr;
> +  char *t_ptr;
>    char **p = endptr ? endptr : &t_ptr;
>  
> +  *p = (char *) nptr;
> +
> +  if (base < 0 || base == 1 || 36 < base)
> +    return LONGINT_INVALID;
> +
>    if (! TYPE_SIGNED (__strtol_t))
>      {
>        char const *q = nptr;
> @@ -81,20 +86,23 @@ __xstrtol (char const *nptr, char **endptr, int base,
>        while (isspace (ch))
>          ch = *++q;
>        if (ch == '-')
> -        {
> -          *p = (char *) nptr;
> -          return LONGINT_INVALID;
> -        }
> +        return LONGINT_INVALID;
>      }
>  
>    errno = 0;
> -  __strtol_t tmp = __strtol (nptr, &t_ptr, base);
> -  if (!t_ptr)
> -    return LONGINT_INVALID;
> -  *p = t_ptr;
> +  __strtol_t tmp = __strtol (nptr, p, base);
> +
>    strtol_error err = LONGINT_OK;
>  
> -  if (*p == nptr && (errno == 0 || errno == EINVAL))
> +  if (errno == ERANGE)
> +    {
> +      err = LONGINT_OVERFLOW;
> +    }
> +  else if (errno != 0 && errno != EINVAL)
> +    {
> +      return LONGINT_INVALID;
> +    }
> +  else if (*p == nptr)
>      {
>        /* If there is no number but there is a valid suffix, assume the
>           number is 1.  The string is invalid otherwise.  */
> @@ -102,12 +110,6 @@ __xstrtol (char const *nptr, char **endptr, int base,
>          return LONGINT_INVALID;
>        tmp = 1;
>      }
> -  else if (errno != 0)
> -    {
> -      if (errno != ERANGE)
> -        return LONGINT_INVALID;
> -      err = LONGINT_OVERFLOW;
> -    }
>  
>    /* Let valid_suffixes == NULL mean "allow any suffix".  */
>    /* FIXME: update all callers except the ones that allow suffixes
> -- 
> 2.45.2
> 



-- 
<https://www.alejandro-colomar.es/>

signature.asc
Description: PGP signature