Re: [Findutils-patches] patch for #20662 memory leak

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to James Youngman on 8/4/2007 8:21 AM:
> Hmm, does this change modify the result of this program?   (on Cygwin
> you may prefer to remove the "//" argument without affecting the
> validity of the check).

Oops, there was an unintentional modification.

> 
> $ cd /
> $ find tmp/ / // ///  /tmp/ /tmp -maxdepth 0 -printf "%p %f\n"
> tmp/ tmp/
> / /
> // /

On cygwin, this line should end up as:
// //
but other than that, the pre-patch output matched yours.

Another example that my first patch inadvertently broke (showing the
correct pre-patch behavior):

$ find / // /// -maxdepth 0 -name /
find: warning: Unix filenames usually don't contain slashes (though
pathnames do).  That means that '-name `/'' will probably evaluate to
false all the time on this system.  You might find the '-wholename' test
more useful, or perhaps '-samefile'.  Alternatively, if you are using GNU
grep, you could use 'find ... -print0 | grep -FzZ `/''.
/
///

In other words, -name / will match for the root directory (and nothing
else, since / and possibly // are the only filenames where basename()
results in an answer that contains /).

The problem with the first version of my patch is that last_component is
documented to return the empty string for /.  So we'll have to go with
base_name/free instead (but preferably, only when we have to, rather than
always, to minimize malloc pressure), as in the following patch.  I've
tested this version with the problematic cases you listed, both with
oldfind and fts-based find, and added the non-platform specific tests to
the testsuite, in the process fixing 20688.

OK to commit?  Should I port the entire patch back to the branch,
including the -nowarn and testsuite changes; or just the memory leak fix?

2007-08-04  Eric Blake  <address@hidden>

        Fix Savannah bugs #20662, 20688.
        * find/find.c (at_top): Avoid memory leak.
        * find/pred.c (do_fprintf, pred_iname, pred_name): Likewise.
        (pred_name_common): New function, factored from pred_iname and
        pred_name.
        * find/parser.c (check_name_arg): Let -nowarn silence -name /.
        * locate/locate.c (visit_basename): Avoid memory leak.
        * NEWS: Document the changes.
        * doc/find.texi (Warning Messages): Document -nowarn's effect on
        -name and -iname.
        * find/testsuite/find.gnu/name-slash.exp: New test, to ensure
        20662 doesn't regress on '-name /', and that 20688 silences the
        warning.
        * find/testsuite/find.gnu/printf-slash.exp: Likewise.
        * find/testsuite/find.gnu/name-slash.xo: Expected results.
        * find/testsuite/find.gnu/printf-slash.xo: Likewise.
        * find/testsuite/Makefile.am (EXTRA_DIST_XO, EXTRA_DIST_EXP):
        Distribute new tests.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGtULB84KuGfSFAYARAjOKAJ9uWPtdxk4rDkDXWeeCJARKEC0jTgCgkTdI
gL+SsCYo1Ac2lScED1O8YOE=
=Mu3E
-----END PGP SIGNATURE-----

Index: NEWS
===================================================================
RCS file: /sources/findutils/findutils/NEWS,v
retrieving revision 1.199
diff -u -p -r1.199 NEWS
--- NEWS        1 Aug 2007 03:39:57 -0000       1.199
+++ NEWS        5 Aug 2007 03:16:39 -0000
@@ -27,11 +27,17 @@ does not follow the POSIX rules of doing
 updatedb, frcode and code now complies with the GNU Project's coding
 standards.
 
+#20662: Avoid memory leak in find -name and other places affected by
+gnulib dirname module.
+
 ** Enhancements
 
 #20594: Allow fine-tuning of the default argument size used by xargs
 and find at ./configure time.
 
+#20688: The warning printed by -name or -iname when the pattern to
+ match contains a slash can now be silenced by -nowarn.
+
 ** Documentation Fixes
 
 Point out more explicitly that the subsecond timestamp support
