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

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

[Ondrej Valousek]

Hi Bruno,
Please review now.
Ondrej

---
 lib/qcopy-acl.c         | 36 ++++++++++++++++++++++++++++++++++-
 m4/xattr.m4             | 42 +++++++++++++++++++++++++++++++++++++++++
 modules/acl             |  1 +
 modules/acl-tests       |  6 +++---
 modules/copy-file       |  1 +
 modules/copy-file-tests |  2 +-
 modules/qacl            |  3 +++
 modules/qcopy-acl       |  3 +++
 modules/supersede       |  1 +
 modules/supersede-tests |  2 +-
 10 files changed, 91 insertions(+), 6 deletions(-)
 create mode 100644 m4/xattr.m4

diff --git a/lib/qcopy-acl.c b/lib/qcopy-acl.c
index 883bcf7d58..44980b71a4 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
@@ -39,13 +53,33 @@ int
 qcopy_acl (const char *src_name, int source_desc, const char *dst_name,
            int dest_desc, mode_t mode)
 {
-  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 */
+  struct permission_context ctx;
+
   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..6efdfa475a
--- /dev/null
+++ b/m4/xattr.m4
@@ -0,0 +1,42 @@
+# 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
+    AC_DEFINE_UNQUOTED([USE_XATTR], 1)
+  fi
+])
diff --git a/modules/acl b/modules/acl
index 1a3a14e6c2..7edfb1e77d 100644
--- a/modules/acl
+++ b/modules/acl
@@ -22,6 +22,7 @@ Include:
 
 Link:
 $(LIB_MBRTOWC)
+$(LIB_XATTR)
 
 License:
 GPL
diff --git a/modules/acl-tests b/modules/acl-tests
index e4173fe9ae..1d8d84d299 100644
--- a/modules/acl-tests
+++ b/modules/acl-tests
@@ -25,6 +25,6 @@ TESTS += \
   test-copy-acl.sh test-copy-acl-1.sh test-copy-acl-2.sh
 TESTS_ENVIRONMENT += USE_ACL=$(USE_ACL)
 check_PROGRAMS += test-set-mode-acl test-copy-acl test-sameacls
-test_set_mode_acl_LDADD = $(LDADD) $(LIB_ACL) @LIBINTL@ $(LIB_MBRTOWC)
-test_copy_acl_LDADD = $(LDADD) $(LIB_ACL) @LIBINTL@ $(LIB_MBRTOWC)
-test_sameacls_LDADD = $(LDADD) $(LIB_ACL) @LIBINTL@ $(LIB_MBRTOWC)
+test_set_mode_acl_LDADD = $(LDADD) $(LIB_ACL) $(LIB_XATTR) @LIBINTL@ $(LIB_MBRTOWC)
+test_copy_acl_LDADD = $(LDADD) $(LIB_ACL) $(LIB_XATTR) @LIBINTL@ $(LIB_MBRTOWC)
+test_sameacls_LDADD = $(LDADD) $(LIB_ACL) $(LIB_XATTR) @LIBINTL@ $(LIB_MBRTOWC)
diff --git a/modules/copy-file b/modules/copy-file
index 6140c72fa1..0a86a15889 100644
--- a/modules/copy-file
+++ b/modules/copy-file
@@ -37,6 +37,7 @@ Include:
 
 Link:
 $(LIB_MBRTOWC)
+$(LIB_XATTR)
 
 License:
 GPL
diff --git a/modules/copy-file-tests b/modules/copy-file-tests
index a0788755f1..dc598b24f3 100644
--- a/modules/copy-file-tests
+++ b/modules/copy-file-tests
@@ -18,4 +18,4 @@ Makefile.am:
 TESTS += test-copy-file.sh test-copy-file-1.sh test-copy-file-2.sh
 TESTS_ENVIRONMENT += USE_ACL=$(USE_ACL)
 check_PROGRAMS += test-copy-file
-test_copy_file_LDADD = $(LDADD) $(LIB_ACL) $(LIB_CLOCK_GETTIME) @LIBINTL@ $(LIB_MBRTOWC)
+test_copy_file_LDADD = $(LDADD) $(LIB_ACL) $(LIB_XATTR) $(LIB_CLOCK_GETTIME) @LIBINTL@ $(LIB_MBRTOWC)
diff --git a/modules/qacl b/modules/qacl
index 45753c7623..7cfd9f3fbc 100644
--- a/modules/qacl
+++ b/modules/qacl
@@ -15,6 +15,9 @@ Makefile.am:
 
 Include:
 
+Link:
+$(LIB_XATTR)
+
 License:
 GPL
 
