patch for a race condition and a related bug in cp

[Paul Eggert]

Here's a patch to fix a couple of bugs in 'cp'.  In re-reviewing this
patch I see that I could split it into two patches if you like, since
the bugs are separably fixable.  But for now I thought I'd just get
this out the door.

2007-02-02  Paul Eggert  <address@hidden>

        * NEWS: Document fixes to cp --parents and cp --preserve=mode.
        * src/copy.c (copy_internal): Omit the group- or other-writeable
        permissions when creating a directory, to avoid a race condition
        if the special mode bits aren't right just after the directory is
        created.
        * src/cp.c (make_dir_parents_private): Report an error with "cp
        --parents DIR/FILE DEST" and DIR turns out to be a non-directory.
        * tests/cp/cp-parents: Test for the non-race-condition bug fixed
        by the above change.

diff --git a/NEWS b/NEWS
index ad0fd1d..bd307c1 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,16 @@ GNU coreutils NEWS                                    -*- 
outline -*-
   chmod no longer fails in an environment (e.g., a chroot) with openat
   support but with insufficient /proc support.

+  "cp --parents F/G D" no longer creates a directory D/F when F is not
+  a directory (and F/G is therefore invalid).
+
+  "cp --preserve=mode" would create directories that briefly had
+  too-generous permissions in some cases.  For example, when copying a
+  directory with permissions 777 the destination directory might
+  temporarily be setgid on some file systems, which would allow other
+  users to create subfiles with the same group as the directory.  Fix
+  similar problems with 'install' and 'mv'.
+
   cut no longer dumps core for usage like "cut -f2- f1 f2" with two or
   more file arguments.  This was due to a double-free bug, introduced
   in coreutils-5.3.0.
diff --git a/src/copy.c b/src/copy.c
index d9ad254..5ec5a92 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1,5 +1,5 @@
 /* copy.c -- core functions for copying files and directories
-   Copyright (C) 89, 90, 91, 1995-2006 Free Software Foundation.
+   Copyright (C) 89, 90, 91, 1995-2007 Free Software Foundation.

    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
@@ -1005,6 +1005,7 @@ copy_internal (char const *src_name, char const *dst_name,
   struct stat dst_sb;
   mode_t src_mode;
   mode_t dst_mode IF_LINT (= 0);
+  mode_t dst_mode_bits;
   mode_t omitted_permissions;
   bool restore_dst_mode = false;
   char *earlier_file = NULL;
@@ -1504,13 +1505,16 @@ copy_internal (char const *src_name, char const 
*dst_name,
       new_dst = true;
     }

-  /* If the ownership might change, omit some permissions at first, so
-     unauthorized users cannot nip in before the file has the right
-     ownership.  */
+  /* If the ownership might change, or if it is a directory (whose
+     special mode bits may change after the directory is created),
+     omit some permissions at first, so unauthorized users cannot nip
+     in before the file is ready.  */
+  dst_mode_bits = (x->set_mode ? x->mode : src_mode) & CHMOD_MODE_BITS;
   omitted_permissions =
-    (x->preserve_ownership
-     ? (x->set_mode ? x->mode : src_mode) & (S_IRWXG | S_IRWXO)
-     : 0);
+    (dst_mode_bits
+     & (x->preserve_ownership ? S_IRWXG | S_IRWXO
+       : S_ISDIR (src_mode) ? S_IWGRP | S_IWOTH
+       : 0));

   delayed_ok = true;

