[PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Florian Weimer]

These patches use the O_PATH hack to implement a fairly close
approximation of fchmodat in AT_SYMLINK_NOFOLLOW mode.

musl has an implementation of fchmodat which works on paths which do not
refer to symbolic links, so this addresses a portability hazard.

Thanks,
Florian

Florian Weimer (5):
  support: Add the xlstat function
  io: Implement lchmod using fchmodat [BZ #14578]
  Linux: Emulate fchmodat with AT_SYMLINK_NOFOLLOW using O_PATH [BZ
    #14578]
  io: Add io/tst-lchmod covering lchmod and fchmodat
  Linux: Add op/tst-o_path-locks

 include/sys/stat.h                         |   1 +
 io/Makefile                                |   2 +-
 io/fchmodat.c                              |   1 +
 io/lchmod.c                                |  10 +-
 io/tst-lchmod.c                            | 309 +++++++++++++++++++++
 support/Makefile                           |   1 +
 support/xlstat.c                           |  28 ++
 support/xunistd.h                          |   1 +
 sysdeps/mach/hurd/fchmodat.c               |   1 +
 sysdeps/unix/sysv/linux/Makefile           |   2 +-
 sysdeps/unix/sysv/linux/fchmodat.c         |  62 ++++-
 sysdeps/unix/sysv/linux/tst-o_path-locks.c | 100 +++++++
 12 files changed, 500 insertions(+), 18 deletions(-)
 create mode 100644 io/tst-lchmod.c
 create mode 100644 support/xlstat.c
 create mode 100644 sysdeps/unix/sysv/linux/tst-o_path-locks.c

-- 
2.24.1

[PATCH 1/5] support: Add the xlstat function

[Florian Weimer]

---
 support/Makefile  |  1 +
 support/xlstat.c  | 28 ++++++++++++++++++++++++++++
 support/xunistd.h |  1 +
 3 files changed, 30 insertions(+)
 create mode 100644 support/xlstat.c

diff --git a/support/Makefile b/support/Makefile
index 3325feb790..a0304e6def 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -98,6 +98,7 @@ libsupport-routines = \
   xgetsockname \
   xlisten \
   xlseek \
+  xlstat \
   xmalloc \
   xmemstream \
   xmkdir \
diff --git a/support/xlstat.c b/support/xlstat.c
new file mode 100644
index 0000000000..de45ef3df2
--- /dev/null
+++ b/support/xlstat.c
@@ -0,0 +1,28 @@
+/* lstat64 with error checking.
+   Copyright (C) 2017-2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <support/check.h>
+#include <support/xunistd.h>
+#include <sys/stat.h>
+
+void
+xlstat (const char *path, struct stat64 *result)
+{
+  if (lstat64 (path, result) != 0)
+    FAIL_EXIT1 ("lstat64 (\"%s\"): %m", path);
+}
diff --git a/support/xunistd.h b/support/xunistd.h
index 96f498f2e5..b299db77ba 100644
--- a/support/xunistd.h
+++ b/support/xunistd.h
@@ -36,6 +36,7 @@ void xpipe (int[2]);
 void xdup2 (int, int);
 int xopen (const char *path, int flags, mode_t);
 void xstat (const char *path, struct stat64 *);
+void xlstat (const char *path, struct stat64 *);
 void xfstat (int fd, struct stat64 *);
 void xmkdir (const char *path, mode_t);
 void xchroot (const char *path);
-- 
2.24.1

[PATCH 2/5] io: Implement lchmod using fchmodat [BZ #14578]

[Florian Weimer]

---
 include/sys/stat.h                 |  1 +
 io/fchmodat.c                      |  1 +
 io/lchmod.c                        | 10 ++++------
 sysdeps/mach/hurd/fchmodat.c       |  1 +
 sysdeps/unix/sysv/linux/fchmodat.c |  1 +
 5 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/sys/stat.h b/include/sys/stat.h
index b82d452780..92284ca48b 100644
--- a/include/sys/stat.h
+++ b/include/sys/stat.h
@@ -9,6 +9,7 @@ extern int __lstat (const char *__file, struct stat *__buf);
 extern int __chmod (const char *__file, __mode_t __mode);
 libc_hidden_proto (__chmod)
 extern int __fchmod (int __fd, __mode_t __mode);
+libc_hidden_proto (fchmodat)
 extern __mode_t __umask (__mode_t __mask);
 extern int __mkdir (const char *__path, __mode_t __mode);
 libc_hidden_proto (__mkdir)
diff --git a/io/fchmodat.c b/io/fchmodat.c
index 7f3a07aaa2..78895ac187 100644
--- a/io/fchmodat.c
+++ b/io/fchmodat.c
@@ -42,3 +42,4 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
   return -1;
 }
 stub_warning (fchmodat)
+libc_hidden_def (fchmodat)
diff --git a/io/lchmod.c b/io/lchmod.c
index 90b33a49f9..8b788034ee 100644
--- a/io/lchmod.c
+++ b/io/lchmod.c
@@ -1,4 +1,4 @@
-/* lchmod -- Change the protections of a file or symbolic link.  Stub version.
+/* lchmod -- Change the protections of a file or symbolic link.  Generic version.
    Copyright (C) 2002-2020 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -17,15 +17,13 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <errno.h>
-#include <sys/stat.h>
+#include <fcntl.h>
 #include <sys/types.h>
+#include <unistd.h>
 
 /* Change the protections of FILE to MODE.  */
 int
 lchmod (const char *file, mode_t mode)
 {
-  __set_errno (ENOSYS);
-  return -1;
+  return fchmodat (AT_FDCWD, file, mode, AT_SYMLINK_NOFOLLOW);
 }
-
-stub_warning (lchmod)
diff --git a/sysdeps/mach/hurd/fchmodat.c b/sysdeps/mach/hurd/fchmodat.c
index cd227d5c83..d42f9520e9 100644
--- a/sysdeps/mach/hurd/fchmodat.c
+++ b/sysdeps/mach/hurd/fchmodat.c
@@ -37,3 +37,4 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
     return __hurd_fail (err);
   return 0;
 }
+libc_hidden_def (fchmodat)
diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
index 224439ffba..c41ebb290d 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -38,3 +38,4 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
 
   return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
 }
+libc_hidden_def (fchmodat)
-- 
2.24.1

[PATCH 3/5] Linux: Emulate fchmodat with AT_SYMLINK_NOFOLLOW using O_PATH [BZ #14578]

[Florian Weimer]

/proc/self/fd files are special and chmod on O_PATH descriptors
in that directory operates on the symbolic link itself (like lchmod).
---
 sysdeps/unix/sysv/linux/fchmodat.c | 61 +++++++++++++++++++++++++-----
 1 file changed, 51 insertions(+), 10 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
index c41ebb290d..ac318ceb79 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -18,24 +18,65 @@
 
 #include <errno.h>
 #include <fcntl.h>
-#include <stddef.h>
+#include <not-cancel.h>
 #include <stdio.h>
-#include <string.h>
-#include <unistd.h>
+#include <sys/stat.h>
 #include <sys/types.h>
-#include <alloca.h>
 #include <sysdep.h>
+#include <unistd.h>
 
 int
 fchmodat (int fd, const char *file, mode_t mode, int flag)
 {
-  if (flag & ~AT_SYMLINK_NOFOLLOW)
+  if (flag == 0)
+    return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+  else if (flag != AT_SYMLINK_NOFOLLOW)
     return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
-#ifndef __NR_lchmod		/* Linux so far has no lchmod syscall.  */
-  if (flag & AT_SYMLINK_NOFOLLOW)
-    return INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOTSUP);
-#endif
+  else
+    {
+      /* The kernel system call does not have a mode argument.
+	 However, we can create an O_PATH descriptor and change that
+	 via /proc (which does not resolve symbolic links).  */
+
+      int pathfd = __openat_nocancel (fd, file,
+				      O_PATH | O_NOFOLLOW | O_CLOEXEC);
+      if (pathfd < 0)
+	{
+	  if (errno == ENFILE || errno == EMFILE)
+	    /* These errors cannot happen with a straight fchmodat
+	       operation because it does not create file descriptors,
+	       so hide them.  */
+	    __set_errno (EOPNOTSUPP);
+	  /* Otherwise, this should accurately reflect the expected
+	     error from fchmodat (e.g., EBADF or ENOENT).  */
+	  return pathfd;
+	}
+
+      char buf[32];
+      if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
+	{
+	  __close_nocancel (pathfd);
+	  return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOPNOTSUPP);
+	}
 
-  return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+      /* This operates directly on the symbolic link if it is one.
+	 /proc/self/fd files look like symbolic links, but they are
+	 not.  (fchmod and fchmodat do not work on O_PATH descriptors,
+	 similar to fstat before Linux 3.6.)  */
+      int ret = __chmod (buf, mode);
+      if (ret != 0)
+	{
+	  if (errno == ENOENT)
+	    /* /proc has not been mounted.  In general, we cannot use
+	       openat with AT_EMPTY_PATH to upgrade the descriptor
+	       because we may not have permission to open the file,
+	       and opening files and closing them again may have side
+	       effects (such as rewinding tape devices, or releasing
+	       POSIX locks).  */
+	    __set_errno (EOPNOTSUPP);
+	}
+      __close_nocancel (pathfd);
+      return ret;
+    }
 }
 libc_hidden_def (fchmodat)
