bug#11100: Racy code in copy.c

[Jim Meyering]

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.

>From b85446f612846ec9c99fe3e13dd78e861428ddde Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 4 May 2012 16:42:31 +0200
Subject: [PATCH] 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://debbugs.gnu.org/11100
---
 src/copy.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

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.456.g16798d0