GNU bug report logs - #9170
[PATCH] cp "restores" permissions it never set

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 9170 in the body.
You can then email your comments to 9170 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eric Lammerts <eric <at> lammerts.org>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] cp "restores" permissions it never set
Date: Mon, 25 Jul 2011 14:50:50 -0400 (EDT)

Hi,
I'm seeing the following strange behavior:

$ umask
0002
$ rm -rf /tmp/src /tmp/dst
$ mkdir -m775 /tmp/src /tmp/src/foo
$ mkdir -m700 /tmp/dst /tmp/dst/foo
$ ls -ld /tmp/dst/foo
drwx------ 2 eric eric 4096 Jul 25 13:40 /tmp/dst/foo
$ cp -r /tmp/src/. /tmp/dst/
$ ls -ld /tmp/dst/foo
drwx-w---- 2 eric eric 4096 Jul 25 13:40 /tmp/dst/foo

Patch below fixes this.

cheers,

Eric

diff --git a/src/copy.c b/src/copy.c
index 65566a0..b9c0ccf 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2197,6 +2197,8 @@ copy_internal (char const *src_name, char const *dst_name,
           if (x->verbose)
             emit_verbose (src_name, dst_name, NULL);
         }
+      else
+        omitted_permissions = 0;

       /* Decide whether to copy the contents of the directory.  */
       if (x->one_file_system && device != 0 && device != src_sb.st_dev)

Message #10 received at 9170-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eric Lammerts <eric <at> lammerts.org>
Cc: 9170-done <at> debbugs.gnu.org
Subject: Re: bug#9170: [PATCH] cp "restores" permissions it never set
Date: Mon, 25 Jul 2011 14:06:26 -0700

Thanks for the bug report.  I committed this patch:

From 3960352798e1815e1682a724f6bd9b02677937d1 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Mon, 25 Jul 2011 13:36:16 -0700
Subject: [PATCH] cp: don't mishandle existing dir dest permissions (Bug#9170)

* src/copy.c (copy_internal): If we don't create the directory,
then we cannot have omitted permissions.  Problem and trivial
fix reported by Eric Lammerts.
* tests/Makefile.am (TESTS): Add cp/existing-perm-dir.
* tests/cp/existing-perm-dir: New file.
---
 src/copy.c                 |    2 ++
 tests/Makefile.am          |    1 +
 tests/cp/existing-perm-dir |   31 +++++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 0 deletions(-)
 create mode 100755 tests/cp/existing-perm-dir

diff --git a/src/copy.c b/src/copy.c
index df8b1db..b2aeb6e 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2211,6 +2211,8 @@ copy_internal (char const *src_name, char const *dst_name,
           if (x->verbose)
             emit_verbose (src_name, dst_name, NULL);
         }
+      else
+        omitted_permissions = 0;
 
       /* Decide whether to copy the contents of the directory.  */
       if (x->one_file_system && device != 0 && device != src_sb.st_dev)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 0a83dae..eb67512 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -323,6 +323,7 @@ TESTS =						\
   cp/dir-rm-dest				\
   cp/dir-slash					\
   cp/dir-vs-file				\
+  cp/existing-perm-dir				\
   cp/existing-perm-race				\
   cp/fail-perm					\
   cp/fiemap-empty                               \
diff --git a/tests/cp/existing-perm-dir b/tests/cp/existing-perm-dir
new file mode 100755
index 0000000..6ef73f0
--- /dev/null
+++ b/tests/cp/existing-perm-dir
@@ -0,0 +1,31 @@
+#!/bin/sh
+# Make sure cp -p doesn't "restore" permissions it shouldn't (Bug#9170).
+
+# Copyright 2011 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
+
+umask 002
+mkdir -p -m ug-s,u=rwx,g=rwx,o=rx src/dir || fail=1
+mkdir -p -m ug-s,u=rwx,g=,o= dst/dir || fail=1
+
+cp -r src/. dst/ || fail=1
+
+mode=`ls -ld dst/dir | cut -b-10`
+test "$mode" = drwx------ || fail=1
+
+Exit $fail
-- 
1.7.4.4

Message #13 received at 9170 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: 9170 <at> debbugs.gnu.org, eggert <at> cs.ucla.edu
Subject: Re: bug#9170: [PATCH] cp "restores" permissions it never set
Date: Mon, 25 Jul 2011 15:21:33 -0600

On 07/25/2011 03:06 PM, Paul Eggert wrote:
> Thanks for the bug report.  I committed this patch:
>
> +++ b/src/copy.c
> @@ -2211,6 +2211,8 @@ copy_internal (char const *src_name, char const *dst_name,
>             if (x->verbose)
>               emit_verbose (src_name, dst_name, NULL);
>           }
> +      else
> +        omitted_permissions = 0;

This violates the style guide in HACKING; it should either be:

if (!cond)
  omitted_permissions = 0;
else
  {
    lots of lines
  }

or:

if (cond)
  {
     lots of lines
  }
else
  {
    omitted_permissions = 0;
  }

-- 
Eric Blake   eblake <at> redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Message #16 received at 9170 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eric Blake <eblake <at> redhat.com>
Cc: 9170 <at> debbugs.gnu.org
Subject: Re: bug#9170: [PATCH] cp "restores" permissions it never set
Date: Mon, 25 Jul 2011 15:13:58 -0700