-- 
2.24.1

Re: [PATCH 3/5] Linux: Emulate fchmodat with AT_SYMLINK_NOFOLLOW using O_PATH [BZ #14578]

[Paul Eggert]

On 1/22/20 12:03 PM, Florian Weimer wrote:
> +	  if (errno == ENFILE || errno == EMFILE)
> +	    /* These errors cannot happen with a straight fchmodat
> +	       operation because it does not create file descriptors,
> +	       so hide them.  */
> +	    __set_errno (EOPNOTSUPP);
> +	  /* Otherwise, this should accurately reflect the expected
> +	     error from fchmodat (e.g., EBADF or ENOENT).  */

These lines can be omitted. I omitted them when recently adding similar code to 
Gnulib in <https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/fchmodat.c>. 
There's no need for fchmodat to masquerade ENFILE and EMFILE as EOPNOTSUPP: 
POSIX does not require it and masquerading those two errno values would be 
misleading, in the sense that it would make it harder for the caller to diagnose 
what actually went wrong.

> +      char buf[32];
> +      if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
> +	{
> +	  __close_nocancel (pathfd);
> +	  return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOPNOTSUPP);
> +	}

Similarly here. Also, there should be no reason for snprintf to fail. One 
possibility is to replace this with what's in Gnulib:

   static char const fmt[] = "/proc/self/fd/%d";
   char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];
   sprintf (buf, fmt, pathfd);

where INT_BUFSIZE_BOUND is taken from <intprops.h>.

Otherwise, this patch series looks OK to me; thanks for writing it.

Re: [PATCH 3/5] Linux: Emulate fchmodat with AT_SYMLINK_NOFOLLOW using O_PATH [BZ #14578]

[Florian Weimer]

* Paul Eggert:

> On 1/22/20 12:03 PM, Florian Weimer wrote:
>> +	  if (errno == ENFILE || errno == EMFILE)
>> +	    /* These errors cannot happen with a straight fchmodat
>> +	       operation because it does not create file descriptors,
>> +	       so hide them.  */
>> +	    __set_errno (EOPNOTSUPP);
>> +	  /* Otherwise, this should accurately reflect the expected
>> +	     error from fchmodat (e.g., EBADF or ENOENT).  */
>
> These lines can be omitted. I omitted them when recently adding similar code to 
> Gnulib in <https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/fchmodat.c>. 
> There's no need for fchmodat to masquerade ENFILE and EMFILE as EOPNOTSUPP: 
> POSIX does not require it and masquerading those two errno values would be 
> misleading, in the sense that it would make it harder for the caller to diagnose 
> what actually went wrong.

Hmm.  The error code is really obscure, particularly with AT_FDCWD,
where there is no file descriptor involved.  I hope that we can
eventually make it go away with proper kernel support.  But I see your
point about telling transient errors (such as ENOMEM) from more
persistent ones.

>> +      char buf[32];
>> +      if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
>> +	{
>> +	  __close_nocancel (pathfd);
>> +	  return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOPNOTSUPP);
>> +	}
>
> Similarly here. Also, there should be no reason for snprintf to fail. One 
> possibility is to replace this with what's in Gnulib:
>
>    static char const fmt[] = "/proc/self/fd/%d";
>    char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];
>    sprintf (buf, fmt, pathfd);

I think we should add a proper wrapper for this eventually, where the
buffer allocation in the caller is properly abstracted via a
suitable struct type.

> where INT_BUFSIZE_BOUND is taken from <intprops.h>.
>
> Otherwise, this patch series looks OK to me; thanks for writing it.

Thanks.

What do you think about introducing new symbol versions vs keeping
things as they are in the patch today?

If we do not introduce new symbol versions, we should strive to
backport this change widely, so that users do not run into obscure
failures.

