On 02/03/2013 08:27 PM, Bernhard Voelker wrote: Hi James,
I rebased my previous version of the patch against git HEAD, and simplified a function in the test; see attached. Have a nice day, Berny
>From 8e239cf781baa4d0e42457ff4737b4518db229cb Mon Sep 17 00:00:00 2001 From: Bernhard Voelker <m...@bernhard-voelker.de> Date: Fri, 2 Aug 2013 12:16:44 +0200 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 | 3 + find/exec.c | 94 +++++++++++++++--------------- find/testsuite/Makefile.am | 9 ++- find/testsuite/sv-34976-execdir-fd-leak.sh | 76 ++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 48 deletions(-) create mode 100755 find/testsuite/sv-34976-execdir-fd-leak.sh diff --git a/NEWS b/NEWS index 4349a21..2fbc5da 100644 --- a/NEWS +++ b/NEWS @@ -43,6 +43,9 @@ https://savannah.gnu.org/bugs/?group=findutils: #36652: Better document that -0/-d turns off the effect of -E. +#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..2d5dace --- /dev/null +++ b/find/testsuite/sv-34976-execdir-fd-leak.sh @@ -0,0 +1,76 @@ +#! /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 + mkdir one two || exit 1 + for i in $(seq 0 100) ; do + printf "./%03d one/%03d two/%03d " $i $i $i + done \ + | xargs touch || 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.8.3.1