diff --git a/modules/qcopy-acl b/modules/qcopy-acl
index c0e5b6a8f8..dcd610464b 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
@@ -17,6 +19,7 @@ Include:
 
 Link:
 $(LIB_ACL)
+$(LIB_XATTR)
 
 License:
 GPL
diff --git a/modules/supersede b/modules/supersede
index 70f3fdc482..5c6ccafe5c 100644
--- a/modules/supersede
+++ b/modules/supersede
@@ -38,6 +38,7 @@ Link:
 $(LIB_GETRANDOM)
 $(LIB_CLOCK_GETTIME)
 $(LIBTHREAD)
+$(LIB_XATTR)
 
 License:
 GPL
diff --git a/modules/supersede-tests b/modules/supersede-tests
index 7224d2e31e..dc01a63652 100644
--- a/modules/supersede-tests
+++ b/modules/supersede-tests
@@ -19,4 +19,4 @@ configure.ac:
 Makefile.am:
 TESTS += test-supersede
 check_PROGRAMS += test-supersede
-test_supersede_LDADD = $(LDADD) $(LIB_ACL) $(LIB_CLOCK_GETTIME) $(LIB_GETRANDOM) $(LIBINTL) $(LIBTHREAD)
+test_supersede_LDADD = $(LDADD) $(LIB_ACL) $(LIB_XATTR) $(LIB_CLOCK_GETTIME) $(LIB_GETRANDOM) $(LIBINTL) $(LIBTHREAD)
-- 
2.38.1

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

[Bruno Haible]

Hi Ondrej,

> Please review now.

To me, it looks good. Paul's opinion?

Bruno

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

[Paul Eggert]

On 2023-01-04 06:46, Bruno Haible wrote:
> To me, it looks good. Paul's opinion?
I see a problem in linking. With this change, programs using the 
qcopy-acl module will link to both $(LIB_ACL) and $(LIB_XATTR), even 
though they do not need to link with $(LIB_ACL). Perhaps you can fix 
this by fixing the Link sections of the relevant modules to use 
$(LIB_HAS_ACL) instead of $(LIB_ACL). That is, for each module where you 
added $(LIB_XATTR), replace its $(LIB_ACL) with $(LIB_HAS_ACL) if the 
only reason it needed $(LIB_ACL) was to copy attributes.

Also, come to think of it, is there a security issue between the 
chmod_or_fchmod call, and the attr_copy_fd call? That is, could the file 
temporarily be set to too-generous permissions, between the two calls? A 
comment explaining this issue would help.

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

[Ondrej Valousek]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]


> Perhaps you can fix
this by fixing the Link sections of the relevant modules to use
$(LIB_HAS_ACL) instead of $(LIB_ACL). That is, for each module where you
added $(LIB_XATTR), replace its $(LIB_ACL) with $(LIB_HAS_ACL) if the
only reason it needed $(LIB_ACL) was to copy attributes.

