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

Note that libc-hacker is a closed list. You may look at the archives of this list, but subscription and posting are not open.


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) 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/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
--- 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"

	Jakub


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