bug#11100: Racy code in copy.c

[Jim Meyering]

Jim Meyering wrote:
> Philipp Thomas wrote:
>> * Jim Meyering (address@hidden) [20120328 18:09]:
>>
>>> At first glance, that might be reasonable: the additional open
>>> is incurred only after a failed stat.
>>> I'll look more closely in a week or two if no one else investigates.
>>
>> Ping, it's been more than two weeks and I'm being bugged for a possible
>> solution and will refrain from making changes that aren't accepted upstream.
>
> Thanks for the reminder.
> I did investigate it back then, but didn't make any progress.
> Today I looked again and saw the light ;-)
>
> Here's a partial patch.
> Still to come: a NEWS addition and a test case.

Here's that same patch, along with a glibc-specific LD_PRELOAD-based
test and a NEWS entry.

>From d8a631cb1a08ee73bde061d36b068585358ff6fe Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 4 May 2012 16:42:31 +0200
Subject: [PATCH 1/2] 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.
Reported by Philipp Thomas and Neil F. Brown in
http://bugs.gnu.org/11100
---
 THANKS.in  |  1 +
 src/copy.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/THANKS.in b/THANKS.in
index a7403fd..d8f7a4b 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -489,6 +489,7 @@ Phil Richards                       address@hidden
 Philippe De Muyter                  address@hidden
 Philippe Schnoebelen                address@hidden
 Phillip Jones                       address@hidden
+Philipp Thomas                      address@hidden
 Piergiorgio Sartor                  address@hidden
 Pieter Bowman                       address@hidden
 Piotr Gackiewicz                    address@hidden
diff --git a/src/copy.c b/src/copy.c
index 844ebcd..2558fea 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;
--
1.7.10.1.457.g8275905


>From 59bee5a6bfac96bba6684e740fdf35063b4461a8 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 5 May 2012 21:58:13 +0200
Subject: [PATCH 2/2] tests: test for just-fixed cp change

* tests/cp/nfs-removal-race: New file.
* tests/Makefile.am (TESTS): Add it.
* NEWS (Bug fixes): Mention it.
---
 NEWS                      |  6 ++++
 tests/Makefile.am         |  1 +
 tests/cp/nfs-removal-race | 70 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)
 create mode 100755 tests/cp/nfs-removal-race

diff --git a/NEWS b/NEWS
index 1c00d96..5e32f79 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,12 @@ GNU coreutils NEWS                                    -*- 
outline -*-
   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 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.
+
 ** New features

   fmt now accepts the --goal=WIDTH (-g) option.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 72717e3..ca19051 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 0000000..6a435b0
--- /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
--
1.7.10.1.457.g8275905