On 07/25/11 14:21, Eric Blake wrote:
> This violates the style guide in HACKING; it should either be:

True, but it's consistent with the style that's already used
in that file, near line 954.  Perhaps the style should
be changed consistently, as a separate patch.

Message #19 received at 9170 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: 9170 <at> debbugs.gnu.org
Cc: eggert <at> cs.ucla.edu
Subject: Re: bug#9170: [PATCH] cp "restores" permissions it never set
Date: Tue, 26 Jul 2011 09:08:32 +0200

Paul Eggert wrote:
> Thanks for the bug report.  I committed this patch:
>
> Subject: [PATCH] cp: don't mishandle existing dir dest permissions (Bug#9170)
>
> * src/copy.c (copy_internal): If we don't create the directory,
> then we cannot have omitted permissions.  Problem and trivial
> fix reported by Eric Lammerts.
> * tests/Makefile.am (TESTS): Add cp/existing-perm-dir.
> * tests/cp/existing-perm-dir: New file.

Thanks, Paul.

That is a bug fix, so it deserves a NEWS entry.
Do you feel like tracking down the point at which
the bug was introduced to mention it in NEWS?

I'll apply the change below to address the style issues.

From 5f35396395a61a9dd60367066bd4495091bd6f88 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Tue, 26 Jul 2011 09:01:44 +0200
Subject: [PATCH] maint: use consistent style in C and test scripts

* src/copy.c (copy_internal): Adjust formatting style to conform with
guidelines in HACKING: put braces around two one-line "else" blocks.
* tests/cp/existing-perm-dir: Use $(...), not `...`, and
stat rather than ls+cut to get the mode string.
mode=$(stat --p=%A dst/dir)
---
 src/copy.c                 |    8 ++++++--
 tests/cp/existing-perm-dir |    2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index b2aeb6e..aaf7e79 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -951,7 +951,9 @@ copy_reg (char const *src_name, char const *dst_name,
         dest_errno = ENOTDIR;
     }
   else
-    omitted_permissions = 0;
+    {
+      omitted_permissions = 0;
+    }

   if (dest_desc < 0)
     {
@@ -2212,7 +2214,9 @@ copy_internal (char const *src_name, char const *dst_name,
             emit_verbose (src_name, dst_name, NULL);
         }
       else
-        omitted_permissions = 0;
+        {
+          omitted_permissions = 0;
+        }

       /* Decide whether to copy the contents of the directory.  */
       if (x->one_file_system && device != 0 && device != src_sb.st_dev)
diff --git a/tests/cp/existing-perm-dir b/tests/cp/existing-perm-dir
index 6ef73f0..a8c2816 100755
--- a/tests/cp/existing-perm-dir
+++ b/tests/cp/existing-perm-dir
@@ -25,7 +25,7 @@ mkdir -p -m ug-s,u=rwx,g=,o= dst/dir || fail=1

 cp -r src/. dst/ || fail=1

-mode=`ls -ld dst/dir | cut -b-10`
+mode=$(stat --p=%A dst/dir)
 test "$mode" = drwx------ || fail=1

 Exit $fail
--
1.7.6.609.gbf6a9

Message #22 received at 9170 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: 9170 <at> debbugs.gnu.org
Subject: Re: bug#9170: [PATCH] cp "restores" permissions it never set
Date: Tue, 26 Jul 2011 00:20:52 -0700

On 07/26/11 00:08, Jim Meyering wrote:

> Do you feel like tracking down the point at which
> the bug was introduced to mention it in NEWS?

To be honest, I prefer thinking about the future
to worrying about the past.  Could we just omit
that part of the announcement?  If someone cares
about the past, they can look it up.

Message #25 received at 9170 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 9170 <at> debbugs.gnu.org
Subject: Re: bug#9170: [PATCH] cp "restores" permissions it never set
Date: Tue, 26 Jul 2011 09:36:12 +0200

Paul Eggert wrote:
> On 07/26/11 00:08, Jim Meyering wrote:
>
>> Do you feel like tracking down the point at which
>> the bug was introduced to mention it in NEWS?
>
> To be honest, I prefer thinking about the future
> to worrying about the past.  Could we just omit
> that part of the announcement?  If someone cares
> about the past, they can look it up.

It's useful for people maintaining older systems, so I looked
it up.  I confirmed it was introduced between 6.7 and 6.8.
Of the six copy.c-affecting commits in that range, only
b28a8851ed22dbf0cd123974a0c97ae0b82bec2b touches permissions.

I'll push this shortly:

From b2bb19b4b32506debf65f03c8e44b66374550597 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Tue, 26 Jul 2011 09:24:31 +0200
Subject: [PATCH] doc: mention cp's dir-permissions fix

* NEWS (Bug fixes): Mention yesterday's dir-permissions fix.
---
 NEWS |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/NEWS b/NEWS
index a4e7e9e..291ce13 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   I.E. for skipped files, the original ownership is output, not the new one.
   [bug introduced in sh-utils-2.0g]

+  cp -r could mistakenly change the permissions of an existing destination
+  directory.  [bug introduced in coreutils-6.8]
+
   cp -u -p would fail to preserve one hard link for each up-to-date copy
   of a src-hard-linked name in the destination tree.  I.e., if s/a and s/b
   are hard-linked and dst/s/a is up to date, "cp -up s dst" would copy s/b
--
1.7.6.609.gbf6a9

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.