Index: doc/find.texi
===================================================================
RCS file: /sources/findutils/findutils/doc/find.texi,v
retrieving revision 1.143
diff -u -p -r1.143 find.texi
--- doc/find.texi       22 Jul 2007 13:52:52 -0000      1.143
+++ doc/find.texi       5 Aug 2007 03:16:48 -0000
@@ -31,8 +31,8 @@
 This file documents the GNU utilities for finding files that match
 certain criteria and performing various operations on them.
 
-Copyright (C) 1994, 1996, 1998, 2000, 2001, 2003, 2004, 2005 Free
-Software Foundation, Inc.
+Copyright (C) 1994, 1996, 1998, 2000, 2001, 2003, 2004, 2005, 2006,
+2007 Free Software Foundation, Inc.
 
 Permission is granted to make and distribute verbatim copies of
 this manual provided the copyright notice and this permission notice
@@ -2972,6 +2972,11 @@ Use of the @samp{-ipath} option which is
 @item
 Specifying an option (for example @samp{-mindepth}) after a non-option
 (for example @samp{-type} or @samp{-print}) on the command line.
address@hidden
+Use of the @samp{-name} or @samp{-iname} option with a slash character
+in combination with any other character.  Since the name predicates
+only compare against the basename of the file under question, the only
+match against a slash can be the root directory itself.
 @end itemize
 
 The default behaviour above is designed to work in that way so that
Index: find/find.c
===================================================================
RCS file: /sources/findutils/findutils/find/find.c,v
retrieving revision 1.122
diff -u -p -r1.122 find.c
--- find/find.c 2 Jul 2007 08:57:58 -0000       1.122
+++ find/find.c 5 Aug 2007 03:16:52 -0000
@@ -1,6 +1,6 @@
 /* find -- search for files in a directory hierarchy
    Copyright (C) 1990, 91, 92, 93, 94, 2000, 
-                 2003, 2004, 2005 Free Software Foundation, Inc.
+                 2003, 2004, 2005, 2007 Free Software Foundation, Inc.
 
    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -947,13 +947,13 @@ at_top (char *pathname,
                       struct stat *pstat))
 {
   int dirchange;
-  char *parent_dir = dir_name(pathname);
-  char *base = base_name(pathname);
-  
+  char *parent_dir = dir_name (pathname);
+  char *base = last_component (pathname);
+
   state.curdepth = 0;
   state.starting_path_length = strlen (pathname);
 
-  if (0 == strcmp(pathname, parent_dir)
+  if (0 == *base
       || 0 == strcmp(parent_dir, "."))
     {
       dirchange = 0;
Index: find/parser.c
===================================================================
RCS file: /sources/findutils/findutils/find/parser.c,v
retrieving revision 1.131
diff -u -p -r1.131 parser.c
--- find/parser.c       24 Jul 2007 03:38:39 -0000      1.131
+++ find/parser.c       5 Aug 2007 03:16:56 -0000
@@ -1124,10 +1124,10 @@ fnmatch_sanitycheck(void)
 }
 
 
-static boolean 
+static boolean
 check_name_arg(const char *pred, const char *arg)
 {
-  if (strchr(arg, '/'))
+  if (strchr(arg, '/') && options.warnings)
     {
       error(0, 0,_("warning: Unix filenames usually don't contain slashes 
(though pathnames do).  That means that '%s %s' will probably evaluate to false 
all the time on this system.  You might find the '-wholename' test more useful, 
or perhaps '-samefile'.  Alternatively, if you are using GNU grep, you could 
use 'find ... -print0 | grep -FzZ %s'."),
            pred,
Index: find/pred.c
===================================================================
RCS file: /sources/findutils/findutils/find/pred.c,v
retrieving revision 1.104
diff -u -p -r1.104 pred.c
--- find/pred.c 2 Jul 2007 08:25:43 -0000       1.104
+++ find/pred.c 5 Aug 2007 03:16:57 -0000
@@ -741,7 +741,11 @@ do_fprintf(struct format_val *dest,
          break;
        case 'f':               /* base name of path */
          /* sanitised */
