diff mbox series

[06/18] posix: Remove glob GET_LOGIN_NAME_MAX usage

Message ID 1502463044-4042-7-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show
Series None | expand

Commit Message

Adhemerval Zanella Aug. 11, 2017, 2:50 p.m. UTC
Current glob implementation allows non limited user name for home
directory construction on GLOB_TILDE case.  To accomplish it glob
either construct a name on stack if size are small enough (based
on current alloca_used) value in heap otherwise.

There is no actual login to resize the buffer in case of the resizing
the buffer in case of ERANGE, so a static buffer using glibc default
LOGIN_NAME_MAX is suffice.

Checked on x86_64-linux-gnu.

	* posix/glob.c (LOGIN_NAME_MAX): Define if not defined.
	(glob): Use static buffer for user_name on getlogin_r.
---
 posix/glob.c | 36 +++++-------------------------------
 1 file changed, 5 insertions(+), 31 deletions(-)

-- 
2.7.4

Comments

Paul Eggert Sept. 2, 2017, 10:50 p.m. UTC | #1
On 08/11/2017 07:50 AM, Adhemerval Zanella wrote:

> There is no actual login to resize the buffer in case of the resizing

> the buffer in case of ERANGE, so a static buffer using glibc default

> LOGIN_NAME_MAX is suffice.


Although I had trouble parsing that, I think you're saying that because 
the current glob.c goes awry when sysconf (_SC_LOGIN_NAME_MAX) < 0, it's 
OK if we change glob.c to insist on a fixed-size limit of 255 bytes on 
user name length so that glob continues to mishandle (presumably 
mostly-theoretical) environments with longer user names. But that's not 
the GNU style, which is to avoid arbitrary limits. Instead, let's fix 
glob.c so that it doesn't need to know the user name length limit. 
Obviously glob should use heap allocation for anything large, which 
suggests that it should use a scratch buffer for the login name.

I looked into this, and it's easy enough to change glob.c to use the 
tail of the scratch buffer that it's already using for getpwnam_r (given 
your previously-proposed patches), and this simplifies glob's 
memory-allocation code. I installed the attached patch into Gnulib to do 
that. Please take a look at it for your next go-round with glibc. Thanks.

This is mostly-theoretical stuff, of course, as this code is exercised 
only when $HOME is unset or empty.

> +	      char user_name[LOGIN_NAME_MAX];


A nit: that array needs to be one byte bigger, for the trailing NULL. 
This point is irrelevant to the attached Gnulib patch, which doesn't use 
LOGIN_NAME_MAX.
From 47688d5de93a7baf7b203a7b687d3f4809667dcd Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>

Date: Sat, 2 Sep 2017 15:39:16 -0700
Subject: [PATCH] glob: fix bugs with long login names
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Adhemerval Zanella in:
https://sourceware.org/ml/libc-alpha/2017-08/msg00455.html
* lib/glob.c (GET_LOGIN_NAME_MAX): Remove.
(glob): Use the same scratch buffer for both getlogin_r and
getpwnam_r.  Don’t require preallocation of the login name.  This
simplifies storage allocation, and corrects the handling of
long login names.
---
 ChangeLog  | 11 ++++++++
 lib/glob.c | 88 +++++++++++++++++++++-----------------------------------------
 2 files changed, 41 insertions(+), 58 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index b67d21799..351495b2f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2017-09-02  Paul Eggert  <eggert@cs.ucla.edu>
+
+	glob: fix bugs with long login names
+	Problem reported by Adhemerval Zanella in:
+	https://sourceware.org/ml/libc-alpha/2017-08/msg00455.html
+	* lib/glob.c (GET_LOGIN_NAME_MAX): Remove.
+	(glob): Use the same scratch buffer for both getlogin_r and
+	getpwnam_r.  Don’t require preallocation of the login name.  This
+	simplifies storage allocation, and corrects the handling of
+	long login names.
+
 2017-09-02  Bruno Haible  <bruno@clisp.org>
 
 	dirent: Update doc.
diff --git a/lib/glob.c b/lib/glob.c
index 7ca11361e..8eb2b9730 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -75,12 +75,6 @@
 #include <flexmember.h>
 #include <glob_internal.h>
 #include <scratch_buffer.h>
-
-#ifdef _SC_LOGIN_NAME_MAX
-# define GET_LOGIN_NAME_MAX()   sysconf (_SC_LOGIN_NAME_MAX)
-#else
-# define GET_LOGIN_NAME_MAX()   (-1)
-#endif
 
 static const char *next_brace_sub (const char *begin, int flags) __THROWNL;
 
@@ -611,67 +605,45 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
               else
                 home_dir = "c:/users/default"; /* poor default */
 #else
