commit ee9e43460f366406edff96b5abfb3ff33587e062 Author: Jim Meyering Date: Fri May 4 16:42:31 2012 +0200 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 Index: NEWS =================================================================== --- NEWS.orig 2012-05-07 14:20:52.079810454 +0200 +++ NEWS 2012-05-07 14:21:35.566738140 +0200 @@ -9,6 +9,14 @@ GNU coreutils NEWS changed, the new group ID would be listed, even though it is not yet effective. + '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_CREAT, 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_CREAT), + 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".] + * Noteworthy changes in release 8.16 (2012-03-26) [stable] Index: THANKS.in =================================================================== --- THANKS.in.orig 2012-05-07 14:20:52.079810454 +0200 +++ THANKS.in 2012-05-07 14:20:52.087810257 +0200 @@ -439,7 +439,7 @@ Minh Tran-Le tran 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 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 Index: src/copy.c =================================================================== --- src/copy.c.orig 2012-03-24 21:26:51.000000000 +0100 +++ src/copy.c 2012-05-07 14:20:52.087810257 +0200 @@ -889,6 +889,8 @@ copy_reg (char const *src_name, char con 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); @@ -939,6 +941,23 @@ copy_reg (char const *src_name, char con 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; Index: tests/Makefile.am =================================================================== --- tests/Makefile.am.orig 2012-05-07 14:20:52.080810429 +0200 +++ tests/Makefile.am 2012-05-07 14:20:52.087810257 +0200 @@ -347,6 +347,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 \ Index: tests/cp/nfs-removal-race =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ tests/cp/nfs-removal-race 2012-05-07 14:20:52.087810257 +0200 @@ -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 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 . + +. "${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 +#include + +#define __xstat __xstat_orig + +#include +#include + +#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