[PATCH v2] getpw: Get rid of alloca

[PATCH v2] getpw: Get rid of alloca

[Joe Simmons-Talbott]

Since _SC_GETPW_R_SIZE_MAX will be always NSS_BUFLEN_PASSWD, use a fixed
sized array rather than alloca to avoid potential stack overflow.
---
Changes to v1:
 * Get rid of the scratch_buffer and use a fixed sized array instead.

 pwd/getpw.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/pwd/getpw.c b/pwd/getpw.c
index cf747374b8..86ccbc8d6e 100644
--- a/pwd/getpw.c
+++ b/pwd/getpw.c
@@ -15,7 +15,6 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <alloca.h>
 #include <errno.h>
 #include <stdio.h>
 #include <unistd.h>
@@ -31,31 +30,39 @@ int __getpw (__uid_t uid, char *buf);
 int
 __getpw (__uid_t uid, char *buf)
 {
-  size_t buflen;
-  char *tmpbuf;
+  char tmpbuf[NSS_BUFLEN_PASSWD];
   struct passwd resbuf, *p;
+  int retval = 0;
 
   if (buf == NULL)
     {
       __set_errno (EINVAL);
-      return -1;
+      retval =  -1;
+      goto error_out;
     }
 
-  buflen = __sysconf (_SC_GETPW_R_SIZE_MAX);
-  tmpbuf = alloca (buflen);
-
-  if (__getpwuid_r (uid, &resbuf, tmpbuf, buflen, &p) != 0)
-    return -1;
+  if (__getpwuid_r (uid, &resbuf, tmpbuf, sizeof (tmpbuf), &p) != 0)
+    {
+      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:
+  return retval;
 }
 weak_alias (__getpw, getpw)
 
-- 
2.39.2

Re: [PATCH v2] getpw: Get rid of alloca

[Florian Weimer]

* Joe Simmons-Talbott via Libc-alpha:

> Since _SC_GETPW_R_SIZE_MAX will be always NSS_BUFLEN_PASSWD, use a fixed
> sized array rather than alloca to avoid potential stack overflow.

_SC_GETPW_R_SIZE_MAX is not very well-named, it is the initial suggested
buffer size.  The code should use the usual scratch buffer retry loop.

Thanks,
Florian

Re: [PATCH v2] getpw: Get rid of alloca

[Adhemerval Zanella Netto]

On 29/08/23 05:03, Florian Weimer via Libc-alpha wrote:
> * Joe Simmons-Talbott via Libc-alpha:
> 
>> Since _SC_GETPW_R_SIZE_MAX will be always NSS_BUFLEN_PASSWD, use a fixed
>> sized array rather than alloca to avoid potential stack overflow.
> 
> _SC_GETPW_R_SIZE_MAX is not very well-named, it is the initial suggested
> buffer size.  The code should use the usual scratch buffer retry loop.

I though about that on initial revision, however this will change the function
semantic and the code below:

  long int sz = sysconf (_SC_GETPW_R_SIZE_MAX);
  char *buf = NULL;
  while (1) {
    buf = realloc (buf,  sz);
    r = getpw (uid, buf);
    if (r != -1 || errno != ENOMEM)
      break;
    sz *= 2;
  }

will start to trigger a buffer overrun.  This is an old tricky interface
where I would prefer if we continue to keep same semantic, and maybe 
deprecate and move it to compat symbol. 

Re: [PATCH v2] getpw: Get rid of alloca

[Joe Simmons-Talbott]

On Tue, Aug 29, 2023 at 09:26:39AM -0300, Adhemerval Zanella Netto via Libc-alpha wrote:
> 
> 
> On 29/08/23 05:03, Florian Weimer via Libc-alpha wrote:
> > * Joe Simmons-Talbott via Libc-alpha:
> > 
> >> Since _SC_GETPW_R_SIZE_MAX will be always NSS_BUFLEN_PASSWD, use a fixed
> >> sized array rather than alloca to avoid potential stack overflow.
> > 
> > _SC_GETPW_R_SIZE_MAX is not very well-named, it is the initial suggested
> > buffer size.  The code should use the usual scratch buffer retry loop.
> 
> I though about that on initial revision, however this will change the function
> semantic and the code below:
> 
>   long int sz = sysconf (_SC_GETPW_R_SIZE_MAX);
>   char *buf = NULL;
>   while (1) {
>     buf = realloc (buf,  sz);
>     r = getpw (uid, buf);
>     if (r != -1 || errno != ENOMEM)
>       break;
>     sz *= 2;
>   }
> 
> will start to trigger a buffer overrun.  This is an old tricky interface
> where I would prefer if we continue to keep same semantic, and maybe 
> deprecate and move it to compat symbol. 
> 

Where does this leave this patch?  Should I submit a patch deprecating
getpw?

Thanks,
Joe