On 13/04/2024 18:42, Pádraig Brady wrote:
On 13/04/2024 17:39, Pádraig Brady wrote:
install(1) defaults to mode 600 for new files, and uses set_acl() with that
(since 2007 https://github.com/coreutils/coreutils/commit/f634e8844 )
The psuedo code that install(1) uses is:
copy_reg()
if (x->set_mode) /* install */
set_acl(dest, x->mode /* 600 */)
ctx->acl = acl_from_mode ( /* 600 */)
acl_set_fd (ctx->acl) /* fails EACCES */
if (! acls_set)
must_chmod = true;
if (must_chmod)
saved_errno = EACCES;
chmod (ctx->mode /* 600 */)
if (save_errno)
return -1;
This issue only only seems to be on CIFS.
I'm seeing lot of weird behavior with ACLs there:
acl_set_fd (acl_from_mode (600)) -> EACCES
acl_set_fd (acl_from_mode (755)) -> EINVAL
getxattr ("system.posix_acl_access") -> EOPNOTSUPP
Note we ignore EINVAL and EOPNOTSUPP errors in set_acl(),
and it's just the EACCES that's problematic.
Note this is quite similar to https://debbugs.gnu.org/65599
where Paul also noticed EACCES with fsetxattr() (and others) on CIFS.
The attached is a potential solution which I tested as working
on the same matoro system that Bruno used.
I think I'll apply that after thinking a bit more about it.
Actually that probably isn't appropriate,
as fsetxattr() can validly return EACESS
even though not documented in the man page at least.
The following demonstrates that:
.
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/xattr.h>
#include <fcntl.h>
#include <attr/libattr.h>
#include <stdio.h>
#include <unistd.h>
int main(void)
{
int wfd;
/* Note S_IWUSR is not set. */
if ((wfd=open("writable", O_CREAT|O_WRONLY|O_EXCL, S_IRUSR)) == -1)
fprintf(stderr, "open('writable') error [%m]\n");
if (write(wfd, "data", 1) == -1)
fprintf(stderr, "write() error [%m]\n");
if (fsetxattr(wfd, "user.test", "test", 4, 0) == -1)
fprintf(stderr, "fsetxattr() error [%m]\n");
}
Another solution might be to file_has_acl() in copy.c
and skip if "not supported" or nothing is returned.
The following would do that, but I'm not sure about this
(as I'm not sure about ACLs in general TBH).
Note listxattr() returns 0 on CIFS here,
while getxattr ("system.posix_acl_access") returns EOPNOTSUPP,
and file_has_acl() uses listxattr() first.
diff --git a/src/copy.c b/src/copy.c
index d584a27eb..2145d89d5 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1673,8 +1673,13 @@ set_dest_mode:
}
else if (x->set_mode)
{
- if (set_acl (dst_name, dest_desc, x->mode) != 0)
- return_val = false;
+ errno = 0;
+ int n = file_has_acl (dst_name, &sb);
+ if (0 < n || (errno && ! (is_ENOTSUP (errno) || errno == ENOSYS)))
+ {
+ if (set_acl (dst_name, dest_desc, x->mode) != 0)
+ return_val = false;
+ }
}
BTW I'm surprised this wasn't reported previously for CIFS,
so I due to this bug and https://debbugs.gnu.org/65599
I suspect a recentish change in CIFS wrt EACCES.
Thinking more about this, the solution presented above wouldn't work,
and I think this needs to be addressed in CIFS.
I.e. we may still need to reset the mode even if the file has no ACLs,
as generally only dirs get default ACLs copied, as demonstrated below:
$ mkdir acl$ setfacl -d -m o::rw acl
$ getfacl acl
# file: acl
# owner: padraig
# group: padraig
user::rwx
group::r-x
other::r-x
default:user::rwx
default:group::r-x
default:other::rw-
$ touch acl/file
$ ls -l acl/file
-rw-r--rw-. 1 padraig padraig 0 May 9 13:11 acl/file
$ getfacl acl/file
# file: acl/file
# owner: padraig
# group: padraig
user::rw-
group::r--
other::rw-
cheers,
Pádraig.
