[PATCH] Use xattr (Linux) in copy-acl.c

[PATCH] Use xattr (Linux) in copy-acl.c

[Ondrej Valousek]

The following patch is aiming to improve & simplify ACL handling in copy-acl.c
Changes/Benefits:
* we no longer try to decode ACLs, instead we just copy the whole xattr with ACLs
making the code simpler and faster
* side effect is support for NFSv4 acls
Disadvantages:
* it pulls in dependency on libattr. We still need dependency on libacl
because of set-acl.c which would have to be cared of separately. The
idea I have here is to confirm if it's safe to replace set_acl() with
chmod_or_fchmod() at least in Linux. It probably has something to do
with ACL inheritance.
Anyhow - with this patch copy_acl() does NOT honor inherited ACLs. It copies ACLs to dest
file exactly - i.e. the same way the old code worked.
* we can't do any ACLs conversions, but these are not (AFAIK) being done anyway in the
old code (the old code only worked with Posix)

The goal is for "cp -p" to preserve ACLs regardless of the type.
I tried to experiment with variety of filesystems (posix and nfs) and it seems to me
to work exactly the way the old code worked (plus handy support for NFSv4).

Ondrej


---
 lib/copy-acl.c | 31 ++++++++++++++++++++++++++++++-
 m4/xattr.m4    | 43 +++++++++++++++++++++++++++++++++++++++++++
 modules/acl    |  1 +
 3 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100644 m4/xattr.m4

diff --git a/lib/copy-acl.c b/lib/copy-acl.c
index 5fc42b7f6..341d1f605 100644
--- a/lib/copy-acl.c
+++ b/lib/copy-acl.c
@@ -29,6 +29,20 @@
 #define _(msgid) gettext (msgid)
 
 
