[PATCH v1] xstrtol: Remove dead code

[PATCH v1] xstrtol: Remove dead code

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 2117 bytes --]

strtol(3) has a limited set of possible states:

-  The base was invalid.
   -  return 0
   -  errno = EINVAL
   -  endp is not set
   We cover this case with the assure() call, before strtol(3).

-  No conversion was performed.
   -  return 0
   -  errno may be EINVAL, or may be unset.
   -  *endp == s
   We cover this case with the 'if (*p == s)' check.

-  Conversion performed with extra trailing characters.
   -  return any number
   -  errno is not set
   -  *endp != s
   -  **endp != '\0'
   We let this fall through.

-  String fully converted.
   -  return any number
   -  errno is not set
   -  *endp != s
   -  **endp == '\0'
   We let this fall through.

-  Overflow
   -  return LONG_MAX or LONG_MIN
   -  errno = ERANGE
   -  *endp != s
   We cover this with 'else if (errno != 0)'

The condition '*endp != s && errno != 0 && errno != ERANGE' is
unreachable.  The only errno possible if '*endp != s' is ERANGE.

Fixes: 790855e18a1d (2003-10-14, "Handle invalid suffixes and overflow independently, so that ...")
Cc: Paul Eggert <eggert@cs.ucla.edu>
Cc: Bruno Haible <bruno@clisp.org>
Cc: Đoàn Trần Công Danh <congdanhqx@gmail.com>
Cc: Eli Schwartz <eschwartz93@gmail.com>
Cc: Sam James <sam@gentoo.org>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Cc: "Andrew J. Hesford" <ajh@sideband.org>
Cc: Michael Vetter <jubalh@iodoru.org>
Cc: <liba2i@lists.linux.dev>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
Range-diff against v0:
-:  ---------- > 1:  1af702673f xstrtol: Remove dead code

 lib/xstrtol.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/xstrtol.c b/lib/xstrtol.c
index 575c16d45f..5d10ce041e 100644
--- a/lib/xstrtol.c
+++ b/lib/xstrtol.c
@@ -110,10 +110,8 @@ __xstrtol (const char *s, char **ptr, int strtol_base,
       else
         return LONGINT_INVALID;
     }
-  else if (errno != 0)
+  else if (errno == ERANGE)
     {
-      if (errno != ERANGE)
-        return LONGINT_INVALID;
       err = LONGINT_OVERFLOW;
     }
 
-- 
2.45.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] xstrtol: Remove dead code

[Bruno Haible]

Hi Alejandro,

> strtol(3) has a limited set of possible states:
> ...
> The condition '*endp != s && errno != 0 && errno != ERANGE' is
> unreachable.  The only errno possible if '*endp != s' is ERANGE.

Such a statement can be true if you look at the standards (ISO C, POSIX).

However, there's a difference between what the standards say and what the
systems actually do. The Gnulib documentation contains thousands of examples
of such differences.

Gnulib therefore (almost) never assumes that there are no possible errno
values besides the ones listed in the standards.
  - Some systems return "wrong" errno values. Example: [1]
  - Some systems fail with ENOMEM when memory is tight. Who says that
    an implementation of strtol() cannot use malloc() ? Some implementations
    of strtod() do use malloc().

So, what you call "dead code", I call "defensive programming". I would not
like to apply this patch.

Bruno

[1] https://www.gnu.org/software/gnulib/manual/html_node/getlogin_005fr.html

Re: [PATCH v1] xstrtol: Remove dead code

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 1721 bytes --]

On Thu, Jul 18, 2024 at 11:09:40PM GMT, Bruno Haible wrote:
> Hi Alejandro,

Hi Bruno,

> > strtol(3) has a limited set of possible states:
> > ...
> > The condition '*endp != s && errno != 0 && errno != ERANGE' is
> > unreachable.  The only errno possible if '*endp != s' is ERANGE.
> 
> Such a statement can be true if you look at the standards (ISO C, POSIX).
> 
> However, there's a difference between what the standards say and what the
> systems actually do. The Gnulib documentation contains thousands of examples
> of such differences.
> 
> Gnulib therefore (almost) never assumes that there are no possible errno
> values besides the ones listed in the standards.
>   - Some systems return "wrong" errno values. Example: [1]
>   - Some systems fail with ENOMEM when memory is tight. Who says that
>     an implementation of strtol() cannot use malloc() ? Some implementations
>     of strtod() do use malloc().
> 
> So, what you call "dead code", I call "defensive programming". I would not
> like to apply this patch.

