lib/58461: strtoi(3) is not portable

[alx%kernel.org@localhost]

>Number:         58461
>Category:       lib
>Synopsis:       strtoi(3) is not portable
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Jul 23 23:20:00 +0000 2024
>Originator:     Alejandro Colomar
>Release:        trunk
>Organization:
Linux man-pages
>Environment:
>Description:
Original report: <https://lists.freedesktop.org/archives/libbsd/2024-July/000456.html>

The implementation of strtoi(3) in NetBSD is not portable.  This is not a problem in NetBSD, but some projects copy the exact implementation for use in other environments.  libbsd is a notorious example, providing BSD APIs in a wide range of Unix(-like) systems.

The reason it's not portable is that strtoimax(3) is allowed to report EINVAL when no digits are present in the string (what strtoi(3) calls ECANCELED).

In such an environment, the following call results in incorrect behavior:

    strtoi("foo", NULL, 0, 0, 0, &st);

The steps are:


   -  strtoimax() reports EINVAL.

   -  *rstatus = errno; sets EINVAL.

   -  The condition `if (*rstatus == 0 && nptr == *endptr)` is not met.

   -  strtoi(3bsd) falls through to the last line, and returns im.

The user receives a bugos error report of EINVAL, where it would expect
ECANCELED.
>How-To-Repeat:

>Fix:
Here's a proposed patch:

<https://mail-index.netbsd.org/tech-misc/2024/07/20/msg000412.html>

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: &#272;oàn Tr&#7847;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>
---

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, so that you can choose.

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;