Re: [PATCH v1] xstrtol: Remove dead code

[Alejandro Colomar]

[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/>

signature.asc
Description: PGP signature