Nope. LIB_HAS_ACL has value only if glibc does not know getxattr() system call. It has nothing to do with libattr (yes, it's bit confusing).
Why don't we solve the linker problem with the "--as-needed" option? This will make linker clever enough not to link libraries that are not needed.

> Also, come to think of it, is there a security issue between the
chmod_or_fchmod call, and the attr_copy_fd call? That is, could the file
temporarily be set to too-generous permissions, between the two calls? A
comment explaining this issue would help.

It depends on a kernel ACL implementation. On Linux the ACLs make the permissions only more opened (hence my code would be fine).
In contrast, on Solaris/ZFS, the opposite could happen.
NetApp NFSv4 server even allows you to break RFC7530 the way that chmod 0 <file> will still allow the inherited ACLs to be applied.
In general, I'd say we should be secure here.


[-- Attachment #2: Type: text/html, Size: 2048 bytes --]

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

[Paul Eggert]

On 2023-01-04 12:54, Ondrej Valousek wrote:

> LIB_HAS_ACL has value only if glibc does not know getxattr() system call. It has nothing to do with libattr (yes, it's bit confusing).

No kidding. This stuff is waaaayy too complicated.

> Why don't we solve the linker problem with the "--as-needed" option? 

--as-needed isn't portable, but Gnulib has a lib-ignore module that 
should be more portable. I suppose we could make the qcopy-acl module 
depend on the lib-ignore module, but we've never done anything like that 
before. Another possibility would be to implement a new variable 
LIB_XATTR_FALLBACK (or perhaps a better name), that acts like LIB_ACL if 
the xattr library is absent and the acl library is present, and is empty 
otherwise.

Perhaps Bruno has an opinion here. I'm not sure how well lib-ignore 
would work for packages that build libraries rather than apps.

> It depends on a kernel ACL implementation. On Linux the ACLs make the permissions only more opened (hence my code would be fine).
> In contrast, on Solaris/ZFS, the opposite could happen.

Ouch, in that case perhaps we should not use the new code on Solaris. 
Solaris is a dead end now anyway, no point trying to make it go faster.

What about OpenZFS on GNU/Linux? That's more important.

> NetApp NFSv4 server even allows you to break RFC7530 the way that chmod 0 <file> will still allow the inherited ACLs to be applied.

We should be OK there, right? We do the chmod 0 first.

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

[Bruno Haible]

Paul Eggert wrote:
> I see a problem in linking. With this change, programs using the 
> qcopy-acl module will link to both $(LIB_ACL) and $(LIB_XATTR), even 
> though they do not need to link with $(LIB_ACL). Perhaps you can fix 
> this by fixing the Link sections of the relevant modules to use 
> $(LIB_HAS_ACL) instead of $(LIB_ACL).

Good point.

But I would call this variable $(COPY_ACL_LIB). Our convention for these
variables is generally
  - $(LIBFOO) or $(LIB_FOO) for the library -lfoo (or empty if nonexistent
    or not needed),
  - $(MODULE_LIB) for the libraries needed by a given module. For example,
    we have
      $(GETADDRINFO_LIB)
      $(GETHOSTNAME_LIB)
      $(GETLOADAVG_LIBS)
      $(INET_NTOP_LIB)
      $(PTY_LIB)
      $(YIELD_LIB)

> Also, come to think of it, is there a security issue between the 
> chmod_or_fchmod call, and the attr_copy_fd call? That is, could the file 
> temporarily be set to too-generous permissions, between the two calls?

For the use of qcopy_acl by copy-file.c, there is no issue, because when
qcopy_acl gets called, the new file has mode 0600 and the owner and group
are already set. So, this is not too generous.

But for the uses of copy_acl in coreutils, I can't really tell...

Bruno

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

[Bruno Haible]

Ondrej Valousek wrote:
> Why don't we solve the linker problem with the "--as-needed" option? This will make linker clever enough not to link libraries that are not needed.

'--as-needed' is a can of worms in itself. In GNU gettext (IIRC) Debian
used '--as-needed' in the link command line, and together with libtool
that reorders the elements of the command line, it had ill effects.

More generally, Gnulib should help GNU packages and integrate into the
package's build system the best it can. It should *not* force specifies
ways of doing onto the package's build system.

Bruno

RE: [PATCH] Use xattr (Linux) in qcopy-acl.c

[Ondrej Valousek]

Interesting.
Can we accept the patch as it is for now and work in the linker dependency with a different commit perhaps?
As I have to admit I am not quite sure what to do now.
Ondrej

-----Original Message-----
From: Bruno Haible <bruno@clisp.org> 
Sent: čtvrtek 5. ledna 2023 10:01
To: Paul Eggert <eggert@cs.ucla.edu>; bug-gnulib@gnu.org; Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Subject: Re: [PATCH] Use xattr (Linux) in qcopy-acl.c

Ondrej Valousek wrote:
> Why don't we solve the linker problem with the "--as-needed" option? This will make linker clever enough not to link libraries that are not needed.

'--as-needed' is a can of worms in itself. In GNU gettext (IIRC) Debian used '--as-needed' in the link command line, and together with libtool that reorders the elements of the command line, it had ill effects.

More generally, Gnulib should help GNU packages and integrate into the package's build system the best it can. It should *not* force specifies ways of doing onto the package's build system.

Bruno

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

[Bruno Haible]

Ondrej Valousek wrote:
> Can we accept the patch as it is for now and work in the linker dependency with a different commit perhaps?

Yes, this is OK. I can work on the linker dependencies shortly after your
patch is in. Basically it consists in compiling a testdir
  ./gnulib-tool --create-testdir --dir=../testdir1 --single-configure qcopy-acl acl copy-file qacl supersede
with various configurations (acl, xattr, none) and see which link options
are needed in which test programs.

Bruno

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

[Paul Eggert]

On 2023-01-05 02:32, Bruno Haible wrote:
> Yes, this is OK. I can work on the linker dependencies shortly after your
> patch is in. Basically it consists in compiling a testdir

Sure, as long as the linker dependencies are fixed soon we'll be OK.

RE: [PATCH] Use xattr (Linux) in qcopy-acl.c

[Ondrej Valousek]

Hi Bruno/Paul,

Are we going to commit the proposed changes there or is there still anything missing?
I am asking because I'd like to file a RFE for redhat coreutils improvement and this change depends on it.

Thanks,
Ondrej

-----Original Message-----
From: Paul Eggert <eggert@cs.ucla.edu> 
Sent: čtvrtek 5. ledna 2023 20:07
To: Bruno Haible <bruno@clisp.org>; bug-gnulib@gnu.org; Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Subject: Re: [PATCH] Use xattr (Linux) in qcopy-acl.c

On 2023-01-05 02:32, Bruno Haible wrote:
> Yes, this is OK. I can work on the linker dependencies shortly after 
> your patch is in. Basically it consists in compiling a testdir

Sure, as long as the linker dependencies are fixed soon we'll be OK.

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

[Bruno Haible]

Ondrej Valousek wrote:
> Hi Bruno/Paul,
> 
> Are we going to commit the proposed changes there or is there still anything missing?
> I am asking because I'd like to file a RFE for redhat coreutils improvement and this change depends on it.

Paul gave his OK in off-list email. So I pushed your patch, excluding
the LIBs part, which I'm going to do next.

Bruno

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

[Bruno Haible]

[Re-adding bug-gnulib in CC]

Paul Eggert wrote:
> > - similar vulnerability does exist in the old code, too
> ...
> But really, isn't it *odd* that there's no way to copy a file securely 
> with ACLs (either with xattr or without)? What's up with that? Didn't 
> ACL/xattr designers think about copying files?

There is a way to do it securely; _we_ just haven't thought about how to
do it securely so far.

When I added the 'copy-file' module in 2003, it did not handle ACLs.
Then, when I added ACL support to it in 2006, I left open a security
hole (namely when the destination file already exists and has an
ALLOWing ACL set): we don't specify O_EXCL here, nor do we delete the
ACL first.

And there's a second case, namely the uses of copy_acl from GNU coreutils...

I think, to handle this in full generality, we need to decompose an ACL
into an ALLOWing ACL and a DENYing ACL. Then, when writing to a file
that already exists and potentially has an ACL, we need to proceed in
these phases:
  1. remove the ALLOWing part of the old ACL,
  2. add the DENYing part of the new ACL,
  3. copy the data,
  4. remove the DENYing part of the old ACL (as far as not also contained
     in the new ACL),
  5. add the ALLOWing part of the new ACL.

Something like that, no?

Bruno

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

[Paul Eggert]

On 1/12/23 12:58, Bruno Haible wrote:
>    1. remove the ALLOWing part of the old ACL,
>    2. add the DENYing part of the new ACL,
>    3. copy the data,
>    4. remove the DENYing part of the old ACL (as far as not also contained
>       in the new ACL),
>    5. add the ALLOWing part of the new ACL.
> 
> Something like that, no?

Yes, this sounds good, although things are more complicated on systems 
where ACLs and the traditional Unix permissions are set separately and 
the two interact.

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

[Bruno Haible]

Hi Ondrej,

How did you test your patch? When I run

  $ ./gnulib-tool --create-testdir --dir=../testdir1 --single-configure \
                  acl-permissions copy-file qcopy-acl qset-acl supersede acl

(with autoconf 2.71), I get two warnings:

  autoheader: warning: missing template: USE_XATTR
  autoheader: warning: Use AC_DEFINE([USE_XATTR], [], [Description])

This patch fixes them. But I would have expected that you fix it before
you submit the patch.


2023-01-13  Bruno Haible  <bruno@clisp.org>

	qcopy-acl: Avoid autoconf warning.
	* m4/xattr.m4 (gl_FUNC_XATTR): Provide a description in AC_DEFINE.
	Also, protect against unsafe use of a configure option value.

diff --git a/m4/xattr.m4 b/m4/xattr.m4
index 6efdfa475a..5cbfc86002 100644
--- a/m4/xattr.m4
+++ b/m4/xattr.m4
@@ -1,5 +1,5 @@
 # xattr.m4 - check for Extended Attributes (Linux)
-# serial 4
+# serial 5
 
 # Copyright (C) 2003-2021 Free Software Foundation, Inc.
 # This file is free software; the Free Software Foundation
@@ -16,7 +16,7 @@ AC_DEFUN([gl_FUNC_XATTR],
   LIB_XATTR=
   AC_SUBST([LIB_XATTR])
 
-  if test "$use_xattr" = "yes"; then
+  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 \
@@ -36,7 +36,8 @@ AC_DEFUN([gl_FUNC_XATTR],
       AC_MSG_WARN([AC_PACKAGE_NAME will be built without xattr support.])
     fi
   fi
-  if test $use_xattr = yes; then
-    AC_DEFINE_UNQUOTED([USE_XATTR], 1)
+  if test "$use_xattr" = yes; then
+    AC_DEFINE([USE_XATTR], [1],
+      [Define to 1 to use the Linux extended attributes library.])
   fi
 ])

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

[Bruno Haible]

This part
> Also, protect against unsafe use of a configure option value.
is needed when the user does

  ./configure --enable-xattr="no no"

In this case we don't want to see an error during configuration:

  checking for opendir... yes
  ../configure: line 29819: test: too many arguments
  checking for raise... yes

Always putting $use_attr in double-quotes achieves this.

Bruno

RE: [PATCH] Use xattr (Linux) in qcopy-acl.c

[Ondrej Valousek]

Hi Bruno,

Ok thanks for the good finding - my bad.

To be honest, I did not test the automake stuff, I only tested the functionality via coreutils.
In coreutils they have their own xattr.m4 which basically does the same.
Do I need to resubmit another patch with your changes included?

Thanks,
Ondrej

-----Original Message-----
From: Bruno Haible <bruno@clisp.org> 
Sent: pátek 13. ledna 2023 9:10
To: bug-gnulib@gnu.org; Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Subject: Re: [PATCH] Use xattr (Linux) in qcopy-acl.c

This part
> Also, protect against unsafe use of a configure option value.
is needed when the user does

  ./configure --enable-xattr="no no"

In this case we don't want to see an error during configuration:

  checking for opendir... yes
  ../configure: line 29819: test: too many arguments
  checking for raise... yes

Always putting $use_attr in double-quotes achieves this.

Bruno

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

[Bruno Haible]

Paul Eggert wrote:
> Sure, as long as the linker dependencies are fixed soon we'll be OK.

The linker dependencies are fixed through this patch. It was only a
matter of
1) going through all reverse module dependencies,
2) building a testdir of these modules with --enable-xattr and looking
   out for link errors.


2023-01-13  Bruno Haible  <bruno@clisp.org>

	qcopy-acl: Adjust link dependencies.
	* modules/qcopy-acl (Depends-on): Add condition.
	(configure.ac): Set QCOPY_ACL_LIB.
	(Link): Add $(QCOPY_ACL_LIB). Remove $(LIB_ACL).
	* modules/qacl (Link): Add $(LIB_ACL) and $(QCOPY_ACL_LIB).
	* modules/acl (Link): Add $(LIB_ACL) and $(QCOPY_ACL_LIB).
	* modules/copy-file (Link): Add $(QCOPY_ACL_LIB).
	* modules/supersede (Link): Add $(QCOPY_ACL_LIB).
	* modules/acl-tests (Makefile.am): Link test-copy-acl with
	$(QCOPY_ACL_LIB).
	* modules/copy-file-tests (Makefile.am): Link test-copy-file with
	$(QCOPY_ACL_LIB).
	* modules/supersede-tests (Makefile.am): Link test-supersede with
	$(QCOPY_ACL_LIB).

diff --git a/modules/acl b/modules/acl
index 8bcfd99420..bcc1f4906c 100644
--- a/modules/acl
+++ b/modules/acl
@@ -22,6 +22,8 @@ Include:
 
 Link:
 $(MBRTOWC_LIB)
+$(LIB_ACL)
+$(QCOPY_ACL_LIB)
 
 License:
 GPL
diff --git a/modules/acl-tests b/modules/acl-tests
index 44190b607d..2ee34c6523 100644
--- a/modules/acl-tests
+++ b/modules/acl-tests
@@ -26,5 +26,5 @@ TESTS += \
 TESTS_ENVIRONMENT += USE_ACL=$(USE_ACL)
 check_PROGRAMS += test-set-mode-acl test-copy-acl test-sameacls
 test_set_mode_acl_LDADD = $(LDADD) $(LIB_ACL) @LIBINTL@ $(MBRTOWC_LIB)
-test_copy_acl_LDADD = $(LDADD) $(LIB_ACL) @LIBINTL@ $(MBRTOWC_LIB)
+test_copy_acl_LDADD = $(LDADD) $(LIB_ACL) $(QCOPY_ACL_LIB) @LIBINTL@ $(MBRTOWC_LIB)
 test_sameacls_LDADD = $(LDADD) $(LIB_ACL) @LIBINTL@ $(MBRTOWC_LIB)
diff --git a/modules/copy-file b/modules/copy-file
index dac8bd6358..a73e2a474c 100644
--- a/modules/copy-file
+++ b/modules/copy-file
@@ -37,6 +37,7 @@ Include:
 
 Link:
 $(LIB_ACL)
+$(QCOPY_ACL_LIB)
 $(MBRTOWC_LIB)
 
 License:
diff --git a/modules/copy-file-tests b/modules/copy-file-tests
index 1c554696f9..087797c727 100644
--- a/modules/copy-file-tests
+++ b/modules/copy-file-tests
@@ -18,4 +18,4 @@ Makefile.am:
 TESTS += test-copy-file.sh test-copy-file-1.sh test-copy-file-2.sh
 TESTS_ENVIRONMENT += USE_ACL=$(USE_ACL)
 check_PROGRAMS += test-copy-file
-test_copy_file_LDADD = $(LDADD) $(LIB_ACL) $(CLOCK_TIME_LIB) @LIBINTL@ $(MBRTOWC_LIB)
+test_copy_file_LDADD = $(LDADD) $(LIB_ACL) $(QCOPY_ACL_LIB) $(CLOCK_TIME_LIB) @LIBINTL@ $(MBRTOWC_LIB)
diff --git a/modules/qacl b/modules/qacl
index 45753c7623..445f639f7b 100644
--- a/modules/qacl
+++ b/modules/qacl
@@ -15,6 +15,10 @@ Makefile.am:
 
 Include:
 
+Link:
+$(LIB_ACL)
+$(QCOPY_ACL_LIB)
+
 License:
 GPL
 
diff --git a/modules/qcopy-acl b/modules/qcopy-acl
index e0cd914953..8dd147927d 100644
--- a/modules/qcopy-acl
+++ b/modules/qcopy-acl
@@ -6,10 +6,16 @@ lib/qcopy-acl.c
 m4/xattr.m4
 
 Depends-on:
-acl-permissions
+acl-permissions [test "$use_xattr" != yes]
 
 configure.ac:
 gl_FUNC_XATTR
+if test "$use_xattr" = yes; then
+  QCOPY_ACL_LIB="$LIB_XATTR"
+else
+  QCOPY_ACL_LIB="$LIB_ACL"
+fi
+AC_SUBST([QCOPY_ACL_LIB])
 
 Makefile.am:
 lib_SOURCES += qcopy-acl.c
@@ -18,7 +24,7 @@ Include:
 "acl.h"
 
 Link:
-$(LIB_ACL)
+$(QCOPY_ACL_LIB)
 
 License:
 GPL
diff --git a/modules/supersede b/modules/supersede
index 3118fad426..1f434fe221 100644
--- a/modules/supersede
+++ b/modules/supersede
@@ -36,6 +36,7 @@ Include:
 
 Link:
 $(LIB_ACL)
+$(QCOPY_ACL_LIB)
 $(GETRANDOM_LIB)
 $(CLOCK_TIME_LIB)
 $(LIBTHREAD)
diff --git a/modules/supersede-tests b/modules/supersede-tests
index f3a9d777d9..d4b0566b0d 100644
--- a/modules/supersede-tests
+++ b/modules/supersede-tests
@@ -19,4 +19,4 @@ configure.ac:
 Makefile.am:
 TESTS += test-supersede
 check_PROGRAMS += test-supersede
-test_supersede_LDADD = $(LDADD) $(LIB_ACL) $(CLOCK_TIME_LIB) $(GETRANDOM_LIB) $(LIBINTL) $(LIBTHREAD)
+test_supersede_LDADD = $(LDADD) $(LIB_ACL) $(QCOPY_ACL_LIB) $(CLOCK_TIME_LIB) $(GETRANDOM_LIB) $(LIBINTL) $(LIBTHREAD)

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

[Bruno Haible]

Hi Ondrej,

> To be honest, I did not test the automake stuff, I only tested the functionality via coreutils.

Testing via a Gnulib testdir is also necessary:
  * For testing the module descriptions. When you test via coreutils, you
    are testing only a specific use. You will rarely detect a missing module
    dependency when testing via coreutils only.
  * It encourages adding unit tests to Gnulib. These unit tests in return
    make it less important to test via coreutils.

> In coreutils they have their own xattr.m4 which basically does the same.

Then the one in coreutils will have to be deleted, because it conflicts
with the one from Gnulib.

> Do I need to resubmit another patch with your changes included?

For Gnulib, I pushed my change. For coreutils, I don't know what you have
submitted so far.

Bruno

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

[Bruno Haible]

I wrote:
> 	* modules/qcopy-acl (Depends-on): Add condition.
> 	(configure.ac): Set QCOPY_ACL_LIB.
> 	(Link): Add $(QCOPY_ACL_LIB). Remove $(LIB_ACL).

Oops, this has a mistake. The variable LIB_ACL needs to be initialized
before the code uses its value; otherwise $LIB_ACL will come out as empty.
The way to order the initialization is to use AC_REQUIRE of the macro
which defines LIB_ACL. Then, since invocation and AC_REQUIRE of a macro
defined through AC_DEFUN triggers an autoconf warning, one needs AC_DEFUN_ONCE
instead.


2023-01-13  Bruno Haible  <bruno@clisp.org>

	qcopy-acl: Make last patch more robust.
	* m4/acl.m4 (gl_FUNC_ACL): Define through AC_DEFUN_ONCE.
	* modules/qcopy-acl (configure.ac): Require gl_FUNC_ACL.

diff --git a/m4/acl.m4 b/m4/acl.m4
index e612f1ae34..dc9853a156 100644
--- a/m4/acl.m4
+++ b/m4/acl.m4
@@ -1,5 +1,5 @@
 # acl.m4 - check for access control list (ACL) primitives
-# serial 26
+# serial 27
 
 # Copyright (C) 2002, 2004-2023 Free Software Foundation, Inc.
 # This file is free software; the Free Software Foundation
@@ -17,7 +17,7 @@ AC_DEFUN([gl_FUNC_ACL_ARG],
 ])
 
 
