Re: [bug #34976] find: Failed to save working directory in order to [...]: Too many open files

[Bernhard Voelker]

On 01/07/2013 07:27 PM, Bernhard Voelker wrote:
> On 01/07/2013 05:20 PM, Aaron Burgemeister wrote:
>>   <http://savannah.gnu.org/bugs/?34976>
> 
> The following fixes the issue. Unfortunately, the test-suite is not
> very comprehensive, so I can't tell 100% if it is free of side effects.
> Comments?

Here comes v2 of the fix which includes a test for the failure.
It is also rebased against latest Git master.

Have a nice day,
Berny

>From 3d6cf9278aa122142fe12f236c28a8685e38d800 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <address@hidden>
Date: Sun, 3 Feb 2013 20:26:06 +0100
Subject: [PATCH] find: fix fd leak with --execdir option (bug#34976)

Prevent "Failed to save working dir[...]: Too many open files"
error by closing the file descriptor of the working directory.

* find/exec.c (impl_pred_exec): Free the working directory if find
executes the command in the local dir, i.e. if it has been saved
by record_exec_dir().  Re-indent code.
* find/testsuite/sv-34976-execdir-fd-leak.sh: Add test.
* find/testsuite/Makefile.am (test_shell_progs): Mention the test.
* NEWS: Mention the fix.
---
 NEWS                                       |    7 ++
 find/exec.c                                |   94 ++++++++++++++--------------
 find/testsuite/Makefile.am                 |    9 ++-
 find/testsuite/sv-34976-execdir-fd-leak.sh |   82 ++++++++++++++++++++++++
 4 files changed, 144 insertions(+), 48 deletions(-)
 create mode 100755 find/testsuite/sv-34976-execdir-fd-leak.sh

diff --git a/NEWS b/NEWS
index 184722a..526baa4 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,12 @@
 GNU findutils NEWS - User visible changes.      -*- outline -*- (allout)

+* Major changes in release 4.5.??, ????-??-??
+
+** Bug Fixes
+
+#34976: find -execdir leaks file descriptors for the working directory
+
+
 * Major changes in release 4.5.11, 2013-02-02

 ** Documentation Changes
diff --git a/find/exec.c b/find/exec.c
index aa69fe3..a191d36 100644
--- a/find/exec.c
+++ b/find/exec.c
@@ -112,8 +112,8 @@ record_exec_dir (struct exec_val *execp)

 bool
 impl_pred_exec (const char *pathname,
-               struct stat *stat_buf,
-               struct predicate *pred_ptr)
+                struct stat *stat_buf,
+                struct predicate *pred_ptr)
 {
   struct exec_val *execp = &pred_ptr->args.exec_vec;
   char *buf = NULL;
@@ -127,36 +127,36 @@ impl_pred_exec (const char *pathname,
   if (local)
     {
       /* For -execdir/-okdir predicates, the parser did not fill in
-        the wd_for_exec member of sturct exec_val.  So for those
-        predicates, we do so now.
+         the wd_for_exec member of sturct exec_val.  So for those
+         predicates, we do so now.
       */
       if (!record_exec_dir (execp))
-       {
-         error (EXIT_FAILURE, errno,
-                _("Failed to save working directory in order to "
-                  "run a command on %s"),
-                safely_quote_err_filename (0, pathname));
-         /*NOTREACHED*/
-       }
+        {
+          error (EXIT_FAILURE, errno,
+                 _("Failed to save working directory in order to "
+                   "run a command on %s"),
+                 safely_quote_err_filename (0, pathname));
+          /*NOTREACHED*/
+        }
       target = buf = base_name (state.rel_pathname);
       if ('/' == target[0])
-       {
-         /* find / execdir ls -d {} \; */
-         prefix = NULL;
-         pfxlen = 0;
-       }
+        {
+          /* find / execdir ls -d {} \; */
+          prefix = NULL;
+          pfxlen = 0;
+        }
       else
-       {
-         prefix = "./";
-         pfxlen = 2u;
-       }
+        {
+          prefix = "./";
+          pfxlen = 2u;
+        }
     }
   else
     {
       /* For the others (-exec, -ok), the parser should
-        have set wd_for_exec to initial_wd, indicating
-        that the exec should take place from find's initial
-        working directory.
+         have set wd_for_exec to initial_wd, indicating
+         that the exec should take place from find's initial
+         working directory.
       */
       assert (execp->wd_for_exec == initial_wd);
       target = pathname;
@@ -171,14 +171,14 @@ impl_pred_exec (const char *pathname,
        * depending on the command line length limits.
        */
       bc_push_arg (&execp->ctl,
-                  &execp->state,
-                  target, strlen (target)+1,
-                  prefix, pfxlen,
-                  0);
+                   &execp->state,
+                   target, strlen (target)+1,
+                   prefix, pfxlen,
+                   0);

       /* remember that there are pending execdirs. */
       if (execp->state.todo)
-       state.execdirs_outstanding = true;
+        state.execdirs_outstanding = true;

       /* POSIX: If the primary expression is punctuated by a plus
        * sign, the primary shall always evaluate as true
@@ -190,29 +190,31 @@ impl_pred_exec (const char *pathname,
       int i;

       for (i=0; i<execp->num_args; ++i)
-       {
-         bc_do_insert (&execp->ctl,
-                       &execp->state,
-                       execp->replace_vec[i],
-                       strlen (execp->replace_vec[i]),
-                       prefix, pfxlen,
-                       target, strlen (target),
-                       0);
-       }
+        {
+          bc_do_insert (&execp->ctl,
+                        &execp->state,
+                        execp->replace_vec[i],
+                        strlen (execp->replace_vec[i]),
+                        prefix, pfxlen,
+                        target, strlen (target),
+                        0);
+        }

       /* Actually invoke the command. */
       bc_do_exec (&execp->ctl, &execp->state);
       if (WIFEXITED(execp->last_child_status))
-       {
-         if (0 == WEXITSTATUS(execp->last_child_status))
-           result = true;      /* The child succeeded. */
-         else
-           result = false;
-       }
+        {
+          if (0 == WEXITSTATUS(execp->last_child_status))
+            result = true;        /* The child succeeded. */
+          else
+            result = false;
+        }
       else
-       {
-         result = false;
-       }
+        {
+          result = false;
+        }
+      if (local)
+        free_cwd (execp->wd_for_exec);
     }
   if (buf)
     {
diff --git a/find/testsuite/Makefile.am b/find/testsuite/Makefile.am
index a1e49b8..71c82e3 100644
--- a/find/testsuite/Makefile.am
+++ b/find/testsuite/Makefile.am
@@ -246,8 +246,13 @@ find.posix/user-missing.exp
 EXTRA_DIST_GOLDEN = \
        test_escapechars.golden

-test_shell_progs = sv-bug-32043.sh test_escapechars.sh test_escape_c.sh \
-       test_inode.sh sv-34079.sh
+test_shell_progs = \
+sv-bug-32043.sh \
+test_escapechars.sh \
+test_escape_c.sh \
+test_inode.sh \
+sv-34079.sh \
+sv-34976-execdir-fd-leak.sh

 EXTRA_DIST = $(EXTRA_DIST_EXP) $(EXTRA_DIST_XO) $(EXTRA_DIST_GOLDEN) \
        $(test_shell_progs) binary_locations.sh
diff --git a/find/testsuite/sv-34976-execdir-fd-leak.sh 
b/find/testsuite/sv-34976-execdir-fd-leak.sh
new file mode 100755
index 0000000..f22ef80
--- /dev/null
+++ b/find/testsuite/sv-34976-execdir-fd-leak.sh
@@ -0,0 +1,82 @@
+#! /bin/sh
+# Copyright (C) 2013 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
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# This test verifies that find does not leak a file descriptor for the working
+# directory specified by the -execdir option [Savannah bug #34976].
+
+testname="$(basename $0)"
+
+. "${srcdir}"/binary_locations.sh
+
+# Test if restricting the number of file descriptors via ulimit -n works.
+test_ulimit() {
+  n="$1"  # number of files
+  l="$2"  # limit to use
+  (
+    ulimit -n "$l" || exit 1
+    for i in $(seq $n) ; do
+      printf "exec %d> /dev/null || exit 1\n" $i
+    done | sh ;
+  ) 2>/dev/null
+}
+# Opening 30 files with a limit of 40 should work.
+test_ulimit 30 40 || { echo "SKIP: ulimit does not work" >&2; exit 0 ; }
+# Opening 30 files with a limit of 20 should fail.
+test_ulimit 30 20 && { echo "SKIP: ulimit does not work" >&2; exit 0 ; }
+
+die() {
+  echo "$@" >&2
+  exit 1
+}
+
+# Create test files, each 100 in the directories ".", "one" and "two".
+make_test_data() {
+  d="$1"
+  (
+    cd "$1" || exit 1
+    {
+      for j in $(seq 0 100) ; do
+        printf "%03d " $i
+      done
+      for i in one two ; do
+        mkdir $i
+        for j in $(seq 0 100) ; do
+          printf "%s/%03d " $i $j
+        done
+      done
+    } | xargs sh -c 'touch "$@" || exit 255' fnord || exit 1
+  ) \
+  || die "failed to set up the test in ${outdir}"
+}
+
+outdir="$(mktemp -d)" || die "FAIL: could not create a test files."
+
+# Create some test files.
+make_test_data "${outdir}" || die "FAIL: failed to set up the test in 
${outdir}"
+
+fail=0
+for exe in "${ftsfind}" "${oldfind}"; do
+  ( ulimit -n 30 && \
+    ${exe} "${outdir}"  -type f -execdir cat '{}' \; >/dev/null; ) \
+  || { \
+    echo "Option -execdir of ${exe} leaks file descriptors" >&2 ; \
+    fail=1 ; \
+  }
+done
+
+rm -rf "${outdir}" || exit 1
+exit $fail
-- 
1.7.7