GNU bug report logs - #5956
[PATCH] cp: preserve "capabilities" when also preserving file ownership

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 5956 in the body.
You can then email your comments to 5956 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: Pádraig Brady <P <at> draigBrady.com>
To: Report bugs to <bug-coreutils <at> gnu.org>
Subject: [PATCH] cp: preserve "capabilities" when also preserving file
	ownership
Date: Fri, 16 Apr 2010 09:41:00 +0100

[Message part 1 (text/plain, inline)]

`sudo cp -a non-root-file copy` would not preserve capabilities.
The attached fixes this and passes all tests.

cheers,
Pádraig.

[cp-capabilities.diff (text/x-patch, attachment)]

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

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 5956 <at> debbugs.gnu.org
Subject: Re: bug#5956: [PATCH] cp: preserve "capabilities" when also
	preserving file ownership
Date: Fri, 16 Apr 2010 22:13:36 +0200

Pádraig Brady wrote:
> `sudo cp -a non-root-file copy` would not preserve capabilities.
> The attached fixes this and passes all tests.
...
> Subject: [PATCH] cp: preserve "capabilities" when also preserving file ownership
>
> * src/copy.c (copy_reg): Copy xattrs _after_ setting file ownership
> so that capabilities are not cleared when setting ownership.
> * tests/cp/capability: A new root test.
> * tests/Makefile.am (root_tests): Reference the new test.
> * NEWS: Mention the fix.

Good catch!
The patch looks fine.
Some tiny suggestions:

> diff --git a/NEWS b/NEWS
...
> +  cp now preserves "capabilities" when also preserving file ownership.

s/when also/also when/

>    ls --color once again honors the 'NORMAL' dircolors directive.
>    [bug introduced in coreutils-6.11]
>
> diff --git a/src/copy.c b/src/copy.c
> index 0fa148e..4e70c21 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -826,6 +826,22 @@ copy_reg (char const *src_name, char const *dst_name,
>          }
>      }
>
> +  /* We set ownership before xattrs as changing owners will
> +     clear capabilities.  */

Please use an active/imperative voice:

    /* Set ownership before setting xattrs, since setting ownership
       clears capabilities.  */

> +  if (x->preserve_ownership && ! SAME_OWNER_AND_GROUP (*src_sb, sb))
> +    {
> +      switch (set_owner (x, dst_name, dest_desc, src_sb, *new_dst, &sb))

...
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index db1610d..a943ff3 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -23,6 +23,7 @@ root_tests =					\
>    cp/preserve-gid				\
>    cp/special-bits				\
>    cp/cp-mv-enotsup-xattr			\
> +  cp/capability					\
>    dd/skip-seek-past-dev				\
>    install/install-C-root			\
>    ls/capability					\
> diff --git a/tests/cp/capability b/tests/cp/capability
...
> +(setcap --help) 2>&1 |grep 'usage: setcap' > /dev/null \
> +  || skip_test_ "setcap utility not found"
> +(getcap --help) 2>&1 |grep 'usage: getcap' > /dev/null \
> +  || skip_test_ "getcap utility not found"
> +
> +# Don't let a different umask perturb the results.
> +umask 22

It's slightly better to use this function in place of the above two lines:

working_umask_or_skip_

> +touch file || framework_failure
> +chown $NON_ROOT_USERNAME file || framework_failure
...

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 5956-done <at> debbugs.gnu.org
Subject: Re: bug#5956: [PATCH] cp: preserve "capabilities" when also preserving
	file ownership
Date: Fri, 16 Apr 2010 23:12:59 +0100

On 16/04/10 21:13, Jim Meyering wrote:
> Pádraig Brady wrote:
>> `sudo cp -a non-root-file copy` would not preserve capabilities.
>> The attached fixes this and passes all tests.
> ...
>> Subject: [PATCH] cp: preserve "capabilities" when also preserving file ownership
>>
>> * src/copy.c (copy_reg): Copy xattrs _after_ setting file ownership
>> so that capabilities are not cleared when setting ownership.
>> * tests/cp/capability: A new root test.
>> * tests/Makefile.am (root_tests): Reference the new test.
>> * NEWS: Mention the fix.
> 
> Good catch!
> The patch looks fine.
> Some tiny suggestions:

Thanks for the review.
I've just pushed that.

cheers,
Pádraig.

Previous Next

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