Avoid use of atoi in some places in libc

[Noah Goldstein]

On Thu, Dec 15, 2022 at 2:14 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Thu, 15 Dec 2022, Noah Goldstein via Libc-alpha wrote:
>
> > > diff --git a/argp/argp-help.c b/argp/argp-help.c
> > > index 90a2795cef..328b981374 100644
> > > --- a/argp/argp-help.c
> > > +++ b/argp/argp-help.c
> > > @@ -210,7 +210,7 @@ fill_in_uparams (const struct argp_state *state)
> > >               }
> > >             else if (isdigit ((unsigned char) *arg))
> > >               {
> > > -               val = atoi (arg);
> > > +               val = strtol (arg, NULL, 10);
> > >                 while (isdigit ((unsigned char) *arg))
> > >                   arg++;
> > Can we just use the `end` argument of strtol and remove this loop?.
>
> Here is a version doing that.
>
> Avoid use of atoi in some places in libc
>
> This patch is split out of
> <https://sourceware.org/pipermail/libc-alpha/2022-December/144122.html>.
>
> atoi has undefined behavior on out-of-range input, which makes it
> problematic to use anywhere in glibc that might be processing input
> out-of-range for atoi but not specified to produce undefined behavior
> for the function calling atoi.  Change some uses of atoi to call
> strtol instead; this avoids the undefined behavior, though there is no
> guarantee that the overflow handling of strtol is really right in
> those places either.  This also serves to avoid localplt test failures
> given an installed header redirection for strtol (which means that the
> call from the inline atoi implementation doesn't end up at a hidden
> alias from libc_hidden_proto).
>
> Certainly, the use of atoi is questionable in argp-help.c (shared with
> gnulib, so shouldn't depend on glibc implementation details, and
> processing user-provided input), and maybe also in argp-parse.c (I'm
> not sure what that code in argp-parse.c is meant to be used for).  I
> also changed inet/rexec.c and resolv/res_init.c similarly to use
> strtol to avoid such localplt failures, although given those files (in
> those versions) are only used in glibc it's not problematic for them
> to rely on the specific behavior of glibc's atoi on out-of-range input
> (in the absence of compiler optimizations based on the undefined
> behavior) in the same way it's problematic for gnulib code to do so.
>
> There may be other uses of atoi (or atol or atoll), in any of glibc's
> installed code, for which it would also be appropriate to avoid the
> undefined behavior on out-of-range input; this patch only fixes the
> specific cases needed to avoid localplt failures.
>
> Tested for x86_64.
>
> diff --git a/argp/argp-help.c b/argp/argp-help.c
> index 90a2795cef..f7f1134c80 100644
> --- a/argp/argp-help.c
> +++ b/argp/argp-help.c
> @@ -210,9 +210,9 @@ fill_in_uparams (const struct argp_state *state)
>               }
>             else if (isdigit ((unsigned char) *arg))
>               {
> -               val = atoi (arg);
> -               while (isdigit ((unsigned char) *arg))
> -                 arg++;
> +               char *ep;
> +               val = strtol (arg, &ep, 10);
> +               arg = ep;
>                 SKIPWS (arg);
>               }
>
> diff --git a/argp/argp-parse.c b/argp/argp-parse.c
> index 68dc45417b..1533b43aaf 100644
> --- a/argp/argp-parse.c
> +++ b/argp/argp-parse.c
> @@ -147,7 +147,7 @@ argp_default_parser (int key, char *arg, struct argp_state *state)
>        break;
>
>      case OPT_HANG:
> -      _argp_hang = atoi (arg ? arg : "3600");
> +      _argp_hang = arg ? strtol (arg, NULL, 10) : 3600;
>        while (_argp_hang-- > 0)
>         __sleep (1);
>        break;
> diff --git a/inet/rexec.c b/inet/rexec.c
> index 064e979d68..c647b7ac34 100644
> --- a/inet/rexec.c
> +++ b/inet/rexec.c
> @@ -134,7 +134,7 @@ retry:
>                 if (!getnameinfo(&sa2.sa, sa2len,
>                                  NULL, 0, servbuff, sizeof(servbuff),
>                                  NI_NUMERICSERV))
> -                       port = atoi(servbuff);
> +                       port = strtol(servbuff, NULL, 10);
>                 (void) sprintf(num, "%u", port);
Is this needed at all? Can we just copy `servbuff` to `num`?
Or is this to handle overflow inputs to `servbuff`?

>                 (void) __write(s, num, strlen(num)+1);
>                 { socklen_t len = sizeof (from);
> diff --git a/resolv/res_init.c b/resolv/res_init.c
> index 2c0bea658e..61b958a437 100644
> --- a/resolv/res_init.c
> +++ b/resolv/res_init.c
> @@ -654,7 +654,7 @@ res_setoptions (struct resolv_conf_parser *parser, const char *options)
>        /* Search for and process individual options.  */
>        if (!strncmp (cp, "ndots:", sizeof ("ndots:") - 1))
>          {
> -          int i = atoi (cp + sizeof ("ndots:") - 1);
> +          int i = strtol (cp + sizeof ("ndots:") - 1, NULL, 10);
>            if (i <= RES_MAXNDOTS)
>              parser->template.ndots = i;
>            else
> @@ -662,7 +662,7 @@ res_setoptions (struct resolv_conf_parser *parser, const char *options)
>          }
>        else if (!strncmp (cp, "timeout:", sizeof ("timeout:") - 1))
>          {
> -          int i = atoi (cp + sizeof ("timeout:") - 1);
> +          int i = strtol (cp + sizeof ("timeout:") - 1, NULL, 10);
>            if (i <= RES_MAXRETRANS)
>              parser->template.retrans = i;
>            else
> @@ -670,7 +670,7 @@ res_setoptions (struct resolv_conf_parser *parser, const char *options)
>          }
>        else if (!strncmp (cp, "attempts:", sizeof ("attempts:") - 1))
>          {
> -          int i = atoi (cp + sizeof ("attempts:") - 1);
> +          int i = strtol (cp + sizeof ("attempts:") - 1, NULL, 10);
>            if (i <= RES_MAXRETRY)
>              parser->template.retry = i;
>            else
>
> --
> Joseph S. Myers
> joseph@codesourcery.com