Makes sense.  I think we should document that possibility in the manual
page.  Maybe say that other errno values are possible in some systems?
Otherwise, it's already a hell of a function to take care of, and most
uses don't handle that possibility at the moment.  (Yet more reasons to
use a wrapper that returns -1 & sets errno on error, as the rest of
libc.)

Would you send a patch?  (I'd write it myself, but you probably can
provide more info in the commit message.)

Have a lovely night!
Alex

> 
> Bruno
> 
> [1] https://www.gnu.org/software/gnulib/manual/html_node/getlogin_005fr.html
> 
> 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] xstrtol: Remove dead code

[Andrew J. Hesford]

Stop tagging me in your submissions. I have no involvement in any of this activity.
-- 
Andrew J. Hesford
ajh@sideband.org
[Mobile communication]

> On Jul 18, 2024, at 5:25 PM, Alejandro Colomar <alx@kernel.org> wrote:
> 
> On Thu, Jul 18, 2024 at 11:09:40PM GMT, Bruno Haible wrote:
>> Hi Alejandro,
> 
> Hi Bruno,
> 
>>> strtol(3) has a limited set of possible states:
>>> ...
>>> The condition '*endp != s && errno != 0 && errno != ERANGE' is
>>> unreachable.  The only errno possible if '*endp != s' is ERANGE.
>> 
>> Such a statement can be true if you look at the standards (ISO C, POSIX).
>> 
>> However, there's a difference between what the standards say and what the
>> systems actually do. The Gnulib documentation contains thousands of examples
>> of such differences.
>> 
>> Gnulib therefore (almost) never assumes that there are no possible errno
>> values besides the ones listed in the standards.
>>  - Some systems return "wrong" errno values. Example: [1]
>>  - Some systems fail with ENOMEM when memory is tight. Who says that
>>    an implementation of strtol() cannot use malloc() ? Some implementations
>>    of strtod() do use malloc().
>> 
>> So, what you call "dead code", I call "defensive programming". I would not
>> like to apply this patch.
> 
> Makes sense.  I think we should document that possibility in the manual
> page.  Maybe say that other errno values are possible in some systems?
> Otherwise, it's already a hell of a function to take care of, and most
> uses don't handle that possibility at the moment.  (Yet more reasons to
> use a wrapper that returns -1 & sets errno on error, as the rest of
> libc.)
> 
> Would you send a patch?  (I'd write it myself, but you probably can
> provide more info in the commit message.)
> 
> Have a lovely night!
> Alex
> 
>> 
>> Bruno
>> 
>> [1] https://www.gnu.org/software/gnulib/manual/html_node/getlogin_005fr.html
>> 
>> 
>> 
> 
> --
> <https://www.alejandro-colomar.es/>
> <signature.asc>

Re: [PATCH v1] xstrtol: Remove dead code

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 3699 bytes --]

[CC -= Andrew, per explicit request]

On Thu, Jul 18, 2024 at 11:25:11PM GMT, Alejandro Colomar wrote:
> On Thu, Jul 18, 2024 at 11:09:40PM GMT, Bruno Haible wrote:
> > Hi Alejandro,

Hi Bruno,

> > > strtol(3) has a limited set of possible states:
> > > ...
> > > The condition '*endp != s && errno != 0 && errno != ERANGE' is
> > > unreachable.  The only errno possible if '*endp != s' is ERANGE.
> > 
> > Such a statement can be true if you look at the standards (ISO C, POSIX).
> > 
> > However, there's a difference between what the standards say and what the
> > systems actually do. The Gnulib documentation contains thousands of examples
> > of such differences.
> > 
> > Gnulib therefore (almost) never assumes that there are no possible errno
> > values besides the ones listed in the standards.
> >   - Some systems return "wrong" errno values. Example: [1]
> >   - Some systems fail with ENOMEM when memory is tight. Who says that
> >     an implementation of strtol() cannot use malloc() ? Some implementations
> >     of strtod() do use malloc().
> > 
> > So, what you call "dead code", I call "defensive programming". I would not
> > like to apply this patch.

On the other hand, I'm not sure that defensive programming is valid in
this case.  I already discussed this topic with Serge (but we didn't
have in mind any specific implementation) some time ago, and,

We'd need to know the precise specification of that system that can set
errno = ENOMEM.

Is *endp guaranteed to be set?  Or may it be unset (as happens with
EINVAL)?

