Re: [PATCH] Fix preserve_mode when destination directory partially exists

[Jim Meyering]

Jan Blunck <address@hidden> wrote:

> On Fri, Jan 04, Paul Eggert wrote:
>
>> Jan Blunck <address@hidden> writes:
>>
>> > I found a bug with cp -p --parents when the destination partially exists 
>> > and
>> > the filesystem isn't mounted with acls.
>> >
>> > $ mkdir -p a/b/c a/b/d e
>> > $ touch a/b/c/foo a/b/d/foo
>> > $ cp -p --parent a/b/c e
>> > $ cp -p --parent a/b/d e
>> > $ ls -ld e/a
>> > d--------- 3 jblunck suse 4096 1970-01-01 01:00 e/a
>>
>> I can't reproduce the problem on my Debian stable host, using
>> coreutils 6.9.91:
>>
>>   $ mkdir -p a/b/c a/b/d e
>>   $ touch a/b/c/foo a/b/d/foo
>>   $ cp -p --parent a/b/c e
>>   cp: omitting directory `a/b/c'
>
> Oh, my testcase should read as follows
> $ cp -p --parent a/b/c/foo e
> $ cp -p --parent a/b/d/foo e

Ah.  Now I see.
Thanks for the report and patch.
I've added a test based on the above and applied your patch:

2008-01-07  Jim Meyering  <address@hidden>

        * NEWS: Mention the cp bug fix.

2008-01-07  Jan Blunck  <address@hidden>

        cp --parents: don't use uninitialized memory when restoring permissions
        * src/cp.c (make_dir_parents_private): Always stat each source
        directory, in case its permissions are required in re_protect,
        when setting permissions of a just-created destination directory.

2008-01-07  Jim Meyering  <address@hidden>

        cp: add a test for today's bug fix.
        * tests/cp/parent-perm: New script.  Test today's change.
        Based on reproducer from Jan Blunck.
        * tests/cp/Makefile.am (TESTS): Add parent-perm.

>From db58094e11adb527c31b3510d3430123a4048686 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 7 Jan 2008 11:57:27 +0100
Subject: [PATCH] cp: add a test for today's bug fix.

* tests/cp/parent-perm: New script.  Test today's change.
Based on reproducer from Jan Blunck.
* tests/cp/Makefile.am (TESTS): Add parent-perm.

---
 ChangeLog            |    7 +++++++
 tests/cp/Makefile.am |    3 ++-
 tests/cp/parent-perm |   38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 1 deletions(-)
 create mode 100755 tests/cp/parent-perm

diff --git a/ChangeLog b/ChangeLog
index 85dc1c1..29ae607 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2008-01-07  Jim Meyering  <address@hidden>
+
+       cp: add a test for today's bug fix.
+       * tests/cp/parent-perm: New script.  Test today's change.
+       Based on reproducer from Jan Blunck.
+       * tests/cp/Makefile.am (TESTS): Add parent-perm.
+
 2008-01-06  Jim Meyering  <address@hidden>

        touch: add a test for today's change.
diff --git a/tests/cp/Makefile.am b/tests/cp/Makefile.am
index 4af269c..e2f96c9 100644
--- a/tests/cp/Makefile.am
+++ b/tests/cp/Makefile.am
@@ -1,6 +1,6 @@
 # Make coreutils tests for cp.                         -*-Makefile-*-

-# Copyright (C) 1997-2001, 2003, 2005-2007 Free Software Foundation, Inc.
+# Copyright (C) 1997-2001, 2003, 2005-2008 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
@@ -16,6 +16,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.

 TESTS = \
+  parent-perm \
   abuse \
   proc-zero-len \
   thru-dangling \
diff --git a/tests/cp/parent-perm b/tests/cp/parent-perm
new file mode 100755
index 0000000..1c7a222
--- /dev/null
+++ b/tests/cp/parent-perm
@@ -0,0 +1,38 @@
+#!/bin/sh
+# Ensure that cp --parents works properly with a preexisting dest. directory
+
+# Copyright (C) 2008 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/>.
+
+if test "$VERBOSE" = yes; then
+  set -x
+  cp --version
+fi
+
+. $srcdir/../envvar-check
+. $srcdir/../test-lib.sh
+
+mkdir -p a/b/c a/b/d e || framework_failure
+touch a/b/c/foo a/b/d/foo || framework_failure
+cp -p --parent a/b/c/foo e || framework_failure
+
+fail=0
+cp -p --parent a/b/d/foo e || fail=1
+
+# Ensure that permissions on just-created directory, e/a/,
+# are the same as those on original, a/.
+test $(stat --printf %A a) = $(stat --printf %A e/a) || fail=1
+
+(exit $fail); exit $fail
--
1.5.4.rc2.61.g6ed4e


>From a54e8bb8a547c2ee9147865e2eb42eece69e8072 Mon Sep 17 00:00:00 2001
From: Jan Blunck <address@hidden>
Date: Mon, 7 Jan 2008 12:13:42 +0100
Subject: [PATCH] cp --parents: don't use uninitialized memory when restoring 
permissions

* src/cp.c (make_dir_parents_private): Always stat each source
directory, in case its permissions are required in re_protect,
when setting permissions of a just-created destination directory.

---
 ChangeLog |    7 +++++++
 src/cp.c  |   24 ++++++++++++------------
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 29ae607..435ef43 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2008-01-07  Jan Blunck  <address@hidden>
+
+       cp --parents: don't use uninitialized memory when restoring permissions
+       * src/cp.c (make_dir_parents_private): Always stat each source
+       directory, in case its permissions are required in re_protect,
+       when setting permissions of a just-created destination directory.
+
 2008-01-07  Jim Meyering  <address@hidden>

        cp: add a test for today's bug fix.
diff --git a/src/cp.c b/src/cp.c
index 99e1f73..be3701f 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -403,6 +403,7 @@ make_dir_parents_private (char const *const_dir, size_t 
src_offset,
        slash++;
       while ((slash = strchr (slash, '/')))
        {
+         int src_errno;
          /* Add this directory to the list of directories whose modes need
             fixing later. */
          struct dir_attr *new = xmalloc (sizeof *new);
@@ -412,12 +413,22 @@ make_dir_parents_private (char const *const_dir, size_t 
src_offset,
          *attr_list = new;

          *slash = '\0';
+         src_errno = (stat (src, &new->st) != 0
+                      ? errno
+                      : S_ISDIR (new->st.st_mode)
+                      ? 0
+                      : ENOTDIR);
+         if (src_errno)
+           {
+             error (0, src_errno, _("failed to get attributes of %s"),
+                    quote (src));
+             return false;
+           }
          if (stat (dir, &stats) != 0)
            {
              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->st.st_mode inside this loop because,
@@ -425,17 +436,6 @@ 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;
-             src_errno = (stat (src, &new->st) != 0
-                          ? errno
-                          : S_ISDIR (new->st.st_mode)
-                          ? 0
-                          : ENOTDIR);
-             if (src_errno)
-               {
-                 error (0, src_errno, _("failed to get attributes of %s"),
-                        quote (src));
-                 return false;
-               }
              src_mode = new->st.st_mode;

              /* If the ownership or special mode bits might change,
--
1.5.4.rc2.61.g6ed4e


>From ab1c9b54b173312c132c9452eff6080a5b4cf6e7 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 7 Jan 2008 12:17:52 +0100
Subject: [PATCH] NEWS: Mention the cp bug fix.


---
 ChangeLog |    4 ++++
 NEWS      |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 435ef43..676f33e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2008-01-07  Jim Meyering  <address@hidden>
+
+       * NEWS: Mention the cp bug fix.
+
 2008-01-07  Jan Blunck  <address@hidden>

        cp --parents: don't use uninitialized memory when restoring permissions
diff --git a/NEWS b/NEWS
index 542e5f2..ca3bbc8 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,10 @@ GNU coreutils NEWS                                    -*- 
outline -*-

 ** Bug fixes

+  cp --parents no longer uses uninitialized memory when restoring the
+  permissions of a just-created destination directory.
+  [bug introduced in coreutils-6.9.90]
+
   tr's case conversion would fail in a locale with differing numbers
   of lower case and upper case characters.  E.g., this would fail:
   env LC_CTYPE=en_US.iso88591 tr '[:upper:]' '[:lower:]'
--
1.5.4.rc2.61.g6ed4e