+#if USE_XATTR
+
+# include <attr/libattr.h>
+
+static int
+copy_attr_permissions (const char *name, struct error_context *ctx)
+{
+	int action = attr_copy_action (name, ctx);
+	return action == ATTR_ACTION_PERMISSIONS;
+}
+
+#endif  /* USE_XATTR */
+
+
 /* Copy access control lists from one file to another. If SOURCE_DESC is
    a valid file descriptor, use file descriptor operations, else use
    filename based operations on SRC_NAME. Likewise for DEST_DESC and
@@ -43,7 +57,22 @@ int
 copy_acl (const char *src_name, int source_desc, const char *dst_name,
           int dest_desc, mode_t mode)
 {
-  int ret = qcopy_acl (src_name, source_desc, dst_name, dest_desc, mode);
+  int ret;
+#ifdef USE_XATTR
+  ret = chmod_or_fchmod (dst_name, dest_desc, mode);
+  /* Rather than fiddling with acls one by one, we just copy the whole ACL xattrs 
+   * (Posix or NFSv4). Of course, that won't address ACLs conversion 
+   * (i.e. posix <-> nfs4) but we can't do it anyway, so for now, we don't care
+   */
+  if(ret == 0)
+    ret = source_desc <= 0 && dest_desc <= 0
+      ? attr_copy_file (src_name, dst_name, copy_attr_permissions, NULL)
+      : attr_copy_fd (src_name, source_desc, dst_name, dest_desc, copy_attr_permissions, NULL);
+#else
+  /* no XATTR, so we proceed the old dusty way */
+  ret = qcopy_acl (src_name, source_desc, dst_name, dest_desc, mode);
+#endif
+
   switch (ret)
     {
     case -2:
diff --git a/m4/xattr.m4 b/m4/xattr.m4
new file mode 100644
index 000000000..5f9248939
--- /dev/null
+++ b/m4/xattr.m4
@@ -0,0 +1,43 @@
+# xattr.m4 - check for Extended Attributes (Linux)
+# serial 4
+
+# Copyright (C) 2003-2021 Free Software Foundation, Inc.
+# This file is free software; the Free Software Foundation
+# gives unlimited permission to copy and/or distribute it,
+# with or without modifications, as long as this notice is preserved.
+
+# Originally written by Andreas Gruenbacher.
+
+AC_DEFUN([gl_FUNC_XATTR],
+[
+  AC_ARG_ENABLE([xattr],
+        AS_HELP_STRING([--disable-xattr],
+                       [do not support extended attributes]),
+        [use_xattr=$enableval], [use_xattr=yes])
+
+  LIB_XATTR=
+  AC_SUBST([LIB_XATTR])
+
+  if test "$use_xattr" = "yes"; then
+    AC_CHECK_HEADERS([attr/error_context.h attr/libattr.h])
+    use_xattr=no
+    if test $ac_cv_header_attr_libattr_h = yes \
+        && test $ac_cv_header_attr_error_context_h = yes; then
+      xattr_saved_LIBS=$LIBS
+      AC_SEARCH_LIBS([attr_copy_file], [attr],
+                     [test "$ac_cv_search_attr_copy_file" = "none required" ||
+                        LIB_XATTR=$ac_cv_search_attr_copy_file])
+      AC_CHECK_FUNCS([attr_copy_file])
+      LIBS=$xattr_saved_LIBS
+      if test $ac_cv_func_attr_copy_file = yes; then
+        use_xattr=yes
+      fi
+    fi
+    if test $use_xattr = no; then
+      AC_MSG_WARN([libattr development library was not found or not usable.])
+      AC_MSG_WARN([AC_PACKAGE_NAME will be built without xattr support.])
+    fi
+  fi
+  AC_DEFINE_UNQUOTED([USE_XATTR], [`test $use_xattr != yes; echo $?`],
+                     [Define if you want extended attribute support.])
+])
diff --git a/modules/acl b/modules/acl
index 1a3a14e6c..ca2239823 100644
--- a/modules/acl
+++ b/modules/acl
@@ -4,6 +4,7 @@ Access control lists of files, with diagnostics.  (Unportable.)
 Files:
 lib/copy-acl.c
 lib/set-acl.c
+m4/xattr.m4
 
 Depends-on:
 error
-- 
2.38.1

Re: [PATCH] Use xattr (Linux) in copy-acl.c

[Bruno Haible]

Hi Ondrej,

> diff --git a/lib/copy-acl.c b/lib/copy-acl.c
> index 5fc42b7f6..341d1f605 100644
> --- a/lib/copy-acl.c
> +++ b/lib/copy-acl.c
> @@ -29,6 +29,20 @@
>  #define _(msgid) gettext (msgid)
>  
>  
> +#if USE_XATTR
> +
> +# include <attr/libattr.h>
> +
> +static int
> +copy_attr_permissions (const char *name, struct error_context *ctx)

This function — like all functions — deserves a comment before the
function. I would propose:

/* Returns 1 if NAME is the name of an extended attribute that is related
   to permissions, i.e. ACLs.  Returns 0 otherwise.  */

In view of this description, 'copy_attr_permissions' is a strange name
for a function that returns a boolean value. How about renaming it to
'is_permissions_attr' ?

> +{
> +	int action = attr_copy_action (name, ctx);
> +	return action == ATTR_ACTION_PERMISSIONS;

Gnulib style: Please indent with spaces, not with tabs.

>  /* Copy access control lists from one file to another. If SOURCE_DESC is
>     a valid file descriptor, use file descriptor operations, else use
>     filename based operations on SRC_NAME. Likewise for DEST_DESC and
> @@ -43,7 +57,22 @@ int
>  copy_acl (const char *src_name, int source_desc, const char *dst_name,
>            int dest_desc, mode_t mode)
>  {
> -  int ret = qcopy_acl (src_name, source_desc, dst_name, dest_desc, mode);
> +  int ret;
> +#ifdef USE_XATTR
> +  ret = chmod_or_fchmod (dst_name, dest_desc, mode);
> +  /* Rather than fiddling with acls one by one, we just copy the whole ACL xattrs 
> +   * (Posix or NFSv4). Of course, that won't address ACLs conversion 
> +   * (i.e. posix <-> nfs4) but we can't do it anyway, so for now, we don't care
> +   */
> +  if(ret == 0)

GNU coding style: Please add a space after 'if'.

> +    ret = source_desc <= 0 && dest_desc <= 0
> +      ? attr_copy_file (src_name, dst_name, copy_attr_permissions, NULL)
> +      : attr_copy_fd (src_name, source_desc, dst_name, dest_desc, copy_attr_permissions, NULL);
> +#else
> +  /* no XATTR, so we proceed the old dusty way */
> +  ret = qcopy_acl (src_name, source_desc, dst_name, dest_desc, mode);
> +#endif

Instead of putting this code into copy_acl, I think qcopy_acl would be the
better place. The difference between qcopy_acl and copy_acl is, per definition,
that the former is silent (does not write to stdout or stderr), whereas the
latter signals errors.

> diff --git a/m4/xattr.m4 b/m4/xattr.m4
> new file mode 100644
> index 000000000..5f9248939
> --- /dev/null
> +++ b/m4/xattr.m4

> +  AC_DEFINE_UNQUOTED([USE_XATTR], [`test $use_xattr != yes; echo $?`],

The use of backquotes here is ugly. How about simplifying it by use of a
shell variable? E.g.

     if test $use_xattr = yes; then
       use_xattr_value=1
     else
       use_xattr_value=0
     fi
     AC_DEFINE_UNQUOTED([USE_XATTR], [$use_xattr_value])

> diff --git a/modules/acl b/modules/acl
> index 1a3a14e6c..ca2239823 100644
> --- a/modules/acl
> +++ b/modules/acl
> @@ -4,6 +4,7 @@ Access control lists of files, with diagnostics.  (Unportable.)
>  Files:
>  lib/copy-acl.c
>  lib/set-acl.c
> +m4/xattr.m4
>  
>  Depends-on:
>  error

This change would go into modules/qcopy-acl, not modules/acl.

And gl_FUNC_XATTR needs to be invoked from somewhere, otherwise all of
m4/xattr.m4 is dead code.

Bruno

Re: [PATCH] Use xattr (Linux) in copy-acl.c

[Paul Eggert]

Thanks again for looking into this.

On 2023-01-03 06:08, Ondrej Valousek wrote:

> +static int
> +copy_attr_permissions (const char *name, struct error_context *ctx)
> +{
> +	int action = attr_copy_action (name, ctx);
> +	return action == ATTR_ACTION_PERMISSIONS;
> +}


This returns bool not int, so it should be declared to return bool. The 
body can be a one-liner, no?


> +#ifdef USE_XATTR
> +  ret = chmod_or_fchmod (dst_name, dest_desc, mode);
> +  /* Rather than fiddling with acls one by one, we just copy the whole ACL xattrs
> +   * (Posix or NFSv4). Of course, that won't address ACLs conversion
> +   * (i.e. posix <-> nfs4) but we can't do it anyway, so for now, we don't care
> +   */
> +  if(ret == 0)
> +    ret = source_desc <= 0 && dest_desc <= 0
> +      ? attr_copy_file (src_name, dst_name, copy_attr_permissions, NULL)
> +      : attr_copy_fd (src_name, source_desc, dst_name, dest_desc, copy_attr_permissions, NULL);

Please add brief commentary as to why we need both chmod_or_fchmod and 
attr_copy_* (i.e., why it doesn't suffice to just do the attr_copy_*), 
and why it's important to do chmod_or_fchmod before doing attr_copy_*?

Also, please use Gnulib style in comments; no "*" on each line.

Also, the test "source_desc <= 0 && dest_desc <= 0" looks wrong. 
Shouldn't it be "source_desc < 0 || dest_desc < 0"?

> +    if test $ac_cv_header_attr_libattr_h = yes \
> +        && test $ac_cv_header_attr_error_context_h = yes; then

Although quoting inside "" isn't needed for shell variables that you 
know have only safe characters, these ac_cv_* shell var uses should be 
quoted inside "" for safety, e.g., 'test "$ac_cv_header_attr_libattr_h" 
= yes', in case the user screws up and puts spaces into their values. 
Please do this systematically for ac_cv_* variables.

Re: [PATCH] Use xattr (Linux) in copy-acl.c

[Bruno Haible]

Paul Eggert wrote:
> > +static int
> > +copy_attr_permissions (const char *name, struct error_context *ctx)
> > +{
> > +	int action = attr_copy_action (name, ctx);
> > +	return action == ATTR_ACTION_PERMISSIONS;
> > +}
> 
> 
> This returns bool not int, so it should be declared to return bool.

Nope. This function is used as a callback for attr_copy_file and
attr_copy_fd [1] and therefore needs 'int' as return type.

Bruno

[1] https://git.savannah.nongnu.org/gitweb/?p=attr.git;a=blob;f=include/libattr.h;hb=HEAD#l27

[PATCH] Use xattr (Linux) in copy-acl.c

[Ondrej Valousek]

Hi Paul/Bruno,
Thanks for valuable input. I have included your suggestions in the
following patch.
Hope it looks fine now.
Ondrej

---
 lib/qcopy-acl.c   | 33 +++++++++++++++++++++++++++++++++
 m4/xattr.m4       | 45 +++++++++++++++++++++++++++++++++++++++++++++
 modules/qcopy-acl |  2 ++
 3 files changed, 80 insertions(+)
 create mode 100644 m4/xattr.m4

diff --git a/lib/qcopy-acl.c b/lib/qcopy-acl.c
index 883bcf7d58..5968189733 100644
--- a/lib/qcopy-acl.c
+++ b/lib/qcopy-acl.c
@@ -23,6 +23,20 @@
 
 #include "acl-internal.h"
 
+#if USE_XATTR
+
+# include <attr/libattr.h>
+
+/* Returns 1 if NAME is the name of an extended attribute that is related
+   to permissions, i.e. ACLs.  Returns 0 otherwise.  */
+
+static int
+is_attr_permissions (const char *name, struct error_context *ctx)
+{
+  return attr_copy_action (name, ctx) == ATTR_ACTION_PERMISSIONS;
+}
+
+#endif  /* USE_XATTR */
 
 /* Copy access control lists from one file to another. If SOURCE_DESC is
    a valid file descriptor, use file descriptor operations, else use
@@ -42,10 +56,29 @@ qcopy_acl (const char *src_name, int source_desc, const char *dst_name,
   struct permission_context ctx;
   int ret;
 
+#ifdef USE_XATTR
+  /* in case no ACLs present and also to set higher mode bits
+     we chmod before setting ACLs as doing it after could overwrite them 
+     (especially true for NFSv4, posix ACL has that ugly "mask" hack that
+     nobody understands) */
+  ret = chmod_or_fchmod (dst_name, dest_desc, mode);
+  /* Rather than fiddling with acls one by one, we just copy the whole ACL xattrs
+     (Posix or NFSv4). Of course, that won't address ACLs conversion
+     (i.e. posix <-> nfs4) but we can't do it anyway, so for now, we don't care
+     Functions attr_copy_* return 0 in case we copied something OR nothing
+     to copy */
+  if (ret == 0)
+    ret = source_desc <= 0 || dest_desc <= 0
+      ? attr_copy_file (src_name, dst_name, is_attr_permissions, NULL)
+      : attr_copy_fd (src_name, source_desc, dst_name, dest_desc, 
+                      is_attr_permissions, NULL);
+#else
+  /* no XATTR, so we proceed the old dusty way */
   ret = get_permissions (src_name, source_desc, mode, &ctx);
   if (ret != 0)
     return -2;
   ret = set_permissions (&ctx, dst_name, dest_desc);
   free_permission_context (&ctx);
+#endif
   return ret;
 }
diff --git a/m4/xattr.m4 b/m4/xattr.m4
new file mode 100644
index 0000000000..4e8cbb0c4d
--- /dev/null
+++ b/m4/xattr.m4
@@ -0,0 +1,45 @@
+# xattr.m4 - check for Extended Attributes (Linux)
+# serial 4
+
+# Copyright (C) 2003-2021 Free Software Foundation, Inc.
+# This file is free software; the Free Software Foundation
+# gives unlimited permission to copy and/or distribute it,
+# with or without modifications, as long as this notice is preserved.
+
+AC_DEFUN([gl_FUNC_XATTR],
+[
+  AC_ARG_ENABLE([xattr],
+        AS_HELP_STRING([--disable-xattr],
+                       [do not support extended attributes]),
+        [use_xattr=$enableval], [use_xattr=yes])
+
+  LIB_XATTR=
+  AC_SUBST([LIB_XATTR])
+
+  if test "$use_xattr" = "yes"; then
+    AC_CHECK_HEADERS([attr/error_context.h attr/libattr.h])
+    use_xattr=no
+    if test "$ac_cv_header_attr_libattr_h" = yes \
+        && test "$ac_cv_header_attr_error_context_h" = yes; then
+      xattr_saved_LIBS=$LIBS
+      AC_SEARCH_LIBS([attr_copy_file], [attr],
+                     [test "$ac_cv_search_attr_copy_file" = "none required" ||
+                        LIB_XATTR="$ac_cv_search_attr_copy_file"])
+      AC_CHECK_FUNCS([attr_copy_file])
+      LIBS=$xattr_saved_LIBS
+      if test "$ac_cv_func_attr_copy_file" = yes; then
+        use_xattr=yes
+      fi
+    fi
+    if test $use_xattr = no; then
+      AC_MSG_WARN([libattr development library was not found or not usable.])
+      AC_MSG_WARN([AC_PACKAGE_NAME will be built without xattr support.])
+    fi
+  fi
+  if test $use_xattr = yes; then
+    use_xattr_value=1
+  else
+    use_xattr_value=0
+  fi
+  AC_DEFINE_UNQUOTED([USE_XATTR], [$use_xattr_value])
+])
diff --git a/modules/qcopy-acl b/modules/qcopy-acl
index c0e5b6a8f8..e0cd914953 100644
--- a/modules/qcopy-acl
+++ b/modules/qcopy-acl
@@ -3,11 +3,13 @@ Copy access control list from one file to another.  (Unportable.)
 
 Files:
 lib/qcopy-acl.c
+m4/xattr.m4
 
 Depends-on:
 acl-permissions
 
 configure.ac:
+gl_FUNC_XATTR
 
 Makefile.am:
 lib_SOURCES += qcopy-acl.c
-- 
2.38.1

Re: [PATCH] Use xattr (Linux) in copy-acl.c

[Bruno Haible]

Ondrej Valousek wrote:
> Hope it looks fine now.

This looks better indeed.

> +#ifdef USE_XATTR

Since USE_XATTR is always defined (to 0 or 1), you need to test it
with #if, not #ifdef.

> @@ -42,10 +56,29 @@ qcopy_acl (const char *src_name, int source_desc, const char *dst_name,
>    struct permission_context ctx;

This variable is only used in the #else case. To avoid compiler warnings
(I hope you always test with -Wall, right?) this variable declaration
should be moved into the #else block.

> +  AC_SUBST([LIB_XATTR])

I don't see any use of $(LIB_XATTR) so far. AFAICS, it ought to be
used
  - in the 'Link' section of modules/qcopy-acl,
  - in the 'Link' section of modules/acl, modules/copy-file, modules/qacl,
    modules/supersede — since these modules depend on qcopy-acl.
  - for the programs created in modules/acl-tests, modules/copy-file-tests,
    modules/supersede-tests.
Preferably each time right after $(LIB_ACL).
(One needs to lists the libraries explicitly, because you have no guarantee
that the distro uses shared libraries with implicit dependencies. It could
also be shared libraries without explicit dependencies, or static libraries.)

Bruno