Re: [PATCH]: cp: make -a option preserve SELinux context and xattrs with reduced diagnostics

[Jim Meyering]

Ondřej Vašík <address@hidden> wrote:
> I sent patch about cp -a and it's failure to preserve SELinux context in
> October 08. Meanwhile xattr support was added to cp. As described in
> info pages -a option means "Preserve as much as possible of the
> structure and attributes of the original files". Therefore it should
> preserve xattr if possible as well. This would mean additional possible
> failure reports, which are not acceptable for cp -a option. Therefore I
> had to remove displaying them for it.
>
> After patch it should work like:
> 1) cp -a tries to preserve SELinux context and xattr, but doesn't inform
> about possible failures
> 2) cp --preserve=all tries to preserve SELinux context and xattr, but
> does inform about failures and doesn't fail if xattr/SELinux context are
> not preserved
> 3) cp --preserve=context,xattr obviously tries to preserve attributes,
> inform about failures and does fail if xattr/SELinux context is not
> preserved
>
> NOTE: Added documentation of --preserve=context, description of current
> behaviour to cp --preserve=all and cp -a documentation. Added tests to
> ensure that cp -a actually works with SELinux (until now it was checking
> just failures.

Hi Ondřej,

Unfortunately, with this patch, "make check" fails the cp/link-heap test.
Investigating why, I discovered that it introduces a huge leak into
cp -a, at least on F10.  80MB worth in this case.

And when that test tries to run cp in a restricted-memory
environment (ulimit to 14MB), it fails.  valgrind reports this:

==5599== 81,931,806 bytes in 19,998 blocks are definitely lost in loss record 2 
of 2
==5599==    at 0x4A0739E: malloc (vg_replace_malloc.c:207)
==5599==    by 0x36FD202F69: attr_copy_action (in /lib64/libattr.so.1.1.0)
==5599==    by 0x36FD202E58: attr_copy_check_permissions (in 
/lib64/libattr.so.1.1.0)
==5599==    by 0x36FD2029FB: attr_copy_file (in /lib64/libattr.so.1.1.0)
==5599==    by 0x405107: copy_internal (copy.c:188)
==5599==    by 0x406836: copy_internal (copy.c:246)
==5599==    by 0x406836: copy_internal (copy.c:246)
==5599==    by 0x407D33: copy (copy.c:2196)
==5599==    by 0x403DC9: do_copy (cp.c:743)
==5599==    by 0x404498: main (cp.c:1128)

==3797==    definitely lost: 81,931,806 bytes in 19,998 blocks.
==3797==      possibly lost: 20,485 bytes in 5 blocks.
==3797==    still reachable: 0 bytes in 0 blocks.
==3797==         suppressed: 0 bytes in 0 blocks.

I'll let you investigate that one ;-)

Also, please fold the following into your patch for next time.
Without the first hunk, with -Werror (via ./configure --enable-gcc-warnings)
your new code doesn't even compile.

>From abb2cf734278b46fa5922496ad0942c62cc6c421 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 11 Feb 2009 18:15:27 +0100
Subject: [PATCH] * src/copy.c (ignore_attr_error): Mark fmt parameter as unused.

---
 src/copy.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 2da728a..5cb87ae 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -146,7 +146,7 @@ copy_attr_error (struct error_context *ctx ATTRIBUTE_UNUSED,

 static void
 ignore_attr_error (struct error_context *ctx ATTRIBUTE_UNUSED,
-                char const *fmt, ...)
+                  char const *fmt ATTRIBUTE_UNUSED, ...)
 {
 }

--
1.6.2.rc0.35.g1b53


>From 95197a9c2879080a093873fbfc181dccdbce7f50 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 11 Feb 2009 18:15:27 +0100
Subject: [PATCH] * src/copy.c (ignore_attr_error): Mark fmt parameter as 
unused; tweak spacing

---
 src/copy.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 2da728a..a2fc369 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -146,7 +146,7 @@ copy_attr_error (struct error_context *ctx ATTRIBUTE_UNUSED,

 static void
 ignore_attr_error (struct error_context *ctx ATTRIBUTE_UNUSED,
-                char const *fmt, ...)
+                  char const *fmt ATTRIBUTE_UNUSED, ...)
 {
 }

@@ -177,7 +177,7 @@ copy_attr_by_fd (char const *src_path, int src_fd,

 static bool
 copy_attr_by_name (char const *src_path, char const *dst_path,
-               const struct cp_options *x)
+                  const struct cp_options *x)
 {
   struct error_context ctx =
   {
@@ -482,7 +482,7 @@ copy_reg (char const *src_name, char const *dst_name,
                      goto close_src_and_dst_desc;
                    }
                }
-             freecon(con);
+             freecon (con);
            }
        }

@@ -505,7 +505,7 @@ copy_reg (char const *src_name, char const *dst_name,
   if (*new_dst)
     {
       int open_flags = O_WRONLY | O_CREAT | O_BINARY;
-      dest_desc = open (dst_name, open_flags | O_EXCL ,
+      dest_desc = open (dst_name, open_flags | O_EXCL,
                        dst_mode & ~omitted_permissions);
       dest_errno = errno;

--
1.6.2.rc0.35.g1b53