If it is allowed to keep *endp unset, then the first `if (*p == s)`
would already be UB.  And for truly being defensive, we'd need to assume
that it may leave *endp unset.  Thus we cannot read that until we know
that no errno has been set.  But then comes the problem that some
systems set EINVAL on what strtoi(3) calls ECANCELED.  To work with all
of those systems, we'd probably need to pass a dummy NULL to make sure
we can inspect *endp regardless of strtol(3) having set it or not:

intmax_t
strtoi(char *s, char **restrict endp, int base,
    intmax_t min, intmax_t max, int *restrict status)
{
	int        errno_saved, st;
	char       *e;
	char       **ep;
	intmax_t   n;

	e = NULL;
	ep = &e;

	if (status == NULL)
		status = &st;

	if (base != 0 && (base < 2 || base > 36)) {
		*status = EINVAL;
		return MAX(min, MIN(max, 0));
	}

	errno_saved = errno;
	errno = 0;

	n = strtoimax(s, ep, base);

	if (errno == ERANGE || n < min || n > max)
		*status = ERANGE;
	else if (e == s && (errno == 0 || errno == EINVAL))
		*status = ECANCELED;
	else if (errno != 0)
		*status = errno;
	else if (*e != '\0')
		*status = ENOTSUP;
	else
		*status = 0;

	errno = errno_saved;

	if (endp != NULL)
		*endp = e;

	return MAX(min, MIN(max, n));
}

Does this make sense?

Have a lovely night!
Alex

> 
> Makes sense.  I think we should document that possibility in the manual
> page.  Maybe say that other errno values are possible in some systems?
> Otherwise, it's already a hell of a function to take care of, and most
> uses don't handle that possibility at the moment.  (Yet more reasons to
> use a wrapper that returns -1 & sets errno on error, as the rest of
> libc.)
> 
> Would you send a patch?  (I'd write it myself, but you probably can
> provide more info in the commit message.)
> 
> Have a lovely night!
> Alex
> 
> > 
> > Bruno
> > 
> > [1] https://www.gnu.org/software/gnulib/manual/html_node/getlogin_005fr.html
> > 
> > 
> > 
> 
> -- 
> <https://www.alejandro-colomar.es/>



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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] xstrtol: Remove dead code

[Bruno Haible]

Alejandro Colomar wrote:
> >   - Some systems return "wrong" errno values. Example: [1]
> >   - Some systems fail with ENOMEM when memory is tight. Who says that
> >     an implementation of strtol() cannot use malloc() ? Some implementations
> >     of strtod() do use malloc().
> > 
> > So, what you call "dead code", I call "defensive programming". I would not
> > like to apply this patch.
> 
> Makes sense.  I think we should document that possibility in the manual
> page.

Well, I wouldn't want to document just _theoretical_ platforms. The set of
manual pages has a certain scope, regarding platform, probably Linux
(and Hurd, maybe?). It's the behaviour on these platforms which should
be documented, nothing more, nothing less.

Defensive programming means to imagine other behaviours that could
possibly occur. It is subjective; some programmers want to be more cautious
than others.

> Maybe say that other errno values are possible in some systems?

Other errno values are always possible, as far as I understand POSIX.
<https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap01.html>
section 1.2 ERRORS.

It would be overkill to state this in hundreds of manual pages, IMO.

Bruno

Re: [PATCH v1] xstrtol: Remove dead code

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 2113 bytes --]

Hi Bruno,

On Fri, Jul 19, 2024 at 12:34:39AM GMT, Bruno Haible wrote:
> Alejandro Colomar wrote:
> > >   - Some systems return "wrong" errno values. Example: [1]
> > >   - Some systems fail with ENOMEM when memory is tight. Who says that
> > >     an implementation of strtol() cannot use malloc() ? Some implementations
> > >     of strtod() do use malloc().
> > > 
> > > So, what you call "dead code", I call "defensive programming". I would not
> > > like to apply this patch.
> > 
> > Makes sense.  I think we should document that possibility in the manual
> > page.
> 
> Well, I wouldn't want to document just _theoretical_ platforms.

While it may be theoretical for strtol(3), you said it was real for
strtod(3), and strtod(3) defers to strtol(3) for the example program.

We should provide a portable example of use of strtod(3), and rather
than writing another example for strtod(3), maybe we could improve the
strtol(3) one for theoretical implementations.


> The set of
> manual pages has a certain scope, regarding platform, probably Linux
> (and Hurd, maybe?). It's the behaviour on these platforms which should
> be documented, nothing more, nothing less.