-AC_DEFUN([gl_FUNC_ACL],
+AC_DEFUN_ONCE([gl_FUNC_ACL],
 [
   AC_REQUIRE([gl_FUNC_ACL_ARG])
   AC_CHECK_FUNCS_ONCE([fchmod])
diff --git a/modules/qcopy-acl b/modules/qcopy-acl
index 8dd147927d..b89d8ecab6 100644
--- a/modules/qcopy-acl
+++ b/modules/qcopy-acl
@@ -10,6 +10,7 @@ acl-permissions [test "$use_xattr" != yes]
 
 configure.ac:
 gl_FUNC_XATTR
+AC_REQUIRE([gl_FUNC_ACL])
 if test "$use_xattr" = yes; then
   QCOPY_ACL_LIB="$LIB_XATTR"
 else

Re: ACL complexity

[Bruno Haible]

Paul Eggert wrote (when talking about libxattr and such):
> No kidding. This stuff is waaaayy too complicated.

More generally, I find the semantics and the syntax of ACLs on most
systems to be more demanding than what the average command-line user can
grok. While for random features of the OS this would just be a nuisance
that can be ignored, for a feature with impact on security this is a
major problem.

What I mean is:

1) The syntax.

# getfacl /tmp/file
getfacl: Removing leading '/' from absolute path names  (<< what is this about?)
# file: tmp/file
# owner: test1
# group: test
user::rw-
user:test3:rw-
group::rw-
mask::rw-
other::---

A sysadmin may understand this, but an average command-line user won't.

Suggestion: Add a mode to 'ls' (not to getfacl, because average users
know about 'ls' only) that displays the same info with explanations.
It doesn't matter if the output is 25 lines instead of 8 lines, in this
mode.

2) The semantics.

What are "effective" permissions
https://tylersguides.com/guides/linux-acl-permissions-tutorial/ ?

Suggestion: Provide a kind of "testing toolbox" to the users, which they
can use to simulate what happens when someone tries to access an existing
or new file, after they have set specific permissions and ACLs.

Bruno

RE: ACL complexity

[Ondrej Valousek]

> More generally, I find the semantics and the syntax of ACLs on most systems to be more demanding than what the average command-line user can grok. 
Completely agree here, especially true for so-called posix draft ACLs.
> While for random features of the OS this would just be a nuisance that can be ignored, for a feature with impact on security this is a major problem.
Yes

> Suggestion: Add a mode to 'ls' (not to getfacl, because average users know about 'ls' only) that displays the same info with explanations.
It doesn't matter if the output is 25 lines instead of 8 lines, in this mode.

Well, I was thinking about it as well, see how nicely the OpenSolaris/OmniOS does it using it's -V option:
root@omnios:/mnt# ls -lV acl
-rw-r--r--+  1 root     root           5 Jan  4 09:11 acl
            user:ondrej:rwx-----------:-------:allow
                 owner@:rw-p--aARWcCos:-------:allow
                 group@:r-----a-R-c--s:-------:allow
              everyone@:r-----a-R-c--s:-------:allow

maybe we could reuse some of the code from there (not sure about the legal stuff). The problem is also that I'd like to include support for NFS4 acls, and since Linux libacl does not have it, we'd have to stick in Gnulib probably.

Re: ACL complexity

[Bruno Haible]

Ondrej Valousek wrote:
> Well, I was thinking about it as well, see how nicely the OpenSolaris/OmniOS does it using it's -V option:
> root@omnios:/mnt# ls -lV acl
> -rw-r--r--+  1 root     root           5 Jan  4 09:11 acl
>             user:ondrej:rwx-----------:-------:allow
>                  owner@:rw-p--aARWcCos:-------:allow
>                  group@:r-----a-R-c--s:-------:allow
>               everyone@:r-----a-R-c--s:-------:allow

That's nice. Indeed a tabular presentation helps in making the behaviour
understandable.

> maybe we could reuse some of the code from there (not sure about the legal stuff).

This table-like printout should be easy to program; no need to copy code if
that code is not under GPL.

Bruno

RE: ACL complexity

[Ondrej Valousek]

Still, this wouln't go to Gnulib, but mostly to coreutils, right?

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

[Paul Eggert]

On 2023-01-13 00:46, Bruno Haible wrote:
> Then the one in coreutils will have to be deleted, because it conflicts
> with the one from Gnulib.

Thanks for mentioning that. I deleted coreutils/m4/xattr.m4 and synced 
coreutils from Gnulib.

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

[Ondrej Valousek]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

Why this conversation (+the whole one from yesterday) is missing from the bug-gnulib mail archives?



Zasláno z Outlooku pro Android<https://aka.ms/AAb9ysg>
________________________________
From: Paul Eggert <eggert@cs.ucla.edu>
Sent: Saturday, January 14, 2023 2:52:27 AM
To: Bruno Haible <bruno@clisp.org>; Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Cc: bug-gnulib@gnu.org <bug-gnulib@gnu.org>
Subject: Re: [PATCH] Use xattr (Linux) in qcopy-acl.c

On 2023-01-13 00:46, Bruno Haible wrote:
> Then the one in coreutils will have to be deleted, because it conflicts
> with the one from Gnulib.

Thanks for mentioning that. I deleted coreutils/m4/xattr.m4 and synced
coreutils from Gnulib.

[-- Attachment #2: Type: text/html, Size: 1615 bytes --]

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

[Bruno Haible]

> Why this conversation (+the whole one from yesterday) is missing from the bug-gnulib mail archives?

https://lists.gnu.org/archive/html/bug-gnulib/2023-01/threads.html
looks good to me. Did you press "Refresh" in the browser?

Bruno

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

[Ondrej Valousek]

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

I see some new messages but not a single one about the ACL stuff we discussed (I know that most of it was private, but not everything) yesterday.
Anyway, if it works ok for you, then fine.

Zasláno z Outlooku pro Android<https://aka.ms/AAb9ysg>
________________________________
From: Bruno Haible <bruno@clisp.org>
Sent: Saturday, January 14, 2023 8:55:25 AM
To: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Cc: bug-gnulib@gnu.org <bug-gnulib@gnu.org>
Subject: Re: [PATCH] Use xattr (Linux) in qcopy-acl.c

> Why this conversation (+the whole one from yesterday) is missing from the bug-gnulib mail archives?

https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.gnu.org%2Farchive%2Fhtml%2Fbug-gnulib%2F2023-01%2Fthreads.html&data=05%7C01%7Condrej.valousek.xm%40renesas.com%7Caecfc737015049c44f3008daf604b876%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638092797384382480%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=rqPiVZMUwnrh%2BAu5M%2BZ7QZFoDB8C%2BKyAIAgQV2m5Knc%3D&reserved=0
looks good to me. Did you press "Refresh" in the browser?

Bruno




[-- Attachment #2: Type: text/html, Size: 2333 bytes --]

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

[Bruno Haible]

> 2023-01-13  Bruno Haible  <bruno@clisp.org>
> 
> 	qcopy-acl: Adjust link dependencies.
> 	* modules/qcopy-acl (Depends-on): Add condition.
> 	(configure.ac): Set QCOPY_ACL_LIB.
> 	(Link): Add $(QCOPY_ACL_LIB). Remove $(LIB_ACL).
> 	* modules/qacl (Link): Add $(LIB_ACL) and $(QCOPY_ACL_LIB).
> 	* modules/acl (Link): Add $(LIB_ACL) and $(QCOPY_ACL_LIB).
> 	* modules/copy-file (Link): Add $(QCOPY_ACL_LIB).
> 	* modules/supersede (Link): Add $(QCOPY_ACL_LIB).

I forgot to add a NEWS entry for these changes. Adding it now:


2023-01-30  Bruno Haible  <bruno@clisp.org>

	Update NEWS.
	* NEWS: Update for qcopy-acl change on 2023-01-13.

diff --git a/NEWS b/NEWS
index e8cc0c4ae7..f3b508f950 100644
--- a/NEWS
+++ b/NEWS
@@ -79,6 +79,13 @@ Date        Modules         Changes
 
 2023-01-15  stdalign        This module is deprecated.  Use alignasof instead.
 
+2023-01-13  acl             Link additionally with $(QCOPY_ACL_LIB).
+            qacl
+            copy-file
+            supersede
+
+2023-01-13  qcopy-acl       Link with $(QCOPY_ACL_LIB) instead of $(LIB_ACL).
+
 2023-01-07  timer_time      Link with $(TIMER_TIME_LIB) instead of
                             $(LIB_TIMER_TIME).