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%hadrons.org@localhost>
> Cc: christos <christos%netbsd.org@localhost>
> Cc: Đoàn Trần Công Danh <congdanhqx%gmail.com@localhost>
> Cc: Eli Schwartz <eschwartz93%gmail.com@localhost>
> Cc: Sam James <sam%gentoo.org@localhost>
> Cc: Serge Hallyn <serge%hallyn.com@localhost>
> Cc: Iker Pedrosa <ipedrosa%redhat.com@localhost>
> Cc: Michael Vetter <jubalh%iodoru.org@localhost>
> Cc: <libbsd%lists.freedesktop.org@localhost>
> Cc: <liba2i%lists.linux.dev@localhost>
> Signed-off-by: Alejandro Colomar <alx%kernel.org@localhost>
> ---
>
> 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%kernel.org@localhost>
> *
> * 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/>
Attachment:
signature.asc
Description: PGP signature