Re: [PATCH] getpw: Get rid of alloca

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 07/07/23 17:04, Joe Simmons-Talbott via Libc-alpha wrote:
> Use a scratch_buffer rather than alloca to avoid potential stack
> overflow.
> ---
>  pwd/getpw.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/pwd/getpw.c b/pwd/getpw.c
> index cf747374b8..7a27d79910 100644
> --- a/pwd/getpw.c
> +++ b/pwd/getpw.c
> @@ -15,8 +15,8 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <alloca.h>
>  #include <errno.h>
> +#include <scratch_buffer.h>
>  #include <stdio.h>
>  #include <unistd.h>
>  #include <pwd.h>
> @@ -34,28 +34,48 @@ __getpw (__uid_t uid, char *buf)
>    size_t buflen;
>    char *tmpbuf;
>    struct passwd resbuf, *p;
> +  int retval = 0;
> +  struct scratch_buffer sbuf;
> +  scratch_buffer_init (&sbuf);
>  
>    if (buf == NULL)
>      {
>        __set_errno (EINVAL);
> -      return -1;
> +      retval =  -1;
> +      goto error_out;
>      }
>  

There is no need to call scratch_buffer_free here.  You can move the
scratch_buffer initialization later.

>    buflen = __sysconf (_SC_GETPW_R_SIZE_MAX);
> -  tmpbuf = alloca (buflen);
> +  if (!scratch_buffer_set_array_size (&sbuf, 1, buflen))

The _SC_GETPW_R_SIZE_MAX will be always NSS_BUFLEN_PASSWD so there is no need to
a scratch_buffer here (similar to sysdeps/posix/cuserid.c assumption).  Since the
functions is historical tricky to be used correctly, I think it should continue to
fail with passwords larger than _SC_GETPW_R_SIZE_MAX.

> +    {
> +      retval = -1;
> +      goto error_out;
> +    }
> +  tmpbuf = sbuf.data;
>  
>    if (__getpwuid_r (uid, &resbuf, tmpbuf, buflen, &p) != 0)
> -    return -1;
> +    {
> +      retval = -1;
> +      goto error_out;
> +    }
>  
>    if (p == NULL)
> -    return -1;
> +    {
> +      retval = -1;
> +      goto error_out;
> +    }
>  
>    if (sprintf (buf, "%s:%s:%lu:%lu:%s:%s:%s", p->pw_name, p->pw_passwd,
>  	       (unsigned long int) p->pw_uid, (unsigned long int) p->pw_gid,
>  	       p->pw_gecos, p->pw_dir, p->pw_shell) < 0)
> -    return -1;
> +    {
> +      retval = -1;
> +      goto error_out;
> +    }
>  
> -  return 0;
> +error_out:
> +  scratch_buffer_free (&sbuf);
> +  return retval;
>  }
>  weak_alias (__getpw, getpw)
>