-         checked_print_quoted (dest, segment->text, base_name (pathname));
+         {
+           char *base = base_name (pathname);
+           checked_print_quoted (dest, segment->text, base);
+           free (base);
+         }
          break;
        case 'F':               /* file system type */
          /* trusted */
@@ -1150,20 +1154,38 @@ pred_ilname (const char *pathname, struc
   return match_lname (pathname, stat_buf, pred_ptr, true);
 }
 
-boolean
-pred_iname (const char *pathname, struct stat *stat_buf, struct predicate 
*pred_ptr)
+/* Common code between -name, -iname.  PATHNAME is being visited, STR
+   is name to compare basename against, and FLAGS are passed to
+   fnmatch.  */
+static boolean
+pred_name_common (const char *pathname, const char *str, int flags)
 {
-  const char *base;
-
-  (void) stat_buf;
-
+  /* Prefer last_component over base_name, to avoid malloc when
+     possible.  */
+  char *base = last_component (pathname);
+
+  /* base is empty only if pathname is a file system root.  But recall
+     that 'find / -name /' is one of the few times where a '/' in the
+     -name must actually find something.  */
+  if (!*base)
+    {
+      boolean b;
+      base = base_name (pathname);
+      b = fnmatch (str, base, flags) == 0;
+      free (base);
+      return b;
+    }
   /* FNM_PERIOD is not used here because POSIX requires that it not be.
    * See 
http://standards.ieee.org/reading/ieee/interp/1003-2-92_int/pasc-1003.2-126.html
    */
-  base = base_name (pathname);
-  if (fnmatch (pred_ptr->args.str, base, FNM_CASEFOLD) == 0)
-    return (true);
-  return (false);
+  return fnmatch (str, base, flags) == 0;
+}
+
+boolean
+pred_iname (const char *pathname, struct stat *stat_buf, struct predicate 
*pred_ptr)
+{
+  (void) stat_buf;
+  return pred_name_common (pathname, pred_ptr->args.str, FNM_CASEFOLD);
 }
 
 boolean