@@ -1549,10 +1553,7 @@ copy_internal (char const *src_name, char const 
*dst_name,
             (src_mode & ~S_IRWXUGO) != 0.  However, common practice is
             to ask mkdir to copy all the CHMOD_MODE_BITS, letting mkdir
             decide what to do with S_ISUID | S_ISGID | S_ISVTX.  */
-         mode_t mkdir_mode =
-           ((x->set_mode ? x->mode : src_mode)
-            & CHMOD_MODE_BITS & ~omitted_permissions);
-         if (mkdir (dst_name, mkdir_mode) != 0)
+         if (mkdir (dst_name, dst_mode_bits & ~omitted_permissions) != 0)
            {
              error (0, errno, _("cannot create directory %s"),
                     quote (dst_name));
diff --git a/src/cp.c b/src/cp.c
index 8fe11a1..5759e0d 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -1,5 +1,5 @@
 /* cp.c  -- file copying (main routines)
-   Copyright (C) 89, 90, 91, 1995-2006 Free Software Foundation.
+   Copyright (C) 89, 90, 91, 1995-2007 Free Software Foundation.

    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
@@ -415,6 +415,7 @@ make_dir_parents_private (char const *const_dir, size_t 
src_offset,
              mode_t src_mode;
              mode_t omitted_permissions;
              mode_t mkdir_mode;
+             int src_errno;

              /* This component does not exist.  We must set
                 *new_dst and new->mode inside this loop because,
@@ -422,15 +423,28 @@ make_dir_parents_private (char const *const_dir, size_t 
src_offset,
                 make_dir_parents_private creates only e_dir/../a if
                 ./b already exists. */
              *new_dst = true;
-             if (XSTAT (x, src, &stats))
+             src_errno = (XSTAT (x, src, &stats) != 0
+                          ? errno
+                          : S_ISDIR (stats.st_mode)
+                          ? 0
+                          : ENOTDIR);
+             if (src_errno)
                {
-                 error (0, errno, _("failed to get attributes of %s"),
+                 error (0, src_errno, _("failed to get attributes of %s"),
                         quote (src));
                  return false;
                }
              src_mode = stats.st_mode;
-             omitted_permissions =
-               x->preserve_ownership ? src_mode & (S_IRWXG | S_IRWXO) : 0;
+
+             /* If the ownership or special mode bits might change,
+                omit some permissions at first, so unauthorized users
+                cannot nip in before the file is ready.  */
+             omitted_permissions = (src_mode
+                                    & (x->preserve_ownership
+                                       ? S_IRWXG | S_IRWXO
+                                       : x->preserve_mode
+                                       ? S_IWGRP | S_IWOTH
+                                       : 0));

              /* POSIX says mkdir's behavior is implementation-defined when
                 (src_mode & ~S_IRWXUGO) != 0.  However, common practice is
diff --git a/tests/cp/cp-parents b/tests/cp/cp-parents
index 373e607..6c123d2 100755
--- a/tests/cp/cp-parents
+++ b/tests/cp/cp-parents
@@ -2,7 +2,8 @@
 # cp -R --parents dir-specified-with-trailing-slash/ other-dir
 # would get a failed assertion.

-# Copyright (C) 2000, 2002, 2004, 2005, 2006 Free Software Foundation, Inc.
+# Copyright (C) 2000, 2002, 2004, 2005, 2006, 2007 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
@@ -48,6 +49,7 @@ cd $tmp || framework_failure=1

 mkdir foo bar || framework_failure=1
 mkdir -p a/b/c d e || framework_failure=1
+touch f || framework_failure=1

 if test $framework_failure = 1; then
   echo 'failure in testing framework'
@@ -65,6 +67,11 @@ cp -R --parents foo/ bar || fail=1
 cp --verbose -a --parents a/b/c d > /dev/null 2>&1 || fail=1
 test -d d/a/b/c || fail=1

+# With 6.7 and earlier, cp --parents f/g d would mistakenly create a
+# directory d/f, even though f is a regular file.
+cp --parents f/g d 2>/dev/null && fail=1
+test -d d/f && fail=1
+
 # Check that re_protect works.
 chmod go=w d/a
 cp -a --parents d/a/b/c e || fail=1
M ChangeLog
M NEWS
M src/copy.c
M src/cp.c
M tests/cp/cp-parents
Committed as 048979f0a7f4e38078d9cc4f94e24a0da8ba1ae1