diff mbox series

getpw: Get rid of alloca

Message ID 20230707200400.378096-1-josimmon@redhat.com
State New
Headers
Series getpw: Get rid of alloca |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed
redhat-pt-bot/TryBot-still_applies warning Patch no longer applies to master

Commit Message

Joe Simmons-Talbott July 7, 2023, 8:04 p.m. UTC 
  Use a scratch_buffer rather than alloca to avoid potential stack
overflow.
---
 pwd/getpw.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

Comments

Joe Simmons-Talbott Aug. 10, 2023, 1:45 p.m. UTC | #1 
On Fri, Jul 07, 2023 at 04:04:00PM -0400, Joe Simmons-Talbott wrote:
> Use a scratch_buffer rather than alloca to avoid potential stack
> overflow.

Ping.

Thanks,
Joe
> ---
>  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;
>      }
>  
>    buflen = __sysconf (_SC_GETPW_R_SIZE_MAX);
> -  tmpbuf = alloca (buflen);
> +  if (!scratch_buffer_set_array_size (&sbuf, 1, buflen))
> +    {
> +      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)
>  
> -- 
> 2.39.2
>
Joe Simmons-Talbott Aug. 28, 2023, 1:21 p.m. UTC | #2 
Ping.

On Thu, Aug 10, 2023 at 09:45:47AM -0400, Joe Simmons-Talbott via Libc-alpha wrote:
> On Fri, Jul 07, 2023 at 04:04:00PM -0400, Joe Simmons-Talbott wrote:
> > Use a scratch_buffer rather than alloca to avoid potential stack
> > overflow.
> 
> Ping.
> 
> Thanks,
> Joe
> > ---
> >  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;
> >      }
> >  
> >    buflen = __sysconf (_SC_GETPW_R_SIZE_MAX);
> > -  tmpbuf = alloca (buflen);
> > +  if (!scratch_buffer_set_array_size (&sbuf, 1, buflen))
> > +    {
> > +      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)
> >  
> > -- 
> > 2.39.2
> > 
>
Adhemerval Zanella Netto Aug. 28, 2023, 5:01 p.m. UTC | #3 
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)
>
Joe Simmons-Talbott Aug. 28, 2023, 8:37 p.m. UTC | #4 
On Mon, Aug 28, 2023 at 02:01:54PM -0300, Adhemerval Zanella Netto wrote:
> 
> 
> 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.

I removed the scratch_buffer in v2 and used a fixed sized array.

Thanks,
Joe
> 
> > +    {
> > +      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)
> >  
>
diff mbox series

Patch

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;
     }
 
   buflen = __sysconf (_SC_GETPW_R_SIZE_MAX);
-  tmpbuf = alloca (buflen);
+  if (!scratch_buffer_set_array_size (&sbuf, 1, buflen))
+    {
+      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)