This is the mail archive of the libc-alpha@sources.redhat.com mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Fix glob


Hi!

POSIX says:
"If glob() terminates due to an error, it shall return one of the non-zero constants
 defined in <glob.h>. The arguments pglob->gl_pathc and pglob->gl_pathv are still
 set as defined above."
My understanding is that globfree call is needed even if glob fails with non-zero
return code to avoid leaks.  But that can result in double free, as shown in the
first part of do_test in bug-glob2.c testcase below.
There is a http://sources.redhat.com/ml/libc-hacker/2002-02/msg00092.html
patch that has not been applied, but it causes a memory leak, as seen
by the second part of do_test in bug-glob2.c below.
If we wanted to have pglob->gl_pathc != 0 check in globfree, we'd need to
initialize gl_pathv = NULL for both GLOB_DOOFFS and !GLOB_DOOFFS and malloc
it only when incrementing gl_pathc for the first time.
But I think applications aren't supposed to modify gl_path{c,v} themselves,
so this makes no difference.
The side-effect of this patch is that globfree () can be called multiple
times.  If that is not desirable, we can instead modify all places that call
globfree (pglob) in glob.c as in:
                  if (!(flags & GLOB_APPEND))
                    {
                      globfree (pglob);
                      pglob->gl_pathc = 0;
+                     pglob->gl_pathv = NULL;
                    }
                  return result;

2004-10-27  Jakub Jelinek  <jakub@redhat.com>

	* sysdeps/generic/glob.c (globfree): Clear gl_pathv after freeing it.
	* posix/Makefile: Add rules to build and run bug-glob2 test.
	* posix/bug-glob2.c: New test.

--- libc/posix/Makefile.jj	2004-10-01 12:04:59.000000000 +0200
+++ libc/posix/Makefile	2004-10-27 16:27:03.620479730 +0200
@@ -82,7 +82,7 @@ tests		:= tstgetopt testfnm runtests run
 		   bug-regex21 bug-regex22 bug-regex23 tst-nice tst-nanosleep \
 		   transbug tst-rxspencer tst-pcre tst-boost \
 		   bug-ga1 tst-vfork1 tst-vfork2 tst-waitid \
-		   tst-getaddrinfo2 bug-glob1
+		   tst-getaddrinfo2 bug-glob1 bug-glob2
 xtests		:= bug-ga2
 ifeq (yes,$(build-shared))
 test-srcs	:= globtest