Not really.  We do document portability to other Unix systems.  Mainly
the BSDs, but in some important cases we document others.

> 
> Defensive programming means to imagine other behaviours that could
> possibly occur. It is subjective; some programmers want to be more cautious
> than others.
> 
> > Maybe say that other errno values are possible in some systems?
> 
> Other errno values are always possible, as far as I understand POSIX.
> <https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap01.html>
> section 1.2 ERRORS.

True.

> It would be overkill to state this in hundreds of manual pages, IMO.

strtol(3) is special-enough regarding errors, that it might make sense.
Even if we don't mention it in ERRORS, it would make sense to fix the
example program to make it portable.

> 
> Bruno

Have a lovely night!
Alex

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] xstrtol: Remove dead code

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 4395 bytes --]

On Fri, Jul 19, 2024 at 12:14:19AM GMT, Alejandro Colomar wrote:
> [CC -= Andrew, per explicit request]
> 
> On Thu, Jul 18, 2024 at 11:25:11PM GMT, Alejandro Colomar wrote:
> > On Thu, Jul 18, 2024 at 11:09:40PM GMT, Bruno Haible wrote:
> > > Hi Alejandro,
> 
> Hi Bruno,
> 
> > > > strtol(3) has a limited set of possible states:
> > > > ...
> > > > The condition '*endp != s && errno != 0 && errno != ERANGE' is
> > > > unreachable.  The only errno possible if '*endp != s' is ERANGE.
> > > 
> > > Such a statement can be true if you look at the standards (ISO C, POSIX).
> > > 
> > > However, there's a difference between what the standards say and what the
> > > systems actually do. The Gnulib documentation contains thousands of examples
> > > of such differences.
> > > 
> > > Gnulib therefore (almost) never assumes that there are no possible errno
> > > values besides the ones listed in the standards.
> > >   - Some systems return "wrong" errno values. Example: [1]
> > >   - Some systems fail with ENOMEM when memory is tight. Who says that
> > >     an implementation of strtol() cannot use malloc() ? Some implementations
> > >     of strtod() do use malloc().
> > > 
> > > So, what you call "dead code", I call "defensive programming". I would not
> > > like to apply this patch.
> 
> On the other hand, I'm not sure that defensive programming is valid in
> this case.  I already discussed this topic with Serge (but we didn't
> have in mind any specific implementation) some time ago, and,
> 
> We'd need to know the precise specification of that system that can set
> errno = ENOMEM.
> 
> Is *endp guaranteed to be set?  Or may it be unset (as happens with
> EINVAL)?
> 
> If it is allowed to keep *endp unset, then the first `if (*p == s)`
> would already be UB.  And for truly being defensive, we'd need to assume
> that it may leave *endp unset.  Thus we cannot read that until we know
> that no errno has been set.  But then comes the problem that some
> systems set EINVAL on what strtoi(3) calls ECANCELED.  To work with all
> of those systems, we'd probably need to pass a dummy NULL to make sure
> we can inspect *endp regardless of strtol(3) having set it or not:
> 
> intmax_t
> strtoi(char *s, char **restrict endp, int base,
>     intmax_t min, intmax_t max, int *restrict status)
> {
> 	int        errno_saved, st;
> 	char       *e;
> 	char       **ep;
> 	intmax_t   n;
> 
> 	e = NULL;
> 	ep = &e;
> 
> 	if (status == NULL)
> 		status = &st;
> 
> 	if (base != 0 && (base < 2 || base > 36)) {
> 		*status = EINVAL;
> 		return MAX(min, MIN(max, 0));
> 	}
> 
> 	errno_saved = errno;
> 	errno = 0;
> 
> 	n = strtoimax(s, ep, base);
> 
> 	if (errno == ERANGE || n < min || n > max)
> 		*status = ERANGE;
> 	else if (e == s && (errno == 0 || errno == EINVAL))
> 		*status = ECANCELED;
> 	else if (errno != 0)
> 		*status = errno;
> 	else if (*e != '\0')
> 		*status = ENOTSUP;
> 	else
> 		*status = 0;

Hmm, bug there.  I should have written:

	if (errno == ERANGE)
		*status = ERANGE;
	else if (e == s && (errno == 0 || errno == EINVAL))
		*status = ECANCELED;
	else if (errno != 0)
		*status = errno;
	else if (n < min || n > max)
		*status = ERANGE;
	else if (*e != '\0')
		*status = ENOTSUP;
	else
		*status = 0;

> 
> 	errno = errno_saved;
> 
> 	if (endp != NULL)
> 		*endp = e;
> 
> 	return MAX(min, MIN(max, n));
> }
> 
> Does this make sense?
> 
> Have a lovely night!
> Alex
> 
> > 
> > Makes sense.  I think we should document that possibility in the manual
> > page.  Maybe say that other errno values are possible in some systems?
> > Otherwise, it's already a hell of a function to take care of, and most
> > uses don't handle that possibility at the moment.  (Yet more reasons to
> > use a wrapper that returns -1 & sets errno on error, as the rest of
> > libc.)
> > 
> > Would you send a patch?  (I'd write it myself, but you probably can
> > provide more info in the commit message.)
> > 
> > Have a lovely night!
> > Alex
> > 
> > > 
> > > Bruno
> > > 
> > > [1] https://www.gnu.org/software/gnulib/manual/html_node/getlogin_005fr.html
> > > 
> > > 
> > > 
> > 
> > -- 
> > <https://www.alejandro-colomar.es/>
> 
> 
> 
> -- 
> <https://www.alejandro-colomar.es/>



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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] xstrtol: Remove dead code

