summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJim Meyering <meyering@redhat.com>2012-05-04 16:42:31 +0200
committerJim Meyering <meyering@redhat.com>2012-05-07 13:39:48 +0200
commitee9e43460f366406edff96b5abfb3ff33587e062 (patch)
tree7451920817eb2c42bcc4aee062074f46e2f9d041
parent3468d26884800d7bc6fcc1ba52cd481175c655d8 (diff)
downloadcoreutils-ee9e43460f366406edff96b5abfb3ff33587e062.tar.xz
cp: handle a race condition more sensibly
* src/copy.c (copy_reg): In a narrow race (stat sees dest, yet open-without-O_CREAT fails with ENOENT), retry the open with O_CREAT. * tests/cp/nfs-removal-race: New file. * tests/Makefile.am (TESTS): Add it. * NEWS (Bug fixes): Mention it. Reported by Philipp Thomas and Neil F. Brown in http://bugs.gnu.org/11100
-rw-r--r--NEWS8
-rw-r--r--THANKS.in3
-rw-r--r--src/copy.c19
-rw-r--r--tests/Makefile.am1
-rwxr-xr-xtests/cp/nfs-removal-race70
5 files changed, 100 insertions, 1 deletions
diff --git a/NEWS b/NEWS
index 8e42c6326..fd563c050 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,14 @@ GNU coreutils NEWS -*- outline -*-
changed, the new group ID would be listed, even though it is not
yet effective. [bug introduced in coreutils-8.1]
+ cp S D is no longer subject to a race: if an existing D were removed
+ between the initial stat and subsequent open-without-O_CREATE, cp would
+ fail with a confusing diagnostic saying that the destination, D, was not
+ found. Now, in this unusual case, it retries the open (but with O_CREATE),
+ and hence usually succeeds. With NFS attribute caching, the condition
+ was particularly easy to trigger, since there, the removal of D could
+ precede the initial stat. [This bug was present in "the beginning".]
+
** New features
fmt now accepts the --goal=WIDTH (-g) option.
diff --git a/THANKS.in b/THANKS.in
index a7403fdf3..5c7dde826 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -439,7 +439,7 @@ Minh Tran-Le tranle@intellicorp.com
Morten Welinder terra@diku.dk
Nao Nishijima nao.nishijima.xt@hitachi.com
Neal H Walfield neal@cs.uml.edu
-Neil Brown neilb@cse.unsw.edu.au
+Neil F. Brown neilb@suse.de
Nelson H. F. Beebe beebe@math.utah.edu
Nick Estes debian@nickstoys.com
Nick Graham nick.d.graham@gmail.com
@@ -489,6 +489,7 @@ Phil Richards phil.richards@vf.vodafone.co.uk
Philippe De Muyter phdm@macqel.be
Philippe Schnoebelen Philippe.Schnoebelen@imag.fr
Phillip Jones mouse@datastacks.com
+Philipp Thomas pth@suse.de
Piergiorgio Sartor sartor@sony.de
Pieter Bowman bowman@math.utah.edu
Piotr Gackiewicz gacek@intertele.pl
diff --git a/src/copy.c b/src/copy.c
index 844ebcd78..2558fea14 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -892,6 +892,8 @@ copy_reg (char const *src_name, char const *dst_name,
if (*new_dst)
{
+ open_with_O_CREAT:;
+
int open_flags = O_WRONLY | O_CREAT | O_BINARY;
dest_desc = open (dst_name, open_flags | O_EXCL,
dst_mode & ~omitted_permissions);
@@ -942,6 +944,23 @@ copy_reg (char const *src_name, char const *dst_name,
if (dest_desc < 0)
{
+ /* If we've just failed due to ENOENT for an ostensibly preexisting
+ destination (*new_dst was 0), that's a bit of a contradiction/race:
+ the prior stat/lstat said the file existed (*new_dst was 0), yet
+ the subsequent open-existing-file failed with ENOENT. With NFS,
+ the race window is wider still, since its meta-data caching tends
+ to make the stat succeed for a just-removed remote file, while the
+ more-definitive initial open call will fail with ENOENT. When this
+ situation arises, we attempt to open again, but this time with
+ O_CREAT. Do this only when not in move-mode, since when handling
+ a cross-device move, we must never open an existing destination. */
+ if (dest_errno == ENOENT && ! *new_dst && ! x->move_mode)
+ {
+ *new_dst = 1;
+ goto open_with_O_CREAT;
+ }
+
+ /* Otherwise, it's an error. */
error (0, dest_errno, _("cannot create regular file %s"),
quote (dst_name));
return_val = false;
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 72717e318..ca190517b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -349,6 +349,7 @@ TESTS = \
cp/link-no-deref \
cp/link-preserve \
cp/link-symlink \
+ cp/nfs-removal-race \
cp/no-deref-link1 \
cp/no-deref-link2 \
cp/no-deref-link3 \
diff --git a/tests/cp/nfs-removal-race b/tests/cp/nfs-removal-race
new file mode 100755
index 000000000..6a435b0c5
--- /dev/null
+++ b/tests/cp/nfs-removal-race
@@ -0,0 +1,70 @@
+#!/bin/sh
+# Running cp S D on an NFS client while another client has just removed D
+# would lead (w/coreutils-8.16 and earlier) to cp's initial stat call
+# seeing (via stale NFS cache) that D exists, so that cp would then call
+# open without the O_CREAT flag. Yet, the open must actually consult
+# the server, which confesses that D has been deleted, thus causing the
+# open call to fail with ENOENT.
+#
+# This test simulates that situation by intercepting stat for a nonexistent
+# destination, D, and making the stat fill in the result struct for another
+# file and return 0.
+#
+# This test is skipped on systems that lack LD_PRELOAD support; that's fine.
+# Similarly, on a system that lacks <dlfcn.h> or __xstat, skipping it is fine.
+
+# Copyright (C) 2012 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/>.
+
+. "${srcdir=.}/init.sh"; path_prepend_ ../src
+print_ver_ cp
+
+# Replace each stat call with a call to this wrapper.
+cat > k.c <<'EOF' || framework_failure_
+#define _GNU_SOURCE
+#include <sys/types.h>
+#include <dlfcn.h>
+
+#define __xstat __xstat_orig
+
+#include <sys/stat.h>
+#include <stddef.h>
+
+#undef __xstat
+
+int
+__xstat (int ver, const char *path, struct stat *st)
+{
+ static int (*real_stat)(int ver, const char *path, struct stat *st) = NULL;
+ if (!real_stat)
+ real_stat = dlsym (RTLD_NEXT, "__xstat");
+ /* When asked to stat nonexistent "d",
+ return results suggesting it exists. */
+ return real_stat (ver, *path == 'd' && path[1] == 0 ? "d2" : path, st);
+}
+EOF
+
+# Then compile/link it:
+$CC -shared -fPIC -O2 k.c -o k.so \
+ || framework_failure_ 'failed to compile with -shared -fPIC'
+
+touch d2 || framework_failure_
+echo xyz > src || framework_failure_
+
+# Finally, run the test:
+LD_PRELOAD=./k.so cp src d || fail=1
+
+compare src d || fail=1
+Exit $fail