[PATCH] strto[iu](3): Make the implementation portable

[Alejandro Colomar]

On Sat, Jul 20, 2024 at 09:03:34PM GMT, Alejandro Colomar wrote:
> POSIX allows systems that report EINVAL when no digits are found.  On
> such systems the only way to differentiate EINVAL and ECANCELED is to
> initialized the end pointer to NULL before the call.  On EINVAL cases,
> strto*max(3) will leave the pointer unmodified, so we'll read back the
> original NULL.  On ECANCELED cases, strto*max(3) will set it to nptr.
> 
> Link: <https://lists.freedesktop.org/archives/libbsd/2024-July/000456.html>
> Cc: Guillem Jover <guillem at hadrons.org>
> Cc: christos <christos at netbsd.org>
> Cc: Đoàn Trần Công Danh <congdanhqx at gmail.com>
> Cc: Eli Schwartz <eschwartz93 at gmail.com>
> Cc: Sam James <sam at gentoo.org>
> Cc: Serge Hallyn <serge at hallyn.com>
> Cc: Iker Pedrosa <ipedrosa at redhat.com>
> Cc: Michael Vetter <jubalh at iodoru.org>
> Cc: <libbsd at lists.freedesktop.org>
> Cc: <liba2i at lists.linux.dev>
> Signed-off-by: Alejandro Colomar <alx at kernel.org>
> ---
> 
> Hi Christos,
> 
> I'm not sure if this patch is wanted in NetBSD.  It doesn't fix any bugs
> there.  It would be interesting for those pasting this code in other
> systems (which is currently done by libbsd).  Just in case you're
> interested, here it is.
> 
> I didn't test it (I don't have a NetBSD build around), so please review
> thoroughly.
> 
> BTW, the line 
> 
> +	if (nptr == e && (*rstatus == 0 || *rstatus == EINVAL))
> 
> You could also choose to simplify it as just `if (nptr == e)`, since
> all existing implementations (AFAIK) only arrive at nptr == e with errno
> being either 0 or EINVAL.  However, for preventing ENOMEM or other
> strange errors that an implementation might add, those tests are there.
> Feel free to drop them (I didn't add them in my own strtoi(3)
> implementation).  I've kept them here for completeness (and because you
> already had a test `*rstatus == 0` in that line, which was already
> superfluous, so I guessed you were preventing that kind of problems.

Self-correction: it was not superfluous.  It was there to prevent
reading `*endptr` when the base is invalid, which would have been UB,
since it was uninitialized.

> 
> Have a lovely day!
> Alex
> 
>  common/lib/libc/stdlib/_strtoi.h | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/common/lib/libc/stdlib/_strtoi.h b/common/lib/libc/stdlib/_strtoi.h
> index b838608f6b52..bea6a9f285a7 100644
> --- a/common/lib/libc/stdlib/_strtoi.h
> +++ b/common/lib/libc/stdlib/_strtoi.h
> @@ -3,6 +3,7 @@
>  /*-
>   * Copyright (c) 1990, 1993
>   *	The Regents of the University of California.  All rights reserved.
> + * Copyright (c) 2024, Alejandro Colomar <alx at kernel.org>
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions
> @@ -69,7 +70,7 @@ INT_FUNCNAME(_int_, _FUNCNAME, _l)(const char * __restrict nptr,
>  	int serrno;
>  #endif
>  	__TYPE im;
> -	char *ep;
> +	char *e;
>  	int rep;
>  
>  	_DIAGASSERT(hi >= lo);
> @@ -77,8 +78,7 @@ INT_FUNCNAME(_int_, _FUNCNAME, _l)(const char * __restrict nptr,
>  	_DIAGASSERT(nptr != NULL);
>  	/* endptr may be NULL */
>  
> -	if (endptr == NULL)
> -		endptr = &ep;
> +	e = NULL;
>  
>  	if (rstatus == NULL)
>  		rstatus = &rep;
> @@ -90,9 +90,9 @@ INT_FUNCNAME(_int_, _FUNCNAME, _l)(const char * __restrict nptr,
>  
>  #if defined(_KERNEL) || defined(_STANDALONE) || \
>      defined(HAVE_NBTOOL_CONFIG_H) || defined(BCS_ONLY)
> -	im = __WRAPPED(nptr, endptr, base);
> +	im = __WRAPPED(nptr, &e, base);
>  #else
> -	im = __WRAPPED_L(nptr, endptr, base, loc);
> +	im = __WRAPPED_L(nptr, &e, base, loc);
>  #endif
>  
>  #if !defined(_KERNEL) && !defined(_STANDALONE)
> @@ -100,8 +100,11 @@ INT_FUNCNAME(_int_, _FUNCNAME, _l)(const char * __restrict nptr,
>  	errno = serrno;
>  #endif
>  
> +	if (endptr != NULL && e != NULL)
> +		*endptr = e;
> +
>  	/* No digits were found */
> -	if (*rstatus == 0 && nptr == *endptr)
> +	if (nptr == e && (*rstatus == 0 || *rstatus == EINVAL))
>  		*rstatus = ECANCELED;
>  
>  	if (im < lo) {
> @@ -117,7 +120,7 @@ INT_FUNCNAME(_int_, _FUNCNAME, _l)(const char * __restrict nptr,
>  	}
>  
>  	/* There are further characters after number */
> -	if (*rstatus == 0 && **endptr != '\0')
> +	if (*rstatus == 0 && *e != '\0')
>  		*rstatus = ENOTSUP;
>  
>  	return im;
> 
> base-commit: 7a4c6afd05862bf8c28f0730d8d9cd7e2dce2a50
> -- 
> 2.45.2
> 



-- 
<https://www.alejandro-colomar.es/>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/libbsd/attachments/20240721/88acfb12/attachment.sig>