Re: [PATCH 3/5] Linux: Emulate fchmodat with AT_SYMLINK_NOFOLLOW using O_PATH [BZ #14578]

[Paul Eggert]

On 2/9/20 12:57 AM, Florian Weimer wrote:
> What do you think about introducing new symbol versions vs keeping
> things as they are in the patch today?

I'm not quite following the relationship between the two alternatives. Isn't the 
symbol version independent of whether we keep things as they are now, or install 
the patch that you proposed, or install that patch with my further suggestions? 
That is, I don't see why the patch (or patch variant) would require us to change 
symbol versions.

> If we do not introduce new symbol versions, we should strive to
> backport this change widely, so that users do not run into obscure
> failures.

Yes, backporting would be good. Essentially Gnulib is already doing that, for 
Gnulib-using apps.

Re: [PATCH 3/5] Linux: Emulate fchmodat with AT_SYMLINK_NOFOLLOW using O_PATH [BZ #14578]

[Florian Weimer]

* Paul Eggert:

> On 2/9/20 12:57 AM, Florian Weimer wrote:
>> What do you think about introducing new symbol versions vs keeping
>> things as they are in the patch today?
>
> I'm not quite following the relationship between the two
> alternatives. Isn't the symbol version independent of whether we
> keep things as they are now, or install the patch that you proposed,
> or install that patch with my further suggestions?  That is, I don't
> see why the patch (or patch variant) would require us to change
> symbol versions.

If these functions are now going to be used widely, introducing a new
symbol version at the time of the bug fix ensures that applications do
not accidentally run on glibc versions which lack this change, where
they might fail in obscure ways.

Re: [PATCH 3/5] Linux: Emulate fchmodat with AT_SYMLINK_NOFOLLOW using O_PATH [BZ #14578]

[Florian Weimer]

* Paul Eggert:

> On 2/9/20 12:57 AM, Florian Weimer wrote:
>> What do you think about introducing new symbol versions vs keeping
>> things as they are in the patch today?
>
> I'm not quite following the relationship between the two
> alternatives. Isn't the symbol version independent of whether we keep
> things as they are now, or install the patch that you proposed, or
> install that patch with my further suggestions? That is, I don't see
> why the patch (or patch variant) would require us to change symbol
> versions.
>
>> If we do not introduce new symbol versions, we should strive to
>> backport this change widely, so that users do not run into obscure
>> failures.
>
> Yes, backporting would be good. Essentially Gnulib is already doing
> that, for Gnulib-using apps.

Okay, this is what I'm going to commit soon, with no symbol version
changes.

Thanks,
Florian

8<------------------------------------------------------------------8<
/proc/self/fd files are special and chmod on O_PATH descriptors
in that directory operates on the symbolic link itself (like lchmod).

-----
 sysdeps/unix/sysv/linux/fchmodat.c | 57 +++++++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 10 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
index c41ebb290d..719053b333 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -18,24 +18,61 @@
 
 #include <errno.h>
 #include <fcntl.h>
-#include <stddef.h>
+#include <not-cancel.h>
 #include <stdio.h>
-#include <string.h>
-#include <unistd.h>
+#include <sys/stat.h>
 #include <sys/types.h>
-#include <alloca.h>
 #include <sysdep.h>
+#include <unistd.h>
 
 int
 fchmodat (int fd, const char *file, mode_t mode, int flag)
 {
-  if (flag & ~AT_SYMLINK_NOFOLLOW)
+  if (flag == 0)
+    return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+  else if (flag != AT_SYMLINK_NOFOLLOW)
     return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
-#ifndef __NR_lchmod		/* Linux so far has no lchmod syscall.  */
-  if (flag & AT_SYMLINK_NOFOLLOW)
-    return INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOTSUP);
-#endif
+  else
+    {
+      /* The kernel system call does not have a mode argument.
+	 However, we can create an O_PATH descriptor and change that
+	 via /proc (which does not resolve symbolic links).  */
 
-  return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+      int pathfd = __openat_nocancel (fd, file,
+				      O_PATH | O_NOFOLLOW | O_CLOEXEC);
+      if (pathfd < 0)
+	/* This may report errors such as ENFILE and EMFILE.  The
+	   caller can treat them as temporary if necessary.  */
+	return pathfd;
+
+      char buf[32];
+      if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
+	{
+	  /* This also may report strange error codes to the caller
+	     (although snprintf really should not fail).  */
+	  __close_nocancel (pathfd);
+	  return -1;
+	}
+
+      /* This operates directly on the symbolic link if it is one.
+	 /proc/self/fd files look like symbolic links, but they are
+	 not.  (fchmod and fchmodat do not work on O_PATH descriptors,
+	 similar to fstat before Linux 3.6.)  */
+      int ret = __chmod (buf, mode);
+      if (ret != 0)
+	{
+	  if (errno == ENOENT)
+	    /* /proc has not been mounted.  Without /proc, there is no
+	       way to upgrade the O_PATH descriptor to a full
+	       descriptor.  It is also not possible to re-open the
+	       file without O_PATH because the file name may refer to
+	       another file, and opening that without O_PATH may have
+	       side effects (such as blocking, device rewinding, or
+	       releasing POSIX locks).  */
+	    __set_errno (EOPNOTSUPP);
+	}
+      __close_nocancel (pathfd);
+      return ret;
+    }
 }
 libc_hidden_def (fchmodat)

Re: [PATCH 3/5] Linux: Emulate fchmodat with AT_SYMLINK_NOFOLLOW using O_PATH [BZ #14578]

[Paul Eggert]

Thanks for doing that; looks good.

[PATCH 4/5] io: Add io/tst-lchmod covering lchmod and fchmodat

[Florian Weimer]

---
 io/Makefile     |   2 +-
 io/tst-lchmod.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 310 insertions(+), 1 deletion(-)
 create mode 100644 io/tst-lchmod.c

diff --git a/io/Makefile b/io/Makefile
index d9a1da4566..fe2f8c5065 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -74,7 +74,7 @@ tests		:= test-utime test-stat test-stat2 test-lfs tst-getcwd \
 		   tst-posix_fallocate tst-posix_fallocate64 \
 		   tst-fts tst-fts-lfs tst-open-tmpfile \
 		   tst-copy_file_range tst-getcwd-abspath tst-lockf \
-		   tst-ftw-lnk
+		   tst-ftw-lnk tst-lchmod
 
 # Likewise for statx, but we do not need static linking here.
 tests-internal += tst-statx
diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c
new file mode 100644
index 0000000000..e594030e55
--- /dev/null
+++ b/io/tst-lchmod.c
@@ -0,0 +1,309 @@
+/* Tests for lchmod and fchmodat with AT_SYMLINK_NOFOLLOW.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <array_length.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/descriptors.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+#if __has_include (<sys/mount.h>)
+# include <sys/mount.h>
+#endif
+
+/* Array of file descriptors.  */
+#define DYNARRAY_STRUCT fd_list
+#define DYNARRAY_ELEMENT int
+#define DYNARRAY_INITIAL_SIZE 0
+#define DYNARRAY_PREFIX fd_list_
+#include <malloc/dynarray-skeleton.c>
+
+static int
+fchmodat_with_lchmod (int fd, const char *path, mode_t mode, int flags)
+{
+  TEST_COMPARE (fd, AT_FDCWD);
+  if (flags == 0)
+    return chmod (path, mode);
+  else
+    {
+      TEST_COMPARE (flags, AT_SYMLINK_NOFOLLOW);
+      return lchmod (path, mode);
+    }
+}
+
+/* Chose the appropriate path to pass as the path argument to the *at
+   functions.  */
+static const char *
+select_path (bool do_relative_path, const char *full_path, const char *relative_path)
+{
+  if (do_relative_path)
+    return relative_path;
+  else
+    return full_path;
+}
+
+static void
+test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t, int))
+{
+  char *tempdir = support_create_temp_directory ("tst-lchmod-");
+
+  char *path_dangling = xasprintf ("%s/dangling", tempdir);
+  char *path_file = xasprintf ("%s/file", tempdir);
+  char *path_loop = xasprintf ("%s/loop", tempdir);
+  char *path_missing = xasprintf ("%s/missing", tempdir);
+  char *path_to_file = xasprintf ("%s/to-file", tempdir);
+
+  int fd;
+  if (do_relative_path)
+    fd = xopen (tempdir, O_DIRECTORY | O_RDONLY, 0);
+  else
+    fd = AT_FDCWD;
+
+  add_temp_file (path_dangling);
+  add_temp_file (path_loop);
+  add_temp_file (path_file);
+  add_temp_file (path_to_file);
+
+  support_write_file_string (path_file, "");
+  xsymlink ("file", path_to_file);
+  xsymlink ("loop", path_loop);
+  xsymlink ("target-does-not-exist", path_dangling);
+
+  /* Check that the modes do not collide with what we will use in the
+     test.  */
+  struct stat64 st;
+  xstat (path_file, &st);
+  TEST_VERIFY ((st.st_mode & 0777) != 1);
+  xlstat (path_to_file, &st);
+  TEST_VERIFY ((st.st_mode & 0777) != 2);
+  mode_t original_symlink_mode = st.st_mode;
+
+  /* Set to true if AT_SYMLINK_NOFOLLOW is supported.  */
+  bool nofollow;
+
+  /* We should be able to change the mode of a file, including through
+     the symbolic link to-file.  */
+  const char *arg = select_path (do_relative_path, path_file, "file");
+  TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0);
+  xstat (path_file, &st);
+  TEST_COMPARE (st.st_mode & 0777, 1);
+  int ret = chmod_func (fd, path_file, 2, AT_SYMLINK_NOFOLLOW);
+  if (ret == 0)
+    {
+      printf ("info: AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
+      nofollow = true;
+    }
+  else
+    {
+      printf ("info: no AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
+      nofollow = false;
+
+      /* Set up things for the code below.  */
+      TEST_COMPARE (chmod_func (fd, path_file, 2, 0), 0);
+    }
+  xstat (path_file, &st);
+  TEST_COMPARE (st.st_mode & 0777, 2);
+  arg = select_path (do_relative_path, path_to_file, "to-file");
+  TEST_COMPARE (chmod_func (fd, path_to_file, 1, 0), 0);
+  xstat (path_file, &st);
+  TEST_COMPARE (st.st_mode & 0777, 1);
+  xlstat (path_to_file, &st);
+  TEST_COMPARE (original_symlink_mode, st.st_mode);
+
+  /* Changing the mode of a symbolic link may fail.  */
+  arg = select_path (do_relative_path, path_to_file, "to-file");
+  ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
+  if (nofollow)
+    {
+      TEST_COMPARE (ret, 0);
+
+      /* The mode of the link changed.  */
+      xlstat (path_to_file, &st);
+      TEST_COMPARE (st.st_mode & 0777, 2);
+
+      /* But the mode of the file is unchanged.  */
+      xstat (path_file, &st);
+      TEST_COMPARE (st.st_mode & 0777, 1);
+
+    }
+  else
+    {
+      TEST_COMPARE (ret, -1);
+      TEST_COMPARE (errno, EOPNOTSUPP);
+
+      /* The modes should remain unchanged.  */
+      xstat (path_file, &st);
+      TEST_COMPARE (st.st_mode & 0777, 1);
+      xlstat (path_to_file, &st);
+      TEST_COMPARE (original_symlink_mode, st.st_mode);
+    }
+
+  /* If we have NOFOLLOW support, we should be able to change the mode
+     of a dangling symbolic link or a symbolic link loop.  */
+  const char *paths[] = { path_dangling, path_loop };
+  for (size_t i = 0; i < array_length (paths); ++i)
+    {
+      const char *path = paths[i];
+      const char *filename = strrchr (path, '/');
+      TEST_VERIFY_EXIT (filename != NULL);
+      ++filename;
+      mode_t new_mode = 010 + i;
+
+      xlstat (path, &st);
+      TEST_VERIFY ((st.st_mode & 0777) != new_mode);
+      original_symlink_mode = st.st_mode;
+      arg = select_path (do_relative_path, path, filename);
+      ret = chmod_func (fd, arg, new_mode, AT_SYMLINK_NOFOLLOW);
+      if (nofollow)
+        {
+          TEST_COMPARE (ret, 0);
+          xlstat (path, &st);
+          TEST_COMPARE (st.st_mode & 0777, new_mode);
+        }
+      else /* !nofollow.  */
+        {
+          TEST_COMPARE (ret, -1);
+          TEST_COMPARE (errno, EOPNOTSUPP);
+          xlstat (path, &st);
+          TEST_COMPARE (st.st_mode, original_symlink_mode);
+        }
+    }
+
+   /* A missing file should always result in ENOENT.  The presence of
+      /proc does not matter.  */
+   arg = select_path (do_relative_path, path_missing, "missing");
+   TEST_COMPARE (chmod_func (fd, arg, 020, 0), -1);
+   TEST_COMPARE (errno, ENOENT);
+   TEST_COMPARE (chmod_func (fd, arg, 020, AT_SYMLINK_NOFOLLOW), -1);
+   TEST_COMPARE (errno, ENOENT);
+
+   /* Test without available file descriptors.  */
+   {
+     struct fd_list fd_list;
+     fd_list_init (&fd_list);
+     while (true)
+       {
+         int ret = dup (STDOUT_FILENO);
+         if (ret == -1)
+           {
+             if (errno == ENFILE || errno == EMFILE)
+               break;
+             FAIL_EXIT1 ("dup: %m");
+           }
+         fd_list_add (&fd_list, ret);
+         TEST_VERIFY_EXIT (!fd_list_has_failed (&fd_list));
+       }
+     /* Without AT_SYMLINK_NOFOLLOW, changing the permissions should
+        work as before.  */
+     arg = select_path (do_relative_path, path_file, "file");
+     TEST_COMPARE (chmod_func (fd, arg, 3, 0), 0);
+     xstat (path_file, &st);
+     TEST_COMPARE (st.st_mode & 0777, 3);
+     /* But with AT_SYMLINK_NOFOLLOW, even if we originally had
+        support, we may have lost it.  */
+     ret = chmod_func (fd, arg, 2, 0);
+     if (ret == 0)
+       {
+         xstat (path_file, &st);
+         TEST_COMPARE (st.st_mode & 0777, 2);
+       }
+     else
+       {
+         TEST_COMPARE (ret, -1);
+         TEST_COMPARE (errno, EOPNOTSUPP);
+       }
+     /* Close the descriptors.  */
+     for (int *pfd = fd_list_begin (&fd_list); pfd < fd_list_end (&fd_list);
+          ++pfd)
+       xclose (*pfd);
+     fd_list_free (&fd_list);
+   }
+
+   if (do_relative_path)
+    xclose (fd);
+
+   free (path_dangling);
+   free (path_file);
+   free (path_loop);
+   free (path_missing);
+   free (path_to_file);
+
+   free (tempdir);
+}
+
+static void
+test_3 (void)
+{
+  puts ("info: testing lchmod");
+  test_1 (false, fchmodat_with_lchmod);
+  puts ("info: testing fchmodat with AT_FDCWD");
+  test_1 (false, fchmodat);
+  puts ("info: testing fchmodat with relative path");
+  test_1 (true, fchmodat);
+}
+
+static int
+do_test (void)
+{
+  struct support_descriptors *descriptors = support_descriptors_list ();
+
+  /* Run the three tests in the default environment.  */
+  test_3 ();
+
+  /* Try to set up a /proc-less environment and re-test.  */
+#if __has_include (<sys/mount.h>)
+  if (!support_become_root ())
+    puts ("warning: could not obtain root-like privileges");
+  if (!support_enter_mount_namespace ())
+    puts ("warning: could enter a mount namespace");
+  else
+    {
+      /* Attempt to mount an empty directory over /proc.  */
+      char *tempdir = support_create_temp_directory ("tst-lchmod-");
+      bool proc_emptied
+        = mount (tempdir, "/proc", "none", MS_BIND, NULL) == 0;
+      if (!proc_emptied)
+        printf ("warning: bind-mounting /proc failed: %m");
+      free (tempdir);
+
+      puts ("info: re-running tests (after trying to empty /proc)");
+      test_3 ();
+
+      if (proc_emptied)
+        /* Reveal the original /proc, which is needed by the
+           descriptors check below.  */
+        TEST_COMPARE (umount ("/proc"), 0);
+    }
+#endif /* <sys/mount.h>.  */
+
+  support_descriptors_check (descriptors);
+  support_descriptors_free (descriptors);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.24.1

Re: [PATCH 4/5] io: Add io/tst-lchmod covering lchmod and fchmodat

[Florian Weimer]

* Florian Weimer:

> +     /* Without AT_SYMLINK_NOFOLLOW, changing the permissions should
> +        work as before.  */
> +     arg = select_path (do_relative_path, path_file, "file");
> +     TEST_COMPARE (chmod_func (fd, arg, 3, 0), 0);
> +     xstat (path_file, &st);
> +     TEST_COMPARE (st.st_mode & 0777, 3);
> +     /* But with AT_SYMLINK_NOFOLLOW, even if we originally had
> +        support, we may have lost it.  */
> +     ret = chmod_func (fd, arg, 2, 0);

The last line misses AT_SYMLINK_NOFOLLOW.  The version below (which I'm
going to commit) fixes this and adds another check.

Thanks,
Florian
8<------------------------------------------------------------------8<
-----
 io/Makefile     |   2 +-
 io/tst-lchmod.c | 314 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 315 insertions(+), 1 deletion(-)

diff --git a/io/Makefile b/io/Makefile
index d9a1da4566..fe2f8c5065 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -74,7 +74,7 @@ tests		:= test-utime test-stat test-stat2 test-lfs tst-getcwd \
 		   tst-posix_fallocate tst-posix_fallocate64 \
 		   tst-fts tst-fts-lfs tst-open-tmpfile \
 		   tst-copy_file_range tst-getcwd-abspath tst-lockf \
-		   tst-ftw-lnk
+		   tst-ftw-lnk tst-lchmod
 
 # Likewise for statx, but we do not need static linking here.
 tests-internal += tst-statx
diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c
new file mode 100644
index 0000000000..73e45549af
--- /dev/null
+++ b/io/tst-lchmod.c
@@ -0,0 +1,314 @@
+/* Tests for lchmod and fchmodat with AT_SYMLINK_NOFOLLOW.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <array_length.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/descriptors.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+#if __has_include (<sys/mount.h>)
+# include <sys/mount.h>
+#endif
+
+/* Array of file descriptors.  */
+#define DYNARRAY_STRUCT fd_list
+#define DYNARRAY_ELEMENT int
+#define DYNARRAY_INITIAL_SIZE 0
+#define DYNARRAY_PREFIX fd_list_
+#include <malloc/dynarray-skeleton.c>
+
+static int
+fchmodat_with_lchmod (int fd, const char *path, mode_t mode, int flags)
+{
+  TEST_COMPARE (fd, AT_FDCWD);
+  if (flags == 0)
+    return chmod (path, mode);
+  else
+    {
+      TEST_COMPARE (flags, AT_SYMLINK_NOFOLLOW);
+      return lchmod (path, mode);
+    }
+}
+
+/* Chose the appropriate path to pass as the path argument to the *at
+   functions.  */
+static const char *
+select_path (bool do_relative_path, const char *full_path, const char *relative_path)
+{
+  if (do_relative_path)
+    return relative_path;
+  else
+    return full_path;
+}
+
+static void
+test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t, int))
+{
+  char *tempdir = support_create_temp_directory ("tst-lchmod-");
+
+  char *path_dangling = xasprintf ("%s/dangling", tempdir);
+  char *path_file = xasprintf ("%s/file", tempdir);
+  char *path_loop = xasprintf ("%s/loop", tempdir);
+  char *path_missing = xasprintf ("%s/missing", tempdir);
+  char *path_to_file = xasprintf ("%s/to-file", tempdir);
+
+  int fd;
+  if (do_relative_path)
+    fd = xopen (tempdir, O_DIRECTORY | O_RDONLY, 0);
+  else
+    fd = AT_FDCWD;
+
+  add_temp_file (path_dangling);
+  add_temp_file (path_loop);
+  add_temp_file (path_file);
+  add_temp_file (path_to_file);
+
+  support_write_file_string (path_file, "");
+  xsymlink ("file", path_to_file);
+  xsymlink ("loop", path_loop);
+  xsymlink ("target-does-not-exist", path_dangling);
+
+  /* Check that the modes do not collide with what we will use in the
+     test.  */
+  struct stat64 st;
+  xstat (path_file, &st);
+  TEST_VERIFY ((st.st_mode & 0777) != 1);
+  xlstat (path_to_file, &st);
+  TEST_VERIFY ((st.st_mode & 0777) != 2);
+  mode_t original_symlink_mode = st.st_mode;
+
+  /* Set to true if AT_SYMLINK_NOFOLLOW is supported.  */
+  bool nofollow;
+
+  /* We should be able to change the mode of a file, including through
+     the symbolic link to-file.  */
+  const char *arg = select_path (do_relative_path, path_file, "file");
+  TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0);
+  xstat (path_file, &st);
+  TEST_COMPARE (st.st_mode & 0777, 1);
+  int ret = chmod_func (fd, path_file, 2, AT_SYMLINK_NOFOLLOW);
+  if (ret == 0)
+    {
+      printf ("info: AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
+      nofollow = true;
+    }
+  else
+    {
+      printf ("info: no AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
+      nofollow = false;
+
+      /* Set up things for the code below.  */
+      TEST_COMPARE (chmod_func (fd, path_file, 2, 0), 0);
+    }
+  xstat (path_file, &st);
+  TEST_COMPARE (st.st_mode & 0777, 2);
+  arg = select_path (do_relative_path, path_to_file, "to-file");
+  TEST_COMPARE (chmod_func (fd, path_to_file, 1, 0), 0);
+  xstat (path_file, &st);
+  TEST_COMPARE (st.st_mode & 0777, 1);
+  xlstat (path_to_file, &st);
+  TEST_COMPARE (original_symlink_mode, st.st_mode);
+
+  /* Changing the mode of a symbolic link may fail.  */
+  arg = select_path (do_relative_path, path_to_file, "to-file");
+  ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
+  if (nofollow)
+    {
+      TEST_COMPARE (ret, 0);
+
+      /* The mode of the link changed.  */
+      xlstat (path_to_file, &st);
+      TEST_COMPARE (st.st_mode & 0777, 2);
+
+      /* But the mode of the file is unchanged.  */
+      xstat (path_file, &st);
+      TEST_COMPARE (st.st_mode & 0777, 1);
+
+    }
+  else
+    {
+      TEST_COMPARE (ret, -1);
+      TEST_COMPARE (errno, EOPNOTSUPP);
+
+      /* The modes should remain unchanged.  */
+      xstat (path_file, &st);
+      TEST_COMPARE (st.st_mode & 0777, 1);
+      xlstat (path_to_file, &st);
+      TEST_COMPARE (original_symlink_mode, st.st_mode);
+    }
+
+  /* If we have NOFOLLOW support, we should be able to change the mode
+     of a dangling symbolic link or a symbolic link loop.  */
+  const char *paths[] = { path_dangling, path_loop };
+  for (size_t i = 0; i < array_length (paths); ++i)
+    {
+      const char *path = paths[i];
+      const char *filename = strrchr (path, '/');
+      TEST_VERIFY_EXIT (filename != NULL);
+      ++filename;
+      mode_t new_mode = 010 + i;
+
+      xlstat (path, &st);
+      TEST_VERIFY ((st.st_mode & 0777) != new_mode);
+      original_symlink_mode = st.st_mode;
+      arg = select_path (do_relative_path, path, filename);
+      ret = chmod_func (fd, arg, new_mode, AT_SYMLINK_NOFOLLOW);
+      if (nofollow)
+        {
+          TEST_COMPARE (ret, 0);
+          xlstat (path, &st);
+          TEST_COMPARE (st.st_mode & 0777, new_mode);
+        }
+      else /* !nofollow.  */
+        {
+          TEST_COMPARE (ret, -1);
+          TEST_COMPARE (errno, EOPNOTSUPP);
+          xlstat (path, &st);
+          TEST_COMPARE (st.st_mode, original_symlink_mode);
+        }
+    }
+
+   /* A missing file should always result in ENOENT.  The presence of
+      /proc does not matter.  */
+   arg = select_path (do_relative_path, path_missing, "missing");
+   TEST_COMPARE (chmod_func (fd, arg, 020, 0), -1);
+   TEST_COMPARE (errno, ENOENT);
+   TEST_COMPARE (chmod_func (fd, arg, 020, AT_SYMLINK_NOFOLLOW), -1);
+   TEST_COMPARE (errno, ENOENT);
+
+   /* Test without available file descriptors.  */
+   {
+     struct fd_list fd_list;
+     fd_list_init (&fd_list);
+     while (true)
+       {
+         int ret = dup (STDOUT_FILENO);
+         if (ret == -1)
+           {
+             if (errno == ENFILE || errno == EMFILE)
+               break;
+             FAIL_EXIT1 ("dup: %m");
+           }
+         fd_list_add (&fd_list, ret);
+         TEST_VERIFY_EXIT (!fd_list_has_failed (&fd_list));
+       }
+     /* Without AT_SYMLINK_NOFOLLOW, changing the permissions should
+        work as before.  */
+     arg = select_path (do_relative_path, path_file, "file");
+     TEST_COMPARE (chmod_func (fd, arg, 3, 0), 0);
+     xstat (path_file, &st);
+     TEST_COMPARE (st.st_mode & 0777, 3);
+     /* But with AT_SYMLINK_NOFOLLOW, even if we originally had
+        support, we may have lost it.  */
+     ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
+     if (ret == 0)
+       {
+         xstat (path_file, &st);
+         TEST_COMPARE (st.st_mode & 0777, 2);
+       }
+     else
+       {
+         TEST_COMPARE (ret, -1);
+         /* The error code from the openat fallback leaks out.  */
+         if (errno != ENFILE && errno != EMFILE)
+           TEST_COMPARE (errno, EOPNOTSUPP);
+       }
+     xstat (path_file, &st);
+     TEST_COMPARE (st.st_mode & 0777, 3);
+
+     /* Close the descriptors.  */
+     for (int *pfd = fd_list_begin (&fd_list); pfd < fd_list_end (&fd_list);
+          ++pfd)
+       xclose (*pfd);
+     fd_list_free (&fd_list);
+   }
+
+   if (do_relative_path)
+    xclose (fd);
+
+   free (path_dangling);
+   free (path_file);
+   free (path_loop);
+   free (path_missing);
+   free (path_to_file);
+
+   free (tempdir);
+}
+
+static void
+test_3 (void)
+{
+  puts ("info: testing lchmod");
+  test_1 (false, fchmodat_with_lchmod);
+  puts ("info: testing fchmodat with AT_FDCWD");
+  test_1 (false, fchmodat);
+  puts ("info: testing fchmodat with relative path");
+  test_1 (true, fchmodat);
+}
+
+static int
+do_test (void)
+{
+  struct support_descriptors *descriptors = support_descriptors_list ();
+
+  /* Run the three tests in the default environment.  */
+  test_3 ();
+
+  /* Try to set up a /proc-less environment and re-test.  */
+#if __has_include (<sys/mount.h>)
+  if (!support_become_root ())
+    puts ("warning: could not obtain root-like privileges");
+  if (!support_enter_mount_namespace ())
+    puts ("warning: could enter a mount namespace");
+  else
+    {
+      /* Attempt to mount an empty directory over /proc.  */
+      char *tempdir = support_create_temp_directory ("tst-lchmod-");
+      bool proc_emptied
+        = mount (tempdir, "/proc", "none", MS_BIND, NULL) == 0;
+      if (!proc_emptied)
+        printf ("warning: bind-mounting /proc failed: %m");
+      free (tempdir);
+
+      puts ("info: re-running tests (after trying to empty /proc)");
+      test_3 ();
+
+      if (proc_emptied)
+        /* Reveal the original /proc, which is needed by the
+           descriptors check below.  */
+        TEST_COMPARE (umount ("/proc"), 0);
+    }
+#endif /* <sys/mount.h>.  */
+
+  support_descriptors_check (descriptors);
+  support_descriptors_free (descriptors);
+
+  return 0;
+}
+
+#include <support/test-driver.c>

Re: [PATCH 4/5] io: Add io/tst-lchmod covering lchmod and fchmodat

[Adhemerval Zanella]

On 11/02/2020 12:27, Florian Weimer wrote:
> * Florian Weimer:
> 
>> +     /* Without AT_SYMLINK_NOFOLLOW, changing the permissions should
>> +        work as before.  */
>> +     arg = select_path (do_relative_path, path_file, "file");
>> +     TEST_COMPARE (chmod_func (fd, arg, 3, 0), 0);
>> +     xstat (path_file, &st);
>> +     TEST_COMPARE (st.st_mode & 0777, 3);
>> +     /* But with AT_SYMLINK_NOFOLLOW, even if we originally had
>> +        support, we may have lost it.  */
>> +     ret = chmod_func (fd, arg, 2, 0);
> 
> The last line misses AT_SYMLINK_NOFOLLOW.  The version below (which I'm
> going to commit) fixes this and adds another check.

I am seeing the following failures on ext2/ext3 on both powerpc64le
(4.18.0-80.7.2.el7.ppc64le) and aarch64 (4.12.13).

info: testing lchmod
info: AT_SYMLINK_NOFOLLOW support in /tmp/tst-lchmod-mBFJ8Q
tst-lchmod.c:142: numeric comparison failure
   left: -1 (0xffffffff); from: ret
  right: 0 (0x0); from: 0
tst-lchmod.c:183: numeric comparison failure
   left: -1 (0xffffffff); from: ret
  right: 0 (0x0); from: 0
tst-lchmod.c:183: numeric comparison failure
   left: -1 (0xffffffff); from: ret
  right: 0 (0x0); from: 0
info: testing fchmodat with AT_FDCWD
info: AT_SYMLINK_NOFOLLOW support in /tmp/tst-lchmod-GGO6WR
tst-lchmod.c:142: numeric comparison failure
   left: -1 (0xffffffff); from: ret
  right: 0 (0x0); from: 0
tst-lchmod.c:183: numeric comparison failure
   left: -1 (0xffffffff); from: ret
  right: 0 (0x0); from: 0
tst-lchmod.c:183: numeric comparison failure
   left: -1 (0xffffffff); from: ret
  right: 0 (0x0); from: 0
info: testing fchmodat with relative path
info: AT_SYMLINK_NOFOLLOW support in /tmp/tst-lchmod-blKDQP
tst-lchmod.c:142: numeric comparison failure
   left: -1 (0xffffffff); from: ret
  right: 0 (0x0); from: 0
tst-lchmod.c:183: numeric comparison failure
   left: -1 (0xffffffff); from: ret
  right: 0 (0x0); from: 0
tst-lchmod.c:183: numeric comparison failure
   left: -1 (0xffffffff); from: ret
  right: 0 (0x0); from: 0
info: re-running tests (after trying to empty /proc)
info: testing lchmod
info: no AT_SYMLINK_NOFOLLOW support in /tmp/tst-lchmod-CJrdjQ
info: testing fchmodat with AT_FDCWD
info: no AT_SYMLINK_NOFOLLOW support in /tmp/tst-lchmod-JKBC6P
info: testing fchmodat with relative path
info: no AT_SYMLINK_NOFOLLOW support in /tmp/tst-lchmod-qaG1BP
error: 9 test failures

Re: [PATCH 4/5] io: Add io/tst-lchmod covering lchmod and fchmodat

[Florian Weimer]

* Adhemerval Zanella:

> On 11/02/2020 12:27, Florian Weimer wrote:
>> * Florian Weimer:
>> 
>>> +     /* Without AT_SYMLINK_NOFOLLOW, changing the permissions should
>>> +        work as before.  */
>>> +     arg = select_path (do_relative_path, path_file, "file");
>>> +     TEST_COMPARE (chmod_func (fd, arg, 3, 0), 0);
>>> +     xstat (path_file, &st);
>>> +     TEST_COMPARE (st.st_mode & 0777, 3);
>>> +     /* But with AT_SYMLINK_NOFOLLOW, even if we originally had
>>> +        support, we may have lost it.  */
>>> +     ret = chmod_func (fd, arg, 2, 0);
>> 
>> The last line misses AT_SYMLINK_NOFOLLOW.  The version below (which I'm
>> going to commit) fixes this and adds another check.
>
> I am seeing the following failures on ext2/ext3 on both powerpc64le
> (4.18.0-80.7.2.el7.ppc64le) and aarch64 (4.12.13).

Yeah, please see this thread:

  <https://sourceware.org/ml/libc-alpha/2020-02/msg00467.html>

It affects more than XFS, but apparently not tmpfs (which is what I
happened to use during development of the emulation).

I'll patch this up tomorrow.  It'll considerably simplify the test
case, too.

Re: [PATCH 4/5] io: Add io/tst-lchmod covering lchmod and fchmodat

[Adhemerval Zanella]

On 12/02/2020 15:53, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 11/02/2020 12:27, Florian Weimer wrote:
>>> * Florian Weimer:
>>>
>>>> +     /* Without AT_SYMLINK_NOFOLLOW, changing the permissions should
>>>> +        work as before.  */
>>>> +     arg = select_path (do_relative_path, path_file, "file");
>>>> +     TEST_COMPARE (chmod_func (fd, arg, 3, 0), 0);
>>>> +     xstat (path_file, &st);
>>>> +     TEST_COMPARE (st.st_mode & 0777, 3);
>>>> +     /* But with AT_SYMLINK_NOFOLLOW, even if we originally had
>>>> +        support, we may have lost it.  */
>>>> +     ret = chmod_func (fd, arg, 2, 0);
>>>
>>> The last line misses AT_SYMLINK_NOFOLLOW.  The version below (which I'm
>>> going to commit) fixes this and adds another check.
>>
>> I am seeing the following failures on ext2/ext3 on both powerpc64le
>> (4.18.0-80.7.2.el7.ppc64le) and aarch64 (4.12.13).
> 
> Yeah, please see this thread:
> 
>   <https://sourceware.org/ml/libc-alpha/2020-02/msg00467.html>
> 
> It affects more than XFS, but apparently not tmpfs (which is what I
> happened to use during development of the emulation).
> 
> I'll patch this up tomorrow.  It'll considerably simplify the test
> case, too.
> 

I had the impression from the thread that is was a XFS-only issue.
Maybe we can add a XFAIL meanwhile?

Re: [PATCH 4/5] io: Add io/tst-lchmod covering lchmod and fchmodat

[Florian Weimer]

* Adhemerval Zanella:

> On 12/02/2020 15:53, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> On 11/02/2020 12:27, Florian Weimer wrote:
>>>> * Florian Weimer:
>>>>
>>>>> +     /* Without AT_SYMLINK_NOFOLLOW, changing the permissions should
>>>>> +        work as before.  */
>>>>> +     arg = select_path (do_relative_path, path_file, "file");
>>>>> +     TEST_COMPARE (chmod_func (fd, arg, 3, 0), 0);
>>>>> +     xstat (path_file, &st);
>>>>> +     TEST_COMPARE (st.st_mode & 0777, 3);
>>>>> +     /* But with AT_SYMLINK_NOFOLLOW, even if we originally had
>>>>> +        support, we may have lost it.  */
>>>>> +     ret = chmod_func (fd, arg, 2, 0);
>>>>
>>>> The last line misses AT_SYMLINK_NOFOLLOW.  The version below (which I'm
>>>> going to commit) fixes this and adds another check.
>>>
>>> I am seeing the following failures on ext2/ext3 on both powerpc64le
>>> (4.18.0-80.7.2.el7.ppc64le) and aarch64 (4.12.13).
>> 
>> Yeah, please see this thread:
>> 
>>   <https://sourceware.org/ml/libc-alpha/2020-02/msg00467.html>
>> 
>> It affects more than XFS, but apparently not tmpfs (which is what I
>> happened to use during development of the emulation).
>> 
>> I'll patch this up tomorrow.  It'll considerably simplify the test
>> case, too.
>> 
>
> I had the impression from the thread that is was a XFS-only issue.
> Maybe we can add a XFAIL meanwhile?

Hmph.  I'm testing a patch, should be ready to post in 20 minutes or so.

Re: [PATCH 4/5] io: Add io/tst-lchmod covering lchmod and fchmodat

[Florian Weimer]

* Florian Weimer:

> * Adhemerval Zanella:
>
>> On 12/02/2020 15:53, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>> 
>>>> On 11/02/2020 12:27, Florian Weimer wrote:
>>>>> * Florian Weimer:
>>>>>
>>>>>> +     /* Without AT_SYMLINK_NOFOLLOW, changing the permissions should
>>>>>> +        work as before.  */
>>>>>> +     arg = select_path (do_relative_path, path_file, "file");
>>>>>> +     TEST_COMPARE (chmod_func (fd, arg, 3, 0), 0);
>>>>>> +     xstat (path_file, &st);
>>>>>> +     TEST_COMPARE (st.st_mode & 0777, 3);
>>>>>> +     /* But with AT_SYMLINK_NOFOLLOW, even if we originally had
>>>>>> +        support, we may have lost it.  */
>>>>>> +     ret = chmod_func (fd, arg, 2, 0);
>>>>>
>>>>> The last line misses AT_SYMLINK_NOFOLLOW.  The version below (which I'm
>>>>> going to commit) fixes this and adds another check.
>>>>
>>>> I am seeing the following failures on ext2/ext3 on both powerpc64le
>>>> (4.18.0-80.7.2.el7.ppc64le) and aarch64 (4.12.13).
>>> 
>>> Yeah, please see this thread:
>>> 
>>>   <https://sourceware.org/ml/libc-alpha/2020-02/msg00467.html>
>>> 
>>> It affects more than XFS, but apparently not tmpfs (which is what I
>>> happened to use during development of the emulation).
>>> 
>>> I'll patch this up tomorrow.  It'll considerably simplify the test
>>> case, too.
>>> 
>>
>> I had the impression from the thread that is was a XFS-only issue.
>> Maybe we can add a XFAIL meanwhile?
>
> Hmph.  I'm testing a patch, should be ready to post in 20 minutes or so.

Patch posted: <https://sourceware.org/ml/libc-alpha/2020-02/msg00534.html>

[PATCH 5/5] Linux: Add op/tst-o_path-locks

[Florian Weimer]

The O_PATH-based fchmodat emulation relies on the fact that closing
an O_PATH descriptor never releases POSIX advisory locks, so this
commit adds a test case for this behavior.
---
 sysdeps/unix/sysv/linux/Makefile           |   2 +-
 sysdeps/unix/sysv/linux/tst-o_path-locks.c | 100 +++++++++++++++++++++
 2 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-o_path-locks.c

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index f12b7b1a2d..487ff274b6 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -273,7 +273,7 @@ sysdep_routines += xstatconv internal_statvfs internal_statvfs64 \
 
 sysdep_headers += bits/fcntl-linux.h
 
-tests += tst-fallocate tst-fallocate64
+tests += tst-fallocate tst-fallocate64 tst-o_path-locks
 endif
 
 ifeq ($(subdir),elf)
diff --git a/sysdeps/unix/sysv/linux/tst-o_path-locks.c b/sysdeps/unix/sysv/linux/tst-o_path-locks.c
new file mode 100644
index 0000000000..07e6556f73
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-o_path-locks.c
@@ -0,0 +1,100 @@
+/* Test that closing O_PATH descriptors does not release POSIX advisory locks.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xunistd.h>
+
+/* The subprocess writes the errno value of the lock operation
+   here.  */
+static int *shared_errno;
+
+/* The path of the temporary file which is locked.  */
+static char *path;
+
+/* Try to obtain an exclusive lock on the file at path.  */
+static void
+subprocess (void *closure)
+{
+  int fd = xopen (path, O_RDWR, 0);
+  struct flock64 lock = { .l_type = F_WRLCK, };
+  int ret = fcntl64 (fd, F_SETLK, &lock);
+  if (ret == 0)
+    *shared_errno = 0;
+  else
+    *shared_errno = errno;
+  xclose (fd);
+}
+
+/* Return true if the file at path is currently locked, false if
+   not.  */
+static bool
+probe_lock (void)
+{
+  *shared_errno = -1;
+  support_isolate_in_subprocess (subprocess, NULL);
+  if (*shared_errno == 0)
+    /* Lock was aquired by the subprocess, so this process has not
+       locked it.  */
+    return false;
+  else
+    {
+      /* POSIX allows both EACCES and EAGAIN.  Linux use EACCES.  */
+      TEST_COMPARE (*shared_errno, EAGAIN);
+      return true;
+    }
+}
+
+static int
+do_test (void)
+{
+  shared_errno = support_shared_allocate (sizeof (*shared_errno));
+  int fd = create_temp_file ("tst-o_path-locks-", &path);
+
+  /* The file is not locked initially.  */
+  TEST_VERIFY (!probe_lock ());
+
+  struct flock64 lock = { .l_type = F_WRLCK, };
+  TEST_COMPARE (fcntl64 (fd, F_SETLK, &lock), 0);
+
+  /* The lock has been acquired.  */
+  TEST_VERIFY (probe_lock ());
+
+  /* Closing the same file via a different descriptor releases the
+     lock.  */
+  xclose (xopen (path, O_RDONLY, 0));
+  TEST_VERIFY (!probe_lock ());
+
+  /* But not if it is an O_PATH descriptor.  */
+  TEST_COMPARE (fcntl64 (fd, F_SETLK, &lock), 0);
+  xclose (xopen (path, O_PATH, 0));
+  TEST_VERIFY (probe_lock ());
+
+  xclose (fd);
+  free (path);
+  support_shared_free (shared_errno);
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.24.1

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Andreas Schwab]

On Jan 22 2020, Florian Weimer wrote:

> These patches use the O_PATH hack to implement a fairly close
> approximation of fchmodat in AT_SYMLINK_NOFOLLOW mode.

This means that cp -a now fails when /proc is not mounted.

<https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc:rebuild/installation-images:openSUSE/f/x86_64>

running "cp -a /var/lib/ca-certificates var/lib"
cp: setting permissions for 'var/lib/ca-certificates/pem': Operation not supported
cp: setting permissions for 'var/lib/ca-certificates/openssl': Operation not supported

newfstatat(AT_FDCWD, "/var/lib/ca-certificates/pem", {st_mode=S_IFDIR|0500, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
mkdir("var/lib/ca-certificates/pem", 0500) = 0
lstat("var/lib/ca-certificates/pem", {st_mode=S_IFDIR|0500, st_size=4096, ...}) = 0
openat(AT_FDCWD, "var/lib/ca-certificates/pem", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3
newfstatat(3, "", {st_mode=S_IFDIR|0500, st_size=4096, ...}, AT_EMPTY_PATH) = 0
chmod("/proc/self/fd/3", 040700)        = -1 ENOENT (No such file or directory)
close(3)                                = 0
write(2, "cp: ", 4)                     = 4
write(2, "setting permissions for 'var/lib"..., 53) = 53
write(2, ": Operation not supported", 25) = 25
write(2, "\n", 1)                       = 1

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Florian Weimer]

* Andreas Schwab:

> On Jan 22 2020, Florian Weimer wrote:
>
>> These patches use the O_PATH hack to implement a fairly close
>> approximation of fchmodat in AT_SYMLINK_NOFOLLOW mode.
>
> This means that cp -a now fails when /proc is not mounted.
>
> <https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc:rebuild/installation-images:openSUSE/f/x86_64>
>
> running "cp -a /var/lib/ca-certificates var/lib"
> cp: setting permissions for 'var/lib/ca-certificates/pem': Operation not supported
> cp: setting permissions for 'var/lib/ca-certificates/openssl': Operation not supported
>
> newfstatat(AT_FDCWD, "/var/lib/ca-certificates/pem", {st_mode=S_IFDIR|0500, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
> mkdir("var/lib/ca-certificates/pem", 0500) = 0
> lstat("var/lib/ca-certificates/pem", {st_mode=S_IFDIR|0500, st_size=4096, ...}) = 0
> openat(AT_FDCWD, "var/lib/ca-certificates/pem", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3
> newfstatat(3, "", {st_mode=S_IFDIR|0500, st_size=4096, ...}, AT_EMPTY_PATH) = 0
> chmod("/proc/self/fd/3", 040700)        = -1 ENOENT (No such file or directory)
> close(3)                                = 0
> write(2, "cp: ", 4)                     = 4
> write(2, "setting permissions for 'var/lib"..., 53) = 53
> write(2, ": Operation not supported", 25) = 25
> write(2, "\n", 1)                       = 1

Yes, it's a known issue.

I have not been able to convince the kernel developers to add the
required system call.  Maybe you will have more luck with that?

Thanks,
Florian

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Dmitry V. Levin]

On Tue, Jul 21, 2020 at 11:54:38AM +0200, Florian Weimer via Libc-alpha wrote:
> * Andreas Schwab:
> > On Jan 22 2020, Florian Weimer wrote:
> >
> >> These patches use the O_PATH hack to implement a fairly close
> >> approximation of fchmodat in AT_SYMLINK_NOFOLLOW mode.
> >
> > This means that cp -a now fails when /proc is not mounted.
> >
> > <https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc:rebuild/installation-images:openSUSE/f/x86_64>
> >
> > running "cp -a /var/lib/ca-certificates var/lib"
> > cp: setting permissions for 'var/lib/ca-certificates/pem': Operation not supported
> > cp: setting permissions for 'var/lib/ca-certificates/openssl': Operation not supported
> >
> > newfstatat(AT_FDCWD, "/var/lib/ca-certificates/pem", {st_mode=S_IFDIR|0500, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
> > mkdir("var/lib/ca-certificates/pem", 0500) = 0
> > lstat("var/lib/ca-certificates/pem", {st_mode=S_IFDIR|0500, st_size=4096, ...}) = 0
> > openat(AT_FDCWD, "var/lib/ca-certificates/pem", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3
> > newfstatat(3, "", {st_mode=S_IFDIR|0500, st_size=4096, ...}, AT_EMPTY_PATH) = 0
> > chmod("/proc/self/fd/3", 040700)        = -1 ENOENT (No such file or directory)
> > close(3)                                = 0
> > write(2, "cp: ", 4)                     = 4
> > write(2, "setting permissions for 'var/lib"..., 53) = 53
> > write(2, ": Operation not supported", 25) = 25
> > write(2, "\n", 1)                       = 1
> 
> Yes, it's a known issue.

Sorry but this is a regression, we cannot assume that /proc is always mounted.


-- 
ldv

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Andreas Schwab]

On Jul 21 2020, Florian Weimer wrote:

> * Andreas Schwab:
>
>> On Jan 22 2020, Florian Weimer wrote:
>>
>>> These patches use the O_PATH hack to implement a fairly close
>>> approximation of fchmodat in AT_SYMLINK_NOFOLLOW mode.
>>
>> This means that cp -a now fails when /proc is not mounted.
>>
>> <https://build.opensuse.org/package/live_build_log/home:Andreas_Schwab:glibc:rebuild/installation-images:openSUSE/f/x86_64>
>>
>> running "cp -a /var/lib/ca-certificates var/lib"
>> cp: setting permissions for 'var/lib/ca-certificates/pem': Operation not supported
>> cp: setting permissions for 'var/lib/ca-certificates/openssl': Operation not supported
>>
>> newfstatat(AT_FDCWD, "/var/lib/ca-certificates/pem", {st_mode=S_IFDIR|0500, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
>> mkdir("var/lib/ca-certificates/pem", 0500) = 0
>> lstat("var/lib/ca-certificates/pem", {st_mode=S_IFDIR|0500, st_size=4096, ...}) = 0
>> openat(AT_FDCWD, "var/lib/ca-certificates/pem", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3
>> newfstatat(3, "", {st_mode=S_IFDIR|0500, st_size=4096, ...}, AT_EMPTY_PATH) = 0
>> chmod("/proc/self/fd/3", 040700)        = -1 ENOENT (No such file or directory)
>> close(3)                                = 0
>> write(2, "cp: ", 4)                     = 4
>> write(2, "setting permissions for 'var/lib"..., 53) = 53
>> write(2, ": Operation not supported", 25) = 25
>> write(2, "\n", 1)                       = 1
>
> Yes, it's a known issue.
>
> I have not been able to convince the kernel developers to add the
> required system call.  Maybe you will have more luck with that?

What's the point of having lchmod if it always fails anyway?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Florian Weimer]

* Andreas Schwab:

>> I have not been able to convince the kernel developers to add the
>> required system call.  Maybe you will have more luck with that?
>
> What's the point of having lchmod if it always fails anyway?

It used to be stub that always failed, but it does not anymore.

gnulib has an emulation with a security vulnerability, and they do not
want to remove it.  I'm worried that unless we provide a full (non-stub)
implementation, applications using gnulib end up with security
vulnerabilities.

Thanks,
Florian

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Andreas Schwab]

On Aug 25 2020, Florian Weimer wrote:

> * Andreas Schwab:
>
>>> I have not been able to convince the kernel developers to add the
>>> required system call.  Maybe you will have more luck with that?
>>
>> What's the point of having lchmod if it always fails anyway?
>
> It used to be stub that always failed, but it does not anymore.

Yes, it does.  The only point of lchmod is to call it on a symlink, and
that always fails.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Florian Weimer]

* Andreas Schwab:

> On Aug 25 2020, Florian Weimer wrote:
>
>> * Andreas Schwab:
>>
>>>> I have not been able to convince the kernel developers to add the
>>>> required system call.  Maybe you will have more luck with that?
>>>
>>> What's the point of having lchmod if it always fails anyway?
>>
>> It used to be stub that always failed, but it does not anymore.
>
> Yes, it does.  The only point of lchmod is to call it on a symlink, and
> that always fails.

I'm not sure your assumption is correct.  lchown-type operations are
often called on pathnames that do not refer to symbolic links.  For
example, calling “chown -R 1000.1000” does this under the covers:

  fchownat(4, "1", 1000, 1000, AT_SYMLINK_NOFOLLOW) = 0

And:

-rw-rw-r--. 1 fweimer fweimer 0 Aug 25 13:02 1

So the case for lchmod would be to get the failure for the symbolic
link, instead of changing the permissions of the target.

Thanks,
Florian

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Andreas Schwab]

On Aug 25 2020, Florian Weimer wrote:

> * Andreas Schwab:
>
>> On Aug 25 2020, Florian Weimer wrote:
>>
>>> * Andreas Schwab:
>>>
>>>>> I have not been able to convince the kernel developers to add the
>>>>> required system call.  Maybe you will have more luck with that?
>>>>
>>>> What's the point of having lchmod if it always fails anyway?
>>>
>>> It used to be stub that always failed, but it does not anymore.
>>
>> Yes, it does.  The only point of lchmod is to call it on a symlink, and
>> that always fails.
>
> I'm not sure your assumption is correct.  lchown-type

We are talking about lchmod, not lchown.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Florian Weimer]

* Andreas Schwab:

> On Aug 25 2020, Florian Weimer wrote:
>
>> * Andreas Schwab:
>>
>>> On Aug 25 2020, Florian Weimer wrote:
>>>
>>>> * Andreas Schwab:
>>>>
>>>>>> I have not been able to convince the kernel developers to add the
>>>>>> required system call.  Maybe you will have more luck with that?
>>>>>
>>>>> What's the point of having lchmod if it always fails anyway?
>>>>
>>>> It used to be stub that always failed, but it does not anymore.
>>>
>>> Yes, it does.  The only point of lchmod is to call it on a symlink, and
>>> that always fails.
>>
>> I'm not sure your assumption is correct.  lchown-type
>
> We are talking about lchmod, not lchown.

The rationale for using lchmod in coreutils is the same, I think.

Thanks,
Florian

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Andreas Schwab]

On Aug 25 2020, Florian Weimer wrote:

> * Andreas Schwab:
>
>> On Aug 25 2020, Florian Weimer wrote:
>>
>>> * Andreas Schwab:
>>>
>>>> On Aug 25 2020, Florian Weimer wrote:
>>>>
>>>>> * Andreas Schwab:
>>>>>
>>>>>>> I have not been able to convince the kernel developers to add the
>>>>>>> required system call.  Maybe you will have more luck with that?
>>>>>>
>>>>>> What's the point of having lchmod if it always fails anyway?
>>>>>
>>>>> It used to be stub that always failed, but it does not anymore.
>>>>
>>>> Yes, it does.  The only point of lchmod is to call it on a symlink, and
>>>> that always fails.
>>>
>>> I'm not sure your assumption is correct.  lchown-type
>>
>> We are talking about lchmod, not lchown.
>
> The rationale for using lchmod in coreutils is the same, I think.

You cannot compare lchmod with lchown.  The latter has a useful, working
implementation, whereas the former *always* fails, so implementing it
doesn't make any sense.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Florian Weimer]

* Andreas Schwab:

> You cannot compare lchmod with lchown.  The latter has a useful, working
> implementation, whereas the former *always* fails, so implementing it
> doesn't make any sense.

Why would lchmod of a regular file fail?  lchown doesn't.

Thanks,
Florian

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Andreas Schwab]

On Aug 25 2020, Florian Weimer wrote:

> Why would lchmod of a regular file fail?

Nobody calls lchmod on a regular file.  The one and only point of lchmod
is to call it on a symlink.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Florian Weimer]

* Andreas Schwab:

> On Aug 25 2020, Florian Weimer wrote:
>
>> Why would lchmod of a regular file fail?
>
> Nobody calls lchmod on a regular file.  The one and only point of lchmod
> is to call it on a symlink.

Maybe not on regular files, but coreutils uses it for directories
(src/copy.c):

          /* We need search and write permissions to the new directory
             for writing the directory's contents. Check if these
             permissions are there.  */

          if (lstat (dst_name, &dst_sb) != 0)
            {
              error (0, errno, _("cannot stat %s"), quoteaf (dst_name));
              goto un_backup;
            }
          else if ((dst_sb.st_mode & S_IRWXU) != S_IRWXU)
            {
              /* Make the new directory searchable and writable.  */

              dst_mode = dst_sb.st_mode;
              restore_dst_mode = true;

              if (lchmod (dst_name, dst_mode | S_IRWXU) != 0)
                {
                  error (0, errno, _("setting permissions for %s"),
                         quoteaf (dst_name));
                  goto un_backup;
                }
            }

And FIFOs (src/mkfifo.c):

      if (mkfifo (argv[optind], newmode) != 0)
        {
          error (0, errno, _("cannot create fifo %s"), quoteaf (argv[optind]));
          exit_status = EXIT_FAILURE;
        }
      else if (specified_mode && lchmod (argv[optind], newmode) != 0)
        {
          error (0, errno, _("cannot set permissions of %s"),
                 quoteaf (argv[optind]));
          exit_status = EXIT_FAILURE;
        }
    }

The point is to avoid a race condition where a symbolic link is
introduced after the pathname has been created.

Thanks,
Florian

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Andreas Schwab]

On Aug 25 2020, Florian Weimer wrote:

> The point is to avoid a race condition where a symbolic link is
> introduced after the pathname has been created.

But it causes regressions when /proc is not mounted.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 0/5] Linux: lchmod and AT_SYMLINK_NOFOLLOW support for fchmodat

[Florian Weimer]

* Andreas Schwab:

> On Aug 25 2020, Florian Weimer wrote:
>
>> The point is to avoid a race condition where a symbolic link is
>> introduced after the pathname has been created.
>
> But it causes regressions when /proc is not mounted.

It does.  But what we can we do?  We should not implement lchmod or
fchmodat with this race condition.

We need kernel support for this.  Palmer Dabbelt wrote a patch, but it
hasn't been merged, and I don't know if he's still interested in it.

Thanks,
Florian