-              int success;
-              char *name;
-              int malloc_name = 0;
-              size_t buflen = GET_LOGIN_NAME_MAX () + 1;
-
-              if (buflen == 0)
-                /* 'sysconf' does not support _SC_LOGIN_NAME_MAX.  Try
-                   a moderate value.  */
-                buflen = 20;
-              if (glob_use_alloca (alloca_used, buflen))
-                name = alloca_account (buflen, alloca_used);
-              else
+              int err;
+              struct passwd *p;
+              struct passwd pwbuf;
+              struct scratch_buffer s;
+              scratch_buffer_init (&s);
+              while (true)
                 {
-                  name = malloc (buflen);
-                  if (name == NULL)
+                  p = NULL;
+                  err = __getlogin_r (s.data, s.length);
+                  if (err == 0)
                     {
-                      retval = GLOB_NOSPACE;
-                      goto out;
-                    }
-                  malloc_name = 1;
-                }
-
-              success = __getlogin_r (name, buflen) == 0;
-              if (success)
-                {
-                  struct passwd *p;
-                  struct scratch_buffer pwtmpbuf;
-                  scratch_buffer_init (&pwtmpbuf);
 # if defined HAVE_GETPWNAM_R || defined _LIBC
-                  struct passwd pwbuf;
-
-                  while (getpwnam_r (name, &pwbuf,
-                                     pwtmpbuf.data, pwtmpbuf.length, &p)
-                         == ERANGE)
-                    {
-                      if (!scratch_buffer_grow (&pwtmpbuf))
-                        {
-                          retval = GLOB_NOSPACE;
-                          goto out;
-                        }
-                    }
+                      size_t ssize = strlen (s.data) + 1;
+                      err = getpwnam_r (s.data, &pwbuf, s.data + ssize,
+                                        s.length - ssize, &p);
 # else
-                  p = getpwnam (name);
+                      p = getpwnam (s.data);
+                      if (p == NULL)
+                        err = errno;
 # endif
-                  if (p != NULL)
+                    }
+                  if (err != ERANGE)
+                    break;
+                  if (!scratch_buffer_grow (&s))
                     {
-                      home_dir = strdup (p->pw_dir);
-                      malloc_home_dir = 1;
-                      if (home_dir == NULL)
-                        {
-                          scratch_buffer_free (&pwtmpbuf);
-                          retval = GLOB_NOSPACE;
-                          goto out;
-                        }
+                      retval = GLOB_NOSPACE;
+                      goto out;
                     }
-                  scratch_buffer_free (&pwtmpbuf);
                 }
-              else
+              if (err == 0)
+                {
+                  home_dir = strdup (p->pw_dir);
+                  malloc_home_dir = 1;
+                }
+              scratch_buffer_free (&s);
+              if (err == 0 && home_dir == NULL)
                 {
-                  if (__glibc_unlikely (malloc_name))
-                    free (name);
+                  retval = GLOB_NOSPACE;
+                  goto out;
                 }
 #endif /* WINDOWS32 */
             }
-- 
2.13.5
diff mbox series

Patch

diff --git a/posix/glob.c b/posix/glob.c
index 99e9c54..3a74758 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -91,10 +91,8 @@ 
 #include <glob_internal.h>
 #include <scratch_buffer.h>
 
-#ifdef _SC_LOGIN_NAME_MAX
-# define GET_LOGIN_NAME_MAX()	sysconf (_SC_LOGIN_NAME_MAX)
-#else
-# define GET_LOGIN_NAME_MAX()	(-1)
+#ifndef LOGIN_NAME_MAX
+# define LOGIN_NAME_MAX 256
 #endif
 
 static const char *next_brace_sub (const char *begin, int flags) __THROWNL;
@@ -667,28 +665,9 @@  glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
 	  if (home_dir == NULL || home_dir[0] == '\0')
 	    {
 	      int success;
-	      char *name;
-	      int malloc_name = 0;
-	      size_t buflen = GET_LOGIN_NAME_MAX () + 1;
-
-	      if (buflen == 0)
-		/* `sysconf' does not support _SC_LOGIN_NAME_MAX.  Try
-		   a moderate value.  */
-		buflen = 20;
-	      if (glob_use_alloca (alloca_used, buflen))
-		name = alloca_account (buflen, alloca_used);
-	      else
-		{
-		  name = malloc (buflen);
-		  if (name == NULL)
-		    {
-		      retval = GLOB_NOSPACE;
-		      goto out;
-		    }
-		  malloc_name = 1;
-		}
+	      char user_name[LOGIN_NAME_MAX];
 
-	      success = __getlogin_r (name, buflen) == 0;
+	      success = __getlogin_r (user_name, sizeof (user_name)) == 0;
 	      if (success)
 		{
 		  struct passwd *p;
@@ -698,7 +677,7 @@  glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
 		  struct scratch_buffer pwtmpbuf;
 		  scratch_buffer_init (&pwtmpbuf);
 
-		  while (getpwnam_r (name, &pwbuf,
+		  while (getpwnam_r (user_name, &pwbuf,
 				     pwtmpbuf.data, pwtmpbuf.length, &p)
 			 != 0)
 		    {
@@ -730,11 +709,6 @@  glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
 		    }
 		  scratch_buffer_free (&pwtmpbuf);
 		}
-	      else
-		{
-		  if (__glibc_unlikely (malloc_name))
-		    free (name);
-		}
 	    }
 	  if (home_dir == NULL || home_dir[0] == '\0')
 	    {