@@ -1271,17 +1293,8 @@ pred_mtime (const char *pathname, struct
 boolean
 pred_name (const char *pathname, struct stat *stat_buf, struct predicate 
*pred_ptr)
 {
-  const char *base;
-
   (void) stat_buf;
-  base = base_name (pathname);
-
-  /* FNM_PERIOD is not used here because POSIX requires that it not be.
-   * See 
http://standards.ieee.org/reading/ieee/interp/1003-2-92_int/pasc-1003.2-126.html
-   */
-  if (fnmatch (pred_ptr->args.str, base, 0) == 0)
-    return (true);
-  return (false);
+  return pred_name_common (pathname, pred_ptr->args.str, 0);
 }
 
 boolean
Index: find/testsuite/Makefile.am
===================================================================
RCS file: /sources/findutils/findutils/find/testsuite/Makefile.am,v
retrieving revision 1.45
diff -u -p -r1.45 Makefile.am
--- find/testsuite/Makefile.am  8 May 2007 09:37:41 -0000       1.45
+++ find/testsuite/Makefile.am  5 Aug 2007 03:16:57 -0000
@@ -37,6 +37,7 @@ find.gnu/lname.xo \
 find.gnu/mindepth-arg.xo \
 find.gnu/name-opt.xo \
 find.gnu/name-period.xo \
+find.gnu/name-slash.xo \
 find.gnu/path.xo \
 find.gnu/print_stdout.xo \
 find.gnu/perm.xo \
@@ -46,6 +47,7 @@ find.gnu/posix-dflt.xo \
 find.gnu/posix-h.xo \
 find.gnu/posix-l.xo \
 find.gnu/printfHdfl.xo \
+find.gnu/printf-slash.xo \
 find.gnu/printf-symlink.xo \
 find.gnu/printf-h.xo \
 find.gnu/printf.xo \
@@ -133,6 +135,7 @@ find.gnu/mindepth-arg.exp \
 find.gnu/mindepth-badarg.exp \
 find.gnu/name-opt.exp \
 find.gnu/name-period.exp \
+find.gnu/name-slash.exp \
 find.gnu/path.exp \
 find.gnu/print_stdout.exp \
 find.gnu/print0.exp \
@@ -145,6 +148,7 @@ find.gnu/posix-l.exp \
 find.gnu/printfHdfl.exp \
 find.gnu/printf.exp \
 find.gnu/printf.exp \
+find.gnu/printf-slash.exp \
 find.gnu/printf-symlink.exp \
 find.gnu/printf-h.exp \
 find.gnu/prune-default-print.exp \
Index: find/testsuite/find.gnu/name-slash.exp
===================================================================
RCS file: find/testsuite/find.gnu/name-slash.exp
diff -N find/testsuite/find.gnu/name-slash.exp
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ find/testsuite/find.gnu/name-slash.exp      5 Aug 2007 03:16:57 -0000
@@ -0,0 +1,2 @@
+# tests for '-name /'
+find_start p {/ /// -maxdepth 0 -name /}
Index: find/testsuite/find.gnu/name-slash.xo
===================================================================
RCS file: find/testsuite/find.gnu/name-slash.xo
diff -N find/testsuite/find.gnu/name-slash.xo
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ find/testsuite/find.gnu/name-slash.xo       5 Aug 2007 03:16:57 -0000
@@ -0,0 +1,2 @@
+/
+///
Index: find/testsuite/find.gnu/printf-slash.exp
===================================================================
RCS file: find/testsuite/find.gnu/printf-slash.exp
diff -N find/testsuite/find.gnu/printf-slash.exp
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ find/testsuite/find.gnu/printf-slash.exp    5 Aug 2007 03:16:57 -0000
@@ -0,0 +1 @@
+find_start p {/ /// -maxdepth 0 -printf "%p %f\\n"}
Index: find/testsuite/find.gnu/printf-slash.xo
===================================================================
RCS file: find/testsuite/find.gnu/printf-slash.xo
diff -N find/testsuite/find.gnu/printf-slash.xo
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ find/testsuite/find.gnu/printf-slash.xo     5 Aug 2007 03:16:57 -0000
@@ -0,0 +1,2 @@
+/ /
+/// /
Index: locate/locate.c
===================================================================
RCS file: /sources/findutils/findutils/locate/locate.c,v
retrieving revision 1.88
diff -u -p -r1.88 locate.c
--- locate/locate.c     22 Jul 2007 13:08:22 -0000      1.88
+++ locate/locate.c     5 Aug 2007 03:16:59 -0000
@@ -337,7 +337,7 @@ struct process_data
   int len;
   char *original_filename;     /* The current input database entry. */
   size_t pathsize;             /* Amount allocated for it.  */
-  char *munged_filename;       /* path or base_name(path) */
+  char *munged_filename;       /* path or basename(path) */
   FILE *fp;                    /* The pathname database.  */
   const char *dbfile;          /* Its name, or "<stdin>" */
   int  slocatedb_format;       /* Allows us to cope with slocate's format 
variant */
@@ -641,7 +641,7 @@ static int
 visit_basename(struct process_data *procdata, void *context)
 {
   (void) context;
-  procdata->munged_filename = base_name(procdata->original_filename);
+  procdata->munged_filename = last_component (procdata->original_filename);
 
   return VISIT_CONTINUE;
 }
@@ -1929,4 +1929,3 @@ main (int argc, char **argv)
   
   return dolocate(argc, argv, dbfd);
 }
-