@@ -101,7 +101,7 @@ generated := $(addprefix wordexp-test-re
 	     bug-regex21-mem bug-regex21.mtrace \
 	     tst-rxspencer-mem tst-rxspencer.mtrace tst-getconf.out \
 	     tst-pcre-mem tst-pcre.mtrace tst-boost-mem tst-boost.mtrace \
-	     bug-ga2.mtrace bug-ga2-mem
+	     bug-ga2.mtrace bug-ga2-mem bug-glob2.mtrace bug-glob2-mem
 
 include ../Rules
 
@@ -194,7 +194,8 @@ tests: $(objpfx)annexc.out
 ifeq (no,$(cross-compiling))
 tests: $(objpfx)bug-regex2-mem $(objpfx)bug-regex14-mem \
   $(objpfx)bug-regex21-mem $(objpfx)tst-rxspencer-mem \
-  $(objpfx)tst-pcre-mem $(objpfx)tst-boost-mem $(objpfx)tst-getconf.out
+  $(objpfx)tst-pcre-mem $(objpfx)tst-boost-mem $(objpfx)tst-getconf.out \
+  $(objpfx)bug-glob2-mem
 xtests: $(objpfx)bug-ga2-mem
 endif
 
@@ -251,3 +252,8 @@ $(objpfx)bug-ga2-mem: $(objpfx)bug-ga2.o
 	$(common-objpfx)malloc/mtrace $(objpfx)bug-ga2.mtrace > $@
 
 bug-ga2-ENV = MALLOC_TRACE=$(objpfx)bug-ga2.mtrace
+
+bug-glob2-ENV = MALLOC_TRACE=$(objpfx)bug-glob2.mtrace
+
+$(objpfx)bug-glob2-mem: $(objpfx)bug-glob2.out
+	$(common-objpfx)malloc/mtrace $(objpfx)bug-glob2.mtrace > $@
--- libc/posix/bug-glob2.c.jj	2004-10-27 15:48:21.433838016 +0200
+++ libc/posix/bug-glob2.c	2004-10-27 16:29:23.077715851 +0200
@@ -0,0 +1,303 @@
+/* Test glob memory management.
+   for the filesystem access functions.
+   Copyright (C) 2001, 2002, 2004 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, write to the Free
+   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+   02111-1307 USA.  */
+
+#include <errno.h>
+#include <error.h>
+#include <dirent.h>
+#include <glob.h>
+#include <mcheck.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/stat.h>
+
+// #define DEBUG
+#ifdef DEBUG
+# define PRINTF(fmt, args...) \
+  do					\
+    {					\
+      int save_errno = errno;		\
+      printf (fmt, ##args);		\
+      errno = save_errno;		\
+    } while (0)
+#else
+# define PRINTF(fmt, args...)
+#endif
+
+
+static struct
+{
+  const char *name;
+  int level;
+  int type;
+  mode_t mode;
+} filesystem[] =
+{
+  { ".", 1, DT_DIR, 0755 },
+  { "..", 1, DT_DIR, 0755 },
+  { "dir", 1, DT_DIR, 0755 },
+    { ".", 2, DT_DIR, 0755 },
+    { "..", 2, DT_DIR, 0755 },
+    { "readable", 2, DT_DIR, 0755 },
+      { ".", 3, DT_DIR, 0755 },
+      { "..", 3, DT_DIR, 0755 },
+      { "a", 3, DT_REG, 0644 },
+    { "unreadable", 2, DT_DIR, 0111 },
+      { ".", 3, DT_DIR, 0111 },
+      { "..", 3, DT_DIR, 0755 },
+      { "a", 3, DT_REG, 0644 },
+    { "zz-readable", 2, DT_DIR, 0755 },
+      { ".", 3, DT_DIR, 0755 },
+      { "..", 3, DT_DIR, 0755 },
+      { "a", 3, DT_REG, 0644 }
+};
+#define nfiles (sizeof (filesystem) / sizeof (filesystem[0]))
+
+
+typedef struct
+{
+  int level;
+  int idx;
+  struct dirent d;
+  char room_for_dirent[NAME_MAX];
+} my_DIR;
+
+
+static long int
+find_file (const char *s)
+{
+  int level = 1;
+  long int idx = 0;
+
+  if (strcmp (s, ".") == 0)
+    return 0;
+
+  if (s[0] == '.' && s[1] == '/')
+    s += 2;
+
+  while (*s != '\0')
+    {
+      char *endp = strchrnul (s, '/');
+
+      PRINTF ("looking for %.*s, level %d\n", (int) (endp - s), s, level);
+
+      while (idx < nfiles && filesystem[idx].level >= level)
+	{
+	  if (filesystem[idx].level == level
+	      && memcmp (s, filesystem[idx].name, endp - s) == 0
+	      && filesystem[idx].name[endp - s] == '\0')
+	    break;
+	  ++idx;
+	}
+
+      if (idx == nfiles || filesystem[idx].level < level)
+	{
+	  errno = ENOENT;
+	  return -1;
+	}
+
+      if (*endp == '\0')
+	return idx + 1;
+
+      if (filesystem[idx].type != DT_DIR
+	  && (idx + 1 >= nfiles
+	      || filesystem[idx].level >= filesystem[idx + 1].level))
+	{
+	  errno = ENOTDIR;
+	  return -1;
+	}
+
+      ++idx;
+
+      s = endp + 1;
+      ++level;
+    }
+
+  errno = ENOENT;
+  return -1;
+}
+
+
+static void *
+my_opendir (const char *s)
+{
+  long int idx = find_file (s);
+  my_DIR *dir;
+
+  if (idx == -1)
+    {
+      PRINTF ("my_opendir(\"%s\") == NULL (%m)\n", s);
+      return NULL;
+    }
+
+  if ((filesystem[idx].mode & 0400) == 0)
+    {
+      errno = EACCES;
+      PRINTF ("my_opendir(\"%s\") == NULL (%m)\n", s);
+      return NULL;
+    }
+
+  dir = (my_DIR *) malloc (sizeof (my_DIR));
+  if (dir == NULL)
+    {
+      printf ("cannot allocate directory handle: %m\n");
+      exit (EXIT_FAILURE);
+    }
+
+  dir->level = filesystem[idx].level;
+  dir->idx = idx;
+
+  PRINTF ("my_opendir(\"%s\") == { level: %d, idx: %ld }\n",
+	  s, filesystem[idx].level, idx);
+
+  return dir;
+}
+
+
+static struct dirent *
+my_readdir (void *gdir)
+{
+  my_DIR *dir = gdir;
+
+  if (dir->idx == -1)
+    {
+      PRINTF ("my_readdir ({ level: %d, idx: %ld }) = NULL\n",
+	      dir->level, (long int) dir->idx);
+      return NULL;
+    }
+
+  while (dir->idx < nfiles && filesystem[dir->idx].level > dir->level)
+    ++dir->idx;
+
+  if (dir->idx == nfiles || filesystem[dir->idx].level < dir->level)
+    {
+      dir->idx = -1;
+      PRINTF ("my_readdir ({ level: %d, idx: %ld }) = NULL\n",
+	      dir->level, (long int) dir->idx);
+      return NULL;
+    }
+
+  dir->d.d_ino = dir->idx;
+
+#ifdef _DIRENT_HAVE_D_TYPE
+  dir->d.d_type = filesystem[dir->idx].type;
+#endif
+
+  strcpy (dir->d.d_name, filesystem[dir->idx].name);
+
+#ifdef _DIRENT_HAVE_D_TYPE
+  PRINTF ("my_readdir ({ level: %d, idx: %ld }) = { d_ino: %ld, d_type: %d, d_name: \"%s\" }\n",
+	  dir->level, (long int) dir->idx, dir->d.d_ino, dir->d.d_type,
+	  dir->d.d_name);
+#else
+  PRINTF ("my_readdir ({ level: %d, idx: %ld }) = { d_ino: %ld, d_name: \"%s\" }\n",
+	  dir->level, (long int) dir->idx, dir->d.d_ino,
+	  dir->d.d_name);
+#endif
+
+  ++dir->idx;
+
+  return &dir->d;
+}
+
+
+static void
+my_closedir (void *dir)
+{
+  PRINTF ("my_closedir ()\n");
+  free (dir);
+}
+
+
+/* We use this function for lstat as well since we don't have any.  */
+static int
+my_stat (const char *name, struct stat *st)
+{
+  long int idx = find_file (name);
+
+  if (idx == -1)
+    {
+      PRINTF ("my_stat (\"%s\", ...) = -1 (%m)\n", name);
+      return -1;
+    }
+
+  memset (st, '\0', sizeof (*st));
+
+  if (filesystem[idx].type == DT_UNKNOWN)
+    st->st_mode = DTTOIF (idx + 1 < nfiles
+			  && filesystem[idx].level < filesystem[idx + 1].level
+			  ? DT_DIR : DT_REG) | filesystem[idx].mode;
+  else
+    st->st_mode = DTTOIF (filesystem[idx].type) | filesystem[idx].mode;
+
+  PRINTF ("my_stat (\"%s\", { st_mode: %o }) = 0\n", name, st->st_mode);
+
+  return 0;
+}
+
+
+static void
+init_glob_altdirfuncs (glob_t *pglob)
+{
+  pglob->gl_closedir = my_closedir;
+  pglob->gl_readdir = my_readdir;
+  pglob->gl_opendir = my_opendir;
+  pglob->gl_lstat = my_stat;
+  pglob->gl_stat = my_stat;
+}
+
+
+int
+do_test (void)
+{
+  mtrace ();
+
+  glob_t gl;
+  memset (&gl, 0, sizeof (gl));
+  init_glob_altdirfuncs (&gl);
+
+  if (glob ("dir/*able/*", GLOB_ERR | GLOB_ALTDIRFUNC, NULL, &gl)
+      != GLOB_ABORTED)
+    {
+      puts ("glob did not fail with GLOB_ABORTED");
+      exit (EXIT_FAILURE); 
+    }
+
+  globfree (&gl);
+
+  memset (&gl, 0, sizeof (gl));
+  init_glob_altdirfuncs (&gl);
+
+  gl.gl_offs = 3;
+  if (glob ("dir2/*", GLOB_DOOFFS, NULL, &gl) != GLOB_NOMATCH)
+    {
+      puts ("glob did not fail with GLOB_NOMATCH");
+      exit (EXIT_FAILURE); 
+    }
+
+  globfree (&gl);
+
+  muntrace ();
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
--- libc/sysdeps/generic/glob.c.jj	2004-10-01 12:05:02.000000000 +0200
+++ libc/sysdeps/generic/glob.c	2004-10-27 15:58:58.402729364 +0200
@@ -1135,6 +1135,7 @@ globfree (pglob)
 	if (pglob->gl_pathv[pglob->gl_offs + i] != NULL)
 	  free ((__ptr_t) pglob->gl_pathv[pglob->gl_offs + i]);
       free ((__ptr_t) pglob->gl_pathv);
+      pglob->gl_pathv = NULL;
     }
 }
 #if defined _LIBC && !defined globfree

	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]