[Bruno Haible]

Alejandro Colomar wrote:
> strtod(3) defers to strtol(3) for the example program.

Is this adequate? strtod() can produce underflow (e.g. for "1e-500").
In this case all implementations except MSVC set errno to ERANGE.
This is a case that cannot happen in strtol().

Bruno

Re: [PATCH v1] xstrtol: Remove dead code

[Bruno Haible]

Alejandro Colomar wrote:
> We'd need to know the precise specification of that system that can set
> errno = ENOMEM.
> 
> Is *endp guaranteed to be set?  Or may it be unset (as happens with
> EINVAL)?

One system that calls malloc() during strtod() is NetBSD. See

$ grep -ri malloc src/lib/libc/gdtoa/
src/lib/libc/gdtoa/g__fmt.c:            if ((decimalpoint_cache = MALLOC(strlen(s0) + 1)) != NULL) {
src/lib/libc/gdtoa/README:for intermediate quantities, and MALLOC (see gdtoaimp.h) is called only
src/lib/libc/gdtoa/README:if the private pool does not suffice.   2000 is large enough that MALLOC
src/lib/libc/gdtoa/gdtoaimp.h: * #define MALLOC your_malloc, where your_malloc(n) acts like malloc(n)
src/lib/libc/gdtoa/gdtoaimp.h: *        appropriate.  If MALLOC is undefined, malloc will be invoked
src/lib/libc/gdtoa/gdtoaimp.h: *        recycle memory acquired from MALLOC, #define FREE to be the
src/lib/libc/gdtoa/gdtoaimp.h: *        suffices to get rid of MALLOC calls except for unusual cases,
src/lib/libc/gdtoa/gdtoaimp.h:#ifdef MALLOC
src/lib/libc/gdtoa/gdtoaimp.h:extern Char *MALLOC ANSI((size_t));
src/lib/libc/gdtoa/gdtoaimp.h:#define MALLOC malloc
src/lib/libc/gdtoa/misc.c:              rv = (Bigint *)MALLOC(sizeof(Bigint) + (x-1)*sizeof(ULong));
src/lib/libc/gdtoa/misc.c:                      rv = (Bigint*)MALLOC(len*sizeof(double));

Feel free to look up the source code in glibc, musl libc, FreeBSD,
OpenBSD, Darwin, OpenSolaris, Android. And that still does not
give information about Solaris and AIX.

Here in the Gnulib project, we typically write a unit test that verifies
the behaviour/properties we expect from a certain libc functions, and then
let it run on all platforms. If we get test failures, we need to adjust the
expectations. This works even for ENOMEM situations; see
  gnulib/tests/test-fprintf-posix2.{c,sh}
But it is, admittedly, quite some effort to write and maintain such a test.

Bruno

Re: [PATCH v1] xstrtol: Remove dead code

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

Hi Bruno,

On Fri, Jul 19, 2024 at 06:54:40PM GMT, Bruno Haible wrote:
> Alejandro Colomar wrote:
> > strtod(3) defers to strtol(3) for the example program.
> 
> Is this adequate? strtod() can produce underflow (e.g. for "1e-500").
> In this case all implementations except MSVC set errno to ERANGE.
> This is a case that cannot happen in strtol().

I was thinking tonight that it would be better to actually add an
example program to strtod(3) (and also document in VERSIONS that other
systems do different).

Cheers,
Alex

> 
> Bruno
> 
> 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]