[PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Paul Eggert]

Problem reported by Florian Weimer in:
https://www.sourceware.org/ml/libc-alpha/2020-02/msg00534.html
* lib/fchmodat.c (fchmodat):
* lib/lchmod.c (lchmod):
Don’t assume that chmod on the O_PATH-opened fd will do
the right thing on a symbolic link.
* lib/fchmodat.c (fchmodat):
Don’t attempt to special-case
any flag value other than AT_SYMLINK_NOFOLLOW.
---
 ChangeLog      | 13 +++++++++++++
 lib/fchmodat.c | 33 ++++++++++++++++++++++++++-------
 lib/lchmod.c   | 27 ++++++++++++++++++++++-----
 3 files changed, 61 insertions(+), 12 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index b5ef9b3a1..8bcf1bf0a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2020-02-13  Paul Eggert  <eggert@cs.ucla.edu>
+
+	fchmodat, lchmod: port to buggy Linux filesystems
+	Problem reported by Florian Weimer in:
+	https://www.sourceware.org/ml/libc-alpha/2020-02/msg00534.html
+	* lib/fchmodat.c (fchmodat):
+	* lib/lchmod.c (lchmod):
+	Don’t assume that chmod on the O_PATH-opened fd will do
+	the right thing on a symbolic link.
+	* lib/fchmodat.c (fchmodat):
+	Don’t attempt to special-case
+	any flag value other than AT_SYMLINK_NOFOLLOW.
+
 2020-02-11  Paul Eggert  <eggert@cs.ucla.edu>
 
 	lchmod: pacify Coverity CID 1491216
diff --git a/lib/fchmodat.c b/lib/fchmodat.c
index 87aa0d191..02e2da956 100644
--- a/lib/fchmodat.c
+++ b/lib/fchmodat.c
@@ -63,12 +63,31 @@ orig_fchmodat (int dir, char const *file, mode_t mode, int flags)
 int
 fchmodat (int dir, char const *file, mode_t mode, int flags)
 {
-  if (flags & AT_SYMLINK_NOFOLLOW)
+  if (flags == AT_SYMLINK_NOFOLLOW)
     {
-# ifdef O_PATH
+      struct stat st;
+
+# if defined O_PATH && defined AT_EMPTY_PATH
       int fd = openat (dir, file, O_PATH | O_NOFOLLOW | O_CLOEXEC);
       if (fd < 0)
         return fd;
+
+      /* Use fstatat because fstat does not work on O_PATH descriptors
+         before Linux 3.6.  */
+      if (fstatat (fd, "", &st, AT_EMPTY_PATH) != 0)
+        {
+          int stat_errno = errno;
+          close (fd);
+          errno = stat_errno;
+          return -1;
+        }
+      if (S_ISLNK (st.st_mode))
+        {
+          close (fd);
+          errno = EOPNOTSUPP;
+          return -1;
+        }
+
       static char const fmt[] = "/proc/self/fd/%d";
       char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];
       sprintf (buf, fmt, fd);
@@ -82,10 +101,8 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
           errno = chmod_errno;
           return chmod_result;
         }
-      /* /proc is not mounted; fall back on racy implementation.  */
-# endif
-
-      struct stat st;
+      /* /proc is not mounted.  */
+# else
       int fstatat_result = fstatat (dir, file, &st, AT_SYMLINK_NOFOLLOW);
       if (fstatat_result != 0)
         return fstatat_result;
@@ -94,7 +111,9 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
           errno = EOPNOTSUPP;
           return -1;
         }
-      flags &= ~AT_SYMLINK_NOFOLLOW;
+# endif
+      /* Fall back on chmod, despite the race.  */
+      flags = 0;
     }
 
   return orig_fchmodat (dir, file, mode, flags);
diff --git a/lib/lchmod.c b/lib/lchmod.c
index 5fc658023..c7191c07d 100644
--- a/lib/lchmod.c
+++ b/lib/lchmod.c
@@ -37,10 +37,28 @@ lchmod (char const *file, mode_t mode)
 #if HAVE_FCHMODAT
   return fchmodat (AT_FDCWD, file, mode, AT_SYMLINK_NOFOLLOW);
 #else
-# if defined O_PATH && defined AT_FDCWD
+# if defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH
   int fd = openat (AT_FDCWD, file, O_PATH | O_NOFOLLOW | O_CLOEXEC);
   if (fd < 0)
     return fd;
+
+  /* Use fstatat because fstat does not work on O_PATH descriptors
+     before Linux 3.6.  */
+  struct stat st;
+  if (fstatat (fd, "", &st, AT_EMPTY_PATH) != 0)
+    {
+      int stat_errno = errno;
+      close (fd);
+      errno = stat_errno;
+      return -1;
+    }
+  if (S_ISLNK (st.st_mode))
+    {
+      close (fd);
+      errno = EOPNOTSUPP;
+      return -1;
+    }
+
   static char const fmt[] = "/proc/self/fd/%d";
   char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];
   sprintf (buf, fmt, fd);
@@ -54,10 +72,8 @@ lchmod (char const *file, mode_t mode)
       errno = chmod_errno;
       return chmod_result;
     }
-  /* /proc is not mounted; fall back on racy implementation.  */
-# endif
-
-# if HAVE_LSTAT
+  /* /proc is not mounted.  */
+# elif HAVE_LSTAT
   struct stat st;
   int lstat_result = lstat (file, &st);
   if (lstat_result != 0)
@@ -69,6 +85,7 @@ lchmod (char const *file, mode_t mode)
     }
 # endif
 
+  /* Fall back on chmod, despite the race.  */
   return chmod (file, mode);
 #endif
 }
-- 
2.24.1

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Bruno Haible]

The original thread's subject "XFS reports lchmod failure, but changes
file system contents" gave the impression that it's somehow XFS specific.

But what I observe is that the behaviour is the same on all major file
systems on Linux.

Tested on Linux 5.3.7 (fc31), with the trivial program below, on
  - ext4
  - btrfs
  - jfs
  - reiserfs
  - xfs
Before your latest patch: mode changed, but error "Operation not supported".
After  your latest patch: mode not changed, error "Operation not supported".

Bruno


============================ lchmod.c ============================
#include <config.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
int main (int argc, char *argv[])
{
	int mode = strtol (argv[1], NULL, 8);
	char *path = argv[2];
	int ret = lchmod (path, mode);
	if (ret < 0)
		perror ("lchmod");
}

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Bruno Haible]

Hi Paul,

> +	fchmodat, lchmod: port to buggy Linux filesystems
> +	Problem reported by Florian Weimer in:
> +	https://www.sourceware.org/ml/libc-alpha/2020-02/msg00534.html
> +	* lib/fchmodat.c (fchmodat):
> +	* lib/lchmod.c (lchmod):
> +	Don’t assume that chmod on the O_PATH-opened fd will do
> +	the right thing on a symbolic link.
> +	* lib/fchmodat.c (fchmodat):
> +	Don’t attempt to special-case
> +	any flag value other than AT_SYMLINK_NOFOLLOW.

What about OSes other than Linux?


1) The layout of the /proc file system is specific to Linux. It looks
different on BSD and other systems. Therefore the code block that makes
assumptions about /proc ought to be guarded with 'defined __linux__', no?
Here is a proposed patch:

diff --git a/lib/fchmodat.c b/lib/fchmodat.c
index 02e2da9..180b95d 100644
--- a/lib/fchmodat.c
+++ b/lib/fchmodat.c
@@ -67,7 +67,7 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
     {
       struct stat st;
 
-# if defined O_PATH && defined AT_EMPTY_PATH
+# if defined O_PATH && defined AT_EMPTY_PATH && defined __linux__
       int fd = openat (dir, file, O_PATH | O_NOFOLLOW | O_CLOEXEC);
       if (fd < 0)
         return fd;
diff --git a/lib/lchmod.c b/lib/lchmod.c
index c7191c0..6802774 100644
--- a/lib/lchmod.c
+++ b/lib/lchmod.c
@@ -37,7 +37,7 @@ lchmod (char const *file, mode_t mode)
 #if HAVE_FCHMODAT
   return fchmodat (AT_FDCWD, file, mode, AT_SYMLINK_NOFOLLOW);
 #else
-# if defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH
+# if defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH && defined __linux__
   int fd = openat (AT_FDCWD, file, O_PATH | O_NOFOLLOW | O_CLOEXEC);
   if (fd < 0)
     return fd;


2) Also the discussion what is the "right" behaviour is specific to Linux.
The code in the '#else' case

  if (S_ISLNK (st.st_mode))
    {
      close (fd);
      errno = EOPNOTSUPP;
      return -1;
    }

will surely upset users on BSD systems, where symlinks are intended to have
permission bits.


Bruno

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Paul Eggert]

On 2/13/20 7:46 PM, Bruno Haible wrote:

> Here is a proposed patch:

Thanks, looks good.

> 2) Also the discussion what is the "right" behaviour is specific to Linux.
> The code in the '#else' case
> 
>    if (S_ISLNK (st.st_mode))
>      {
>        close (fd);
>        errno = EOPNOTSUPP;
>        return -1;
>      }
> 
> will surely upset users on BSD systems, where symlinks are intended to have
> permission bits.

Because of the Autoconf tests, that code should be executed only on 
platforms where lchmod fails (or does not exist), whould shouldn't occur 
on BSD systems. Still, I take your point that the code is confusing. 
Perhaps lchmod.m4 and fchmodat.m4 should define a symbol 
HAVE_LCHMOD_BUG_WITH_NON_SYMLINKS and the C code could refer to that. 
The resulting machine code would be the same as now, but the 
cause-and-effect would be clearer.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Bruno Haible]

I wrote:
> But what I observe is that the behaviour is the same on all major file
> systems on Linux.
> 
> Tested on Linux 5.3.7 (fc31), with the trivial program below, on
>   - ext4
>   - btrfs
>   - jfs
>   - reiserfs
>   - xfs

This are different when using the code which goes through the /proc
file system:

===============================================================================
#define _GNU_SOURCE 1
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
int main (int argc, char *argv[])
{
	int mode = strtol (argv[1], NULL, 8);
	char* path = argv[2];
	int ret;
	int fd = openat (AT_FDCWD, path, O_PATH | O_NOFOLLOW | O_CLOEXEC);
	if (fd < 0)
		ret = -1;
	else {
		char buf[100];
		sprintf (buf, "/proc/self/fd/%d", fd);
		ret = chmod (buf, mode);
	}
	if (ret < 0)
		perror ("lchmod");
}
===============================================================================

In this case:
- On ext4, btrfs, jfs, xfs:
    mode changed, but error "Operation not supported".
- On reiserfs:
    mode changed, success.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Paul Eggert]

On 2/16/20 9:24 AM, Bruno Haible wrote:
> In this case:
> - On ext4, btrfs, jfs, xfs:
>      mode changed, but error "Operation not supported".
> - On reiserfs:
>      mode changed, success.

Thanks for checking. I hope all this stuff is worked around by the latest patch.

One other possible bug is that the mode could change temporarily and then 
spontaneously revert later, because the updated mode was cached but not 
committed to stable storage. Filesystem bugs can be nasty.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Bruno Haible]

Hi Paul,

> One other possible bug is that the mode could change temporarily and then 
> spontaneously revert later, because the updated mode was cached but not 
> committed to stable storage. Filesystem bugs can be nasty.

The S_ISLNK check that you put in bypasses this dark/fuzzy area in the kernel.
Therefore I won't check against this possible bug in a unit test. Let the
kernel people do this.

Bruno

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Bruno Haible]

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

Paul Eggert wrote:
> > 2) Also the discussion what is the "right" behaviour is specific to Linux.
> > The code in the '#else' case
> > 
> >    if (S_ISLNK (st.st_mode))
> >      {
> >        close (fd);
> >        errno = EOPNOTSUPP;
> >        return -1;
> >      }
> > 
> > will surely upset users on BSD systems, where symlinks are intended to have
> > permission bits.
> 
> Because of the Autoconf tests, that code should be executed only on 
> platforms where lchmod fails (or does not exist), whould shouldn't occur 
> on BSD systems.

Unfortunately, it doesn't: The line in the module description

lib_SOURCES += lchmod.c

causes lchmod.c to be compiled (to a .o file that defines 'lchmod', not
even 'rpl_lchmod'!) on macOS, FreeBSD, NetBSD, etc.

> Still, I take your point that the code is confusing. 
> Perhaps lchmod.m4 and fchmodat.m4 should define a symbol 
> HAVE_LCHMOD_BUG_WITH_NON_SYMLINKS and the C code could refer to that. 
> The resulting machine code would be the same as now, but the 
> cause-and-effect would be clearer.

Yes, I agree. Let me do this. Also I'm adding more comments. This patch
is tested on all platforms that have an lchown() function.


Before:

                   HAVE_LCHMOD  REPLACE_LCHMOD   nm lchmod.o      nm fchmodat.o
glibc/Linux            0             0          rpl_fchmodat     fchmodat,openat,chmod,...
macOS                  1             0          rpl_lstat,chmod  save_cwd,chmod,lchmod,...
HP-UX                  1             0          rpl_lstat,chmod  save_cwd,chmod,lchmod,...
musl/Linux             1             0          fchmodat         --
glibc/Hurd             1             0          fchmodat         --
glibc/kFreeBSD         1             0          fchmodat         --
FreeBSD                1             0          fchmodat         --
NetBSD                 1             0          fchmodat         --

After:

                   HAVE_LCHMOD  REPLACE_LCHMOD   nm lchmod.o      nm fchmodat.o
glibc/Linux            0             0          rpl_fchmodat     fchmodat,openat,chmod,...
macOS                  1             0          --               save_cwd,chmod,lchmod,...
HP-UX                  1             0          --               save_cwd,chmod,lchmod,...
musl/Linux             1             0          --               --
glibc/Hurd             1             0          --               --
glibc/kFreeBSD         1             0          --               --
FreeBSD                1             0          --               --
NetBSD                 1             0          --               --


2020-02-16  Bruno Haible  <bruno@clisp.org>

	lchmod: Make more future-proof.
	* m4/lchmod.m4 (gl_FUNC_LCHMOD): Define NEED_LCHMOD_NONSYMLINK_FIX.
	(gl_PREREQ_LCHMOD): New macro.
	* lib/lchmod.c (orig_lchmod): New function.
	(lchmod): Test NEED_LCHMOD_NONSYMLINK_FIX. Access /proc only on Linux.
	Return EOPNOTSUPP only on Linux and on platforms without lchmod
	function.
	* modules/lchmod (configure.ac): Invoke gl_PREREQ_LCHMOD.

	lchmod: Fix buggy override on macOS, HP-UX (regression from 2020-02-08).
	* modules/lchmod (Makefile.am): Don't add lchmod.c to lib_SOURCES.


[-- Attachment #2: 0001-lchmod-Fix-buggy-override-on-macOS-HP-UX-regression-.patch --]
[-- Type: text/x-patch, Size: 1183 bytes --]

From 4efc18c4a2927074bf0653ba8bd79230661b8238 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sun, 16 Feb 2020 22:31:34 +0100
Subject: [PATCH 1/2] lchmod: Fix buggy override on macOS, HP-UX (regression
 from 2020-02-08).

* modules/lchmod (Makefile.am): Don't add lchmod.c to lib_SOURCES.
---
 ChangeLog      | 5 +++++
 modules/lchmod | 1 -
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index beacc09..f7a3959 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2020-02-16  Bruno Haible  <bruno@clisp.org>
 
+	lchmod: Fix buggy override on macOS, HP-UX (regression from 2020-02-08).
+	* modules/lchmod (Makefile.am): Don't add lchmod.c to lib_SOURCES.
+
+2020-02-16  Bruno Haible  <bruno@clisp.org>
+
 	lchmod: Improve cross-compilation guess.
 	* m4/lchmod.m4 (gl_FUNC_LCHMOD): Require AC_CANONICAL_HOST. When
 	cross-compiling, guess depending on the platform.
diff --git a/modules/lchmod b/modules/lchmod
index e1054f6..c829910 100644
--- a/modules/lchmod
+++ b/modules/lchmod
@@ -23,7 +23,6 @@ fi
 gl_SYS_STAT_MODULE_INDICATOR([lchmod])
 
 Makefile.am:
-lib_SOURCES += lchmod.c
 
 Include:
 <sys/stat.h>
-- 
2.7.4


[-- Attachment #3: 0002-lchmod-Make-more-future-proof.patch --]
[-- Type: text/x-patch, Size: 6102 bytes --]

From ea91b32b0719d46354c1d995c1d1b1c36842a2eb Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sun, 16 Feb 2020 22:37:28 +0100
Subject: [PATCH 2/2] lchmod: Make more future-proof.

* m4/lchmod.m4 (gl_FUNC_LCHMOD): Define NEED_LCHMOD_NONSYMLINK_FIX.
(gl_PREREQ_LCHMOD): New macro.
* lib/lchmod.c (orig_lchmod): New function.
(lchmod): Test NEED_LCHMOD_NONSYMLINK_FIX. Access /proc only on Linux.
Return EOPNOTSUPP only on Linux and on platforms without lchmod
function.
* modules/lchmod (configure.ac): Invoke gl_PREREQ_LCHMOD.
---
 ChangeLog      |  9 +++++++++
 lib/lchmod.c   | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 m4/lchmod.m4   | 15 +++++++++++++--
 modules/lchmod |  1 +
 4 files changed, 77 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index f7a3959..0705107 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,14 @@
 2020-02-16  Bruno Haible  <bruno@clisp.org>
 
+	lchmod: Make more future-proof.
+	* m4/lchmod.m4 (gl_FUNC_LCHMOD): Define NEED_LCHMOD_NONSYMLINK_FIX.
+	(gl_PREREQ_LCHMOD): New macro.
+	* lib/lchmod.c (orig_lchmod): New function.
+	(lchmod): Test NEED_LCHMOD_NONSYMLINK_FIX. Access /proc only on Linux.
+	Return EOPNOTSUPP only on Linux and on platforms without lchmod
+	function.
+	* modules/lchmod (configure.ac): Invoke gl_PREREQ_LCHMOD.
+
 	lchmod: Fix buggy override on macOS, HP-UX (regression from 2020-02-08).
 	* modules/lchmod (Makefile.am): Don't add lchmod.c to lib_SOURCES.
 
diff --git a/lib/lchmod.c b/lib/lchmod.c
index c7191c0..57f75da 100644
--- a/lib/lchmod.c
+++ b/lib/lchmod.c
@@ -19,30 +19,68 @@
 
 #include <config.h>
 
+/* If the user's config.h happens to include <sys/stat.h>, let it include only
+   the system's <sys/stat.h> here, so that orig_fchmodat doesn't recurse to
+   rpl_fchmodat.  */
+#define __need_system_sys_stat_h
+#include <config.h>
+
+/* Specification.  */
 #include <sys/stat.h>
+#undef __need_system_sys_stat_h
+
+#if HAVE_LCHMOD
+static inline int
+orig_lchmod (char const *file, mode_t mode)
+{
+  return lchmod (file, mode);
+}
+#endif
 
 #include <errno.h>
 #include <fcntl.h>
 #include <stdio.h>
 #include <unistd.h>
 
+#ifdef __osf__
+/* Write "sys/stat.h" here, not <sys/stat.h>, otherwise OSF/1 5.1 DTK cc
+   eliminates this include because of the preliminary #include <sys/stat.h>
+   above.  */
+# include "sys/stat.h"
+#else
+# include <sys/stat.h>
+#endif
+
 #include <intprops.h>
 
-/* Work like lchmod, except when FILE is a symbolic link.
-   In that case, set errno to EOPNOTSUPP and return -1.  */
+/* Work like chmod, except when FILE is a symbolic link.
+   In that case, on systems where permissions on symbolic links are unsupported
+   (such as Linux), set errno to EOPNOTSUPP and return -1.  */
 
 int
 lchmod (char const *file, mode_t mode)
 {
 #if HAVE_FCHMODAT
+  /* Gnulib's fchmodat contains the workaround.  No need to duplicate it
+     here.  */
   return fchmodat (AT_FDCWD, file, mode, AT_SYMLINK_NOFOLLOW);
-#else
-# if defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH
+#elif NEED_LCHMOD_NONSYMLINK_FIX
+# if defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH \
+     && (defined __linux__ || defined __ANDROID__)
+  /* Open a file descriptor with O_NOFOLLOW, to make sure we don't
+     follow symbolic links, if /proc is mounted.  O_PATH is used to
+     avoid a failure if the file is not readable.
+     Cf. <https://sourceware.org/bugzilla/show_bug.cgi?id=14578>  */
   int fd = openat (AT_FDCWD, file, O_PATH | O_NOFOLLOW | O_CLOEXEC);
   if (fd < 0)
     return fd;
 
-  /* Use fstatat because fstat does not work on O_PATH descriptors
+  /* Up to Linux 5.3 at least, when FILE refers to a symbolic link, the
+     chmod call below will change the permissions of the symbolic link
+     - which is undersired - and on many file systems (ext4, btrfs, jfs,
+     xfs, ..., but not reiserfs) fail with error EOPNOTSUPP - which is
+     misleading.  Therefore test for a symbolic link explicitly.
+     Use fstatat because fstat does not work on O_PATH descriptors
      before Linux 3.6.  */
   struct stat st;
   if (fstatat (fd, "", &st, AT_EMPTY_PATH) != 0)
@@ -73,7 +111,10 @@ lchmod (char const *file, mode_t mode)
       return chmod_result;
     }
   /* /proc is not mounted.  */
+  /* Fall back on chmod, despite the race.  */
+  return chmod (file, mode);
 # elif HAVE_LSTAT
+#  if (defined __linux__ || defined __ANDROID__) || !HAVE_LCHMOD
   struct stat st;
   int lstat_result = lstat (file, &st);
   if (lstat_result != 0)
@@ -83,9 +124,15 @@ lchmod (char const *file, mode_t mode)
       errno = EOPNOTSUPP;
       return -1;
     }
-# endif
-
   /* Fall back on chmod, despite the race.  */
   return chmod (file, mode);
+#  else              /* GNU/kFreeBSD, GNU/Hurd, macOS, FreeBSD, NetBSD, HP-UX */
+  return orig_lchmod (file, mode);
+#  endif
+# else                                                      /* native Windows */
+  return chmod (file, mode);
+# endif
+#else
+  return orig_lchmod (file, mode);
 #endif
 }
diff --git a/m4/lchmod.m4 b/m4/lchmod.m4
index 1646bd7..61e3f11 100644
--- a/m4/lchmod.m4
+++ b/m4/lchmod.m4
@@ -1,4 +1,4 @@
-#serial 5
+#serial 6
 
 dnl Copyright (C) 2005-2006, 2008-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
@@ -67,7 +67,18 @@ AC_DEFUN([gl_FUNC_LCHMOD],
        rm -f conftest.lchmod])
     case $gl_cv_func_lchmod_works in
       *yes) ;;
-      *) REPLACE_LCHMOD=1;;
+      *)
+        AC_DEFINE([NEED_LCHMOD_NONSYMLINK_FIX], [1],
+          [Define to 1 if lchmod does not work right on non-symlinks.])
+        REPLACE_LCHMOD=1
+        ;;
     esac
   fi
 ])
+
+# Prerequisites of lib/lchmod.c.
+AC_DEFUN([gl_PREREQ_LCHMOD],
+[
+  AC_REQUIRE([AC_C_INLINE])
+  :
+])
diff --git a/modules/lchmod b/modules/lchmod
index c829910..c3e0a16 100644
--- a/modules/lchmod
+++ b/modules/lchmod
@@ -19,6 +19,7 @@ configure.ac:
 gl_FUNC_LCHMOD
 if test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1; then
   AC_LIBOBJ([lchmod])
+  gl_PREREQ_LCHMOD
 fi
 gl_SYS_STAT_MODULE_INDICATOR([lchmod])
 
-- 
2.7.4

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Bruno Haible]

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

> > Perhaps lchmod.m4 and fchmodat.m4 should define a symbol 
> > HAVE_LCHMOD_BUG_WITH_NON_SYMLINKS and the C code could refer to that. 

> 2020-02-16  Bruno Haible  <bruno@clisp.org>
> 
> 	lchmod: Make more future-proof.
> 	* m4/lchmod.m4 (gl_FUNC_LCHMOD): Define NEED_LCHMOD_NONSYMLINK_FIX.
> 	(gl_PREREQ_LCHMOD): New macro.
> 	* lib/lchmod.c (orig_lchmod): New function.
> 	(lchmod): Test NEED_LCHMOD_NONSYMLINK_FIX. Access /proc only on Linux.
> 	Return EOPNOTSUPP only on Linux and on platforms without lchmod
> 	function.
> 	* modules/lchmod (configure.ac): Invoke gl_PREREQ_LCHMOD.

And here's a similar change for 'fchmodat'. It has one or two code branches
that currently are dead code, but this is on purpose: this is code that
will becomes active when either the NEED_FCHMODAT_NONSYMLINK_FIX test
will trigger on more platforms, or some more autoconf tests will be added
to fchmodat.m4.


2020-02-16  Bruno Haible  <bruno@clisp.org>

	fchmodat: Make more future-proof.
	* m4/fchmodat.m4 (gl_FUNC_FCHMODAT): Define
	NEED_FCHMODAT_NONSYMLINK_FIX.
	(gl_PREREQ_FCHMODAT): New macro.
	* lib/fchmodat.c (fchmodat): Test NEED_FCHMODAT_NONSYMLINK_FIX. Access
	/proc only on Linux. Return EOPNOTSUPP only on Linux and on platforms
	without lchmod function.
	* modules/fchmodat (configure.ac): Invoke gl_PREREQ_FCHMODAT.


[-- Attachment #2: 0001-fchmodat-Make-more-future-proof.patch --]
[-- Type: text/x-patch, Size: 5635 bytes --]

From f4693b0166bab83ab232dcd3cfd95906411d1110 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sun, 16 Feb 2020 23:01:08 +0100
Subject: [PATCH] fchmodat: Make more future-proof.

* m4/fchmodat.m4 (gl_FUNC_FCHMODAT): Define
NEED_FCHMODAT_NONSYMLINK_FIX.
(gl_PREREQ_FCHMODAT): New macro.
* lib/fchmodat.c (fchmodat): Test NEED_FCHMODAT_NONSYMLINK_FIX. Access
/proc only on Linux. Return EOPNOTSUPP only on Linux and on platforms
without lchmod function.
* modules/fchmodat (configure.ac): Invoke gl_PREREQ_FCHMODAT.
---
 ChangeLog        | 11 +++++++++++
 lib/fchmodat.c   | 30 +++++++++++++++++++++++-------
 m4/fchmodat.m4   | 15 +++++++++++++--
 modules/fchmodat |  1 +
 4 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 30cc425..8215147 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
 2020-02-16  Bruno Haible  <bruno@clisp.org>
 
+	fchmodat: Make more future-proof.
+	* m4/fchmodat.m4 (gl_FUNC_FCHMODAT): Define
+	NEED_FCHMODAT_NONSYMLINK_FIX.
+	(gl_PREREQ_FCHMODAT): New macro.
+	* lib/fchmodat.c (fchmodat): Test NEED_FCHMODAT_NONSYMLINK_FIX. Access
+	/proc only on Linux. Return EOPNOTSUPP only on Linux and on platforms
+	without lchmod function.
+	* modules/fchmodat (configure.ac): Invoke gl_PREREQ_FCHMODAT.
+
+2020-02-16  Bruno Haible  <bruno@clisp.org>
+
 	lchmod: Make more future-proof.
 	* m4/lchmod.m4 (gl_FUNC_LCHMOD): Define NEED_LCHMOD_NONSYMLINK_FIX.
 	(gl_PREREQ_LCHMOD): New macro.
diff --git a/lib/fchmodat.c b/lib/fchmodat.c
index 02e2da9..bb48b44 100644
--- a/lib/fchmodat.c
+++ b/lib/fchmodat.c
@@ -14,7 +14,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
 
-/* written by Jim Meyering */
+/* written by Jim Meyering and Paul Eggert */
 
 /* If the user's config.h happens to include <sys/stat.h>, let it include only
    the system's <sys/stat.h> here, so that orig_fchmodat doesn't recurse to
@@ -63,16 +63,27 @@ orig_fchmodat (int dir, char const *file, mode_t mode, int flags)
 int
 fchmodat (int dir, char const *file, mode_t mode, int flags)
 {
+# if NEED_FCHMODAT_NONSYMLINK_FIX
   if (flags == AT_SYMLINK_NOFOLLOW)
     {
       struct stat st;
 
-# if defined O_PATH && defined AT_EMPTY_PATH
+#  if defined O_PATH && defined AT_EMPTY_PATH \
+      && (defined __linux__ || defined __ANDROID__)
+      /* Open a file descriptor with O_NOFOLLOW, to make sure we don't
+         follow symbolic links, if /proc is mounted.  O_PATH is used to
+         avoid a failure if the file is not readable.
+         Cf. <https://sourceware.org/bugzilla/show_bug.cgi?id=14578>  */
       int fd = openat (dir, file, O_PATH | O_NOFOLLOW | O_CLOEXEC);
       if (fd < 0)
         return fd;
 
-      /* Use fstatat because fstat does not work on O_PATH descriptors
+      /* Up to Linux 5.3 at least, when FILE refers to a symbolic link, the
+         chmod call below will change the permissions of the symbolic link
+         - which is undersired - and on many file systems (ext4, btrfs, jfs,
+         xfs, ..., but not reiserfs) fail with error EOPNOTSUPP - which is
+         misleading.  Therefore test for a symbolic link explicitly.
+         Use fstatat because fstat does not work on O_PATH descriptors
          before Linux 3.6.  */
       if (fstatat (fd, "", &st, AT_EMPTY_PATH) != 0)
         {
@@ -102,7 +113,9 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
           return chmod_result;
         }
       /* /proc is not mounted.  */
-# else
+      /* Fall back on orig_fchmodat, despite the race.  */
+      return orig_fchmodat (dir, file, mode, 0);
+#  elif (defined __linux__ || defined __ANDROID__) || !HAVE_LCHMOD
       int fstatat_result = fstatat (dir, file, &st, AT_SYMLINK_NOFOLLOW);
       if (fstatat_result != 0)
         return fstatat_result;
@@ -111,10 +124,13 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
           errno = EOPNOTSUPP;
           return -1;
         }
-# endif
-      /* Fall back on chmod, despite the race.  */
-      flags = 0;
+      /* Fall back on orig_fchmodat, despite the race.  */
+      return orig_fchmodat (dir, file, mode, 0);
+#  else
+      return orig_fchmodat (dir, file, mode, 0);
+#  endif
     }
+# endif
 
   return orig_fchmodat (dir, file, mode, flags);
 }
diff --git a/m4/fchmodat.m4 b/m4/fchmodat.m4
index f284485..e3f2f04 100644
--- a/m4/fchmodat.m4
+++ b/m4/fchmodat.m4
@@ -1,4 +1,4 @@
-# fchmodat.m4 serial 3
+# fchmodat.m4 serial 4
 dnl Copyright (C) 2004-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -65,7 +65,18 @@ AC_DEFUN([gl_FUNC_FCHMODAT],
        rm -f conftest.fchmodat])
     case $gl_cv_func_fchmodat_works in
       *yes) ;;
-      *) REPLACE_FCHMODAT=1;;
+      *)
+        AC_DEFINE([NEED_FCHMODAT_NONSYMLINK_FIX], [1],
+          [Define to 1 if fchmodat+AT_SYMLINK_NOFOLLOW does not work right on non-symlinks.])
+        REPLACE_FCHMODAT=1
+        ;;
     esac
   fi
 ])
+
+# Prerequisites of lib/fchmodat.c.
+AC_DEFUN([gl_PREREQ_FCHMODAT],
+[
+  AC_CHECK_FUNCS_ONCE([lchmod])
+  :
+])
diff --git a/modules/fchmodat b/modules/fchmodat
index b0f06e8..f0dac9d 100644
--- a/modules/fchmodat
+++ b/modules/fchmodat
@@ -28,6 +28,7 @@ configure.ac:
 gl_FUNC_FCHMODAT
 if test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1; then
   AC_LIBOBJ([fchmodat])
+  gl_PREREQ_FCHMODAT
 fi
 gl_MODULE_INDICATOR([fchmodat]) dnl for lib/openat.h
 gl_SYS_STAT_MODULE_INDICATOR([fchmodat])
-- 
2.7.4

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Bruno Haible]

> 2020-02-16  Bruno Haible  <bruno@clisp.org>
> 
> 	lchmod: Make more future-proof.
> 	* m4/lchmod.m4 (gl_FUNC_LCHMOD): Define NEED_LCHMOD_NONSYMLINK_FIX.
> 	(gl_PREREQ_LCHMOD): New macro.
> 	* lib/lchmod.c (orig_lchmod): New function.
> 	(lchmod): Test NEED_LCHMOD_NONSYMLINK_FIX. Access /proc only on Linux.
> 	Return EOPNOTSUPP only on Linux and on platforms without lchmod
> 	function.
> 	* modules/lchmod (configure.ac): Invoke gl_PREREQ_LCHMOD.

Oops, this change is broken: It causes a link error on Solaris 10:

cc -xarch=generic64 -O -D_STDC_C99=  -g  -L/home/haible/prefix-x86_64/lib -o test-areadlinkat test-areadlinkat.o libtests.a ../gllib/libgnu.a libtests.a  
Undefined                       first referenced
 symbol                             in file
orig_lchmod                         ../gllib/libgnu.a(lchmod.o)
ld: fatal: symbol referencing errors. No output written to test-areadlinkat
*** Error code 1

This patch fixes it.


2020-02-22  Bruno Haible  <bruno@clisp.org>

	lchmod: Fix link error on Solaris 10 (regression from 2020-02-16).
	* lib/lchmod.c (lchmod): Use the code with lstat and chmod also when
	NEED_LCHMOD_NONSYMLINK_FIX is not defined.

diff --git a/lib/lchmod.c b/lib/lchmod.c
index 57f75da..e62111c 100644
--- a/lib/lchmod.c
+++ b/lib/lchmod.c
@@ -64,9 +64,9 @@ lchmod (char const *file, mode_t mode)
   /* Gnulib's fchmodat contains the workaround.  No need to duplicate it
      here.  */
   return fchmodat (AT_FDCWD, file, mode, AT_SYMLINK_NOFOLLOW);
-#elif NEED_LCHMOD_NONSYMLINK_FIX
-# if defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH \
-     && (defined __linux__ || defined __ANDROID__)
+#elif NEED_LCHMOD_NONSYMLINK_FIX \
+      && defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH \
+      && (defined __linux__ || defined __ANDROID__)            /* newer Linux */
   /* Open a file descriptor with O_NOFOLLOW, to make sure we don't
      follow symbolic links, if /proc is mounted.  O_PATH is used to
      avoid a failure if the file is not readable.
@@ -113,8 +113,9 @@ lchmod (char const *file, mode_t mode)
   /* /proc is not mounted.  */
   /* Fall back on chmod, despite the race.  */
   return chmod (file, mode);
-# elif HAVE_LSTAT
-#  if (defined __linux__ || defined __ANDROID__) || !HAVE_LCHMOD
+#elif HAVE_LSTAT
+# if (NEED_LCHMOD_NONSYMLINK_FIX && (defined __linux__ || defined __ANDROID__)) \
+     || !HAVE_LCHMOD                               /* older Linux, Solaris 10 */
   struct stat st;
   int lstat_result = lstat (file, &st);
   if (lstat_result != 0)
@@ -126,13 +127,10 @@ lchmod (char const *file, mode_t mode)
     }
   /* Fall back on chmod, despite the race.  */
   return chmod (file, mode);
-#  else              /* GNU/kFreeBSD, GNU/Hurd, macOS, FreeBSD, NetBSD, HP-UX */
+# else               /* GNU/kFreeBSD, GNU/Hurd, macOS, FreeBSD, NetBSD, HP-UX */
   return orig_lchmod (file, mode);
-#  endif
-# else                                                      /* native Windows */
-  return chmod (file, mode);
 # endif
-#else
-  return orig_lchmod (file, mode);
+#else                                                       /* native Windows */
+  return chmod (file, mode);
 #endif
 }

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Bruno Haible]

Hi Paul,

> +  if (S_ISLNK (st.st_mode))
> +    {
> +      close (fd);
> +      errno = EOPNOTSUPP;
> +      return -1;
> +    }

Why EOPNOTSUPP? Why not ENOTSUP?

On Solaris, gnulib's lchmod on a symbolic link produces
  lchmod: Operation not supported on transport endpoint
"transport endpoint" is misleading.

Likewise, in POSIX [1] EOPNOTSUPP means "Operation not supported on socket".

I would find ENOTSUP a better choice.

Bruno

[1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Paul Eggert]

On 2/22/20 5:35 PM, Bruno Haible wrote:
> Why EOPNOTSUPP? Why not ENOTSUP?

glibc will use EOPNOTSUPP, because POSIX says fchmodat with AT_SYMLINK_NOFOLLOW 
  uses EOPNOTSUPP in this situation.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Paul Eggert]

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

On 2/22/20 3:46 PM, Bruno Haible wrote:
> Oops, this change is broken: It causes a link error on Solaris 10:

I ran into that problem independently, and found that I had trouble reading the 
code and there were some other obvious glitches in it. It seems that my original 
patch was too complicated and the porting gingerbread since then has made things 
worse. Instead of making it more complicated still I snipped away everything I 
couldn't figure out the need for, and installed the attached.

Perhaps I went too far and some of the complications need to be brought back, 
but I hope not....

[-- Attachment #2: 0001-fchmodat-lchmod-simplify.patch --]
[-- Type: text/x-patch, Size: 16066 bytes --]

From ad0024b3fefea56629941e6533705fa3a4820064 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 22 Feb 2020 22:47:06 -0800
Subject: [PATCH] fchmodat, lchmod: simplify
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It appears that we may have overengineered lchmod and fchmodat,
in that the code was prepared for some hypothetical platforms but
was so complicated that it was hard to understand.  I attempted to
improve the situation by simplifying the code when this
simplification should not hurt on real platforms; we can re-add
complexity later to port to platforms I didn’t know about.
* lib/fchmodat.c (fchmodat):
* lib/lchmod.c (lchmod):
Put the ‘defined __linux__ || defined __ANDROID__’ #ifdef only
around the /proc code that needs it.
* lib/fchmodat.c (fchmodat): Coalese calls to orig_fchmodat.
* lib/lchmod.c (__need_system_sys_stat_h): Omit; no longer needed.
Do not include <config.h> twice.
(orig_lchmod) [HAVE_LCHMOD]: Remove, since we need not wrap
lchmod on any known hosts.
(lchmod): Do not defer to fchmodat, so that the lchmod module
need not depend on the fchmodat module (which is a circular
dependency).  Do not use openat, since ‘open’ suffices.
Coalesce calls to lchmod/chmod.
* lib/lchmod.c, lib/sys_stat.in.h (lchmod):
* m4/sys_stat_h.m4 (REPLACE_FSTAT):
* modules/lchmod (Depends-on, configure.ac):
* modules/sys_stat (Depends-on):
Do not worry about replacing lchmod, since that shouldn’t happen.
* m4/lchmod.m4 (gl_FUNC_LCHMOD): Do not check for fchmodat.
Do not worry about whether lchmod works on non-symlinks,
since every known lchmod works on non-symlinks.
* modules/lchmod (Depends-on):
Remove circular dependency on fchmodat.
---
 ChangeLog         | 33 +++++++++++++++++++++++++++
 lib/fchmodat.c    | 21 +++++++++--------
 lib/lchmod.c      | 46 +++++++++-----------------------------
 lib/sys_stat.in.h | 16 ++-----------
 m4/lchmod.m4      | 57 ++---------------------------------------------
 m4/sys_stat_h.m4  |  1 -
 modules/lchmod    | 13 +++++------
 modules/sys_stat  |  1 -
 8 files changed, 63 insertions(+), 125 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 71a6ce343..c21e04d00 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,36 @@
+2020-02-22  Paul Eggert  <eggert@cs.ucla.edu>
+
+	fchmodat, lchmod: simplify
+	It appears that we may have overengineered lchmod and fchmodat,
+	in that the code was prepared for some hypothetical platforms but
+	was so complicated that it was hard to understand.  I attempted to
+	improve the situation by simplifying the code when this
+	simplification should not hurt on real platforms; we can re-add
+	complexity later to port to platforms I didn’t know about.
+	* lib/fchmodat.c (fchmodat):
+	* lib/lchmod.c (lchmod):
+	Put the ‘defined __linux__ || defined __ANDROID__’ #ifdef only
+	around the /proc code that needs it.
+	* lib/fchmodat.c (fchmodat): Coalese calls to orig_fchmodat.
+	* lib/lchmod.c (__need_system_sys_stat_h): Omit; no longer needed.
+	Do not include <config.h> twice.
+	(orig_lchmod) [HAVE_LCHMOD]: Remove, since we need not wrap
+	lchmod on any known hosts.
+	(lchmod): Do not defer to fchmodat, so that the lchmod module
+	need not depend on the fchmodat module (which is a circular
+	dependency).  Do not use openat, since ‘open’ suffices.
+	Coalesce calls to lchmod/chmod.
+	* lib/lchmod.c, lib/sys_stat.in.h (lchmod):
+	* m4/sys_stat_h.m4 (REPLACE_FSTAT):
+	* modules/lchmod (Depends-on, configure.ac):
+	* modules/sys_stat (Depends-on):
+	Do not worry about replacing lchmod, since that shouldn’t happen.
+	* m4/lchmod.m4 (gl_FUNC_LCHMOD): Do not check for fchmodat.
+	Do not worry about whether lchmod works on non-symlinks,
+	since every known lchmod works on non-symlinks.
+	* modules/lchmod (Depends-on):
+	Remove circular dependency on fchmodat.
+
 2020-02-22  Bruno Haible  <bruno@clisp.org>
 
 	lchmod: Fix link error on Solaris 10 (regression from 2020-02-16).
diff --git a/lib/fchmodat.c b/lib/fchmodat.c
index bb48b44f5..895016860 100644
--- a/lib/fchmodat.c
+++ b/lib/fchmodat.c
@@ -68,8 +68,7 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
     {
       struct stat st;
 
-#  if defined O_PATH && defined AT_EMPTY_PATH \
-      && (defined __linux__ || defined __ANDROID__)
+#  if defined O_PATH && defined AT_EMPTY_PATH
       /* Open a file descriptor with O_NOFOLLOW, to make sure we don't
          follow symbolic links, if /proc is mounted.  O_PATH is used to
          avoid a failure if the file is not readable.
@@ -80,7 +79,7 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
 
       /* Up to Linux 5.3 at least, when FILE refers to a symbolic link, the
          chmod call below will change the permissions of the symbolic link
-         - which is undersired - and on many file systems (ext4, btrfs, jfs,
+         - which is undesired - and on many file systems (ext4, btrfs, jfs,
          xfs, ..., but not reiserfs) fail with error EOPNOTSUPP - which is
          misleading.  Therefore test for a symbolic link explicitly.
          Use fstatat because fstat does not work on O_PATH descriptors
@@ -99,6 +98,7 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
           return -1;
         }
 
+#   if defined __linux__ || defined __ANDROID__
       static char const fmt[] = "/proc/self/fd/%d";
       char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];
       sprintf (buf, fmt, fd);
@@ -112,10 +112,10 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
           errno = chmod_errno;
           return chmod_result;
         }
-      /* /proc is not mounted.  */
-      /* Fall back on orig_fchmodat, despite the race.  */
-      return orig_fchmodat (dir, file, mode, 0);
-#  elif (defined __linux__ || defined __ANDROID__) || !HAVE_LCHMOD
+#   endif
+      /* /proc is not mounted or would not work as in GNU/Linux.  */
+
+#  else
       int fstatat_result = fstatat (dir, file, &st, AT_SYMLINK_NOFOLLOW);
       if (fstatat_result != 0)
         return fstatat_result;
@@ -124,11 +124,10 @@ fchmodat (int dir, char const *file, mode_t mode, int flags)
           errno = EOPNOTSUPP;
           return -1;
         }
-      /* Fall back on orig_fchmodat, despite the race.  */
-      return orig_fchmodat (dir, file, mode, 0);
-#  else
-      return orig_fchmodat (dir, file, mode, 0);
 #  endif
+
+      /* Fall back on orig_fchmodat with no flags, despite a possible race.  */
+      flags = 0;
     }
 # endif
 
diff --git a/lib/lchmod.c b/lib/lchmod.c
index e62111cb8..e11321162 100644
--- a/lib/lchmod.c
+++ b/lib/lchmod.c
@@ -19,23 +19,8 @@
 
 #include <config.h>
 
-/* If the user's config.h happens to include <sys/stat.h>, let it include only
-   the system's <sys/stat.h> here, so that orig_fchmodat doesn't recurse to
-   rpl_fchmodat.  */
-#define __need_system_sys_stat_h
-#include <config.h>
-
 /* Specification.  */
 #include <sys/stat.h>
-#undef __need_system_sys_stat_h
-
-#if HAVE_LCHMOD
-static inline int
-orig_lchmod (char const *file, mode_t mode)
-{
-  return lchmod (file, mode);
-}
-#endif
 
 #include <errno.h>
 #include <fcntl.h>
@@ -60,24 +45,18 @@ orig_lchmod (char const *file, mode_t mode)
 int
 lchmod (char const *file, mode_t mode)
 {
-#if HAVE_FCHMODAT
-  /* Gnulib's fchmodat contains the workaround.  No need to duplicate it
-     here.  */
-  return fchmodat (AT_FDCWD, file, mode, AT_SYMLINK_NOFOLLOW);
-#elif NEED_LCHMOD_NONSYMLINK_FIX \
-      && defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH \
-      && (defined __linux__ || defined __ANDROID__)            /* newer Linux */
+#if defined O_PATH && defined AT_EMPTY_PATH
   /* Open a file descriptor with O_NOFOLLOW, to make sure we don't
      follow symbolic links, if /proc is mounted.  O_PATH is used to
      avoid a failure if the file is not readable.
      Cf. <https://sourceware.org/bugzilla/show_bug.cgi?id=14578>  */
-  int fd = openat (AT_FDCWD, file, O_PATH | O_NOFOLLOW | O_CLOEXEC);
+  int fd = open (file, O_PATH | O_NOFOLLOW | O_CLOEXEC);
   if (fd < 0)
     return fd;
 
   /* Up to Linux 5.3 at least, when FILE refers to a symbolic link, the
      chmod call below will change the permissions of the symbolic link
-     - which is undersired - and on many file systems (ext4, btrfs, jfs,
+     - which is undesired - and on many file systems (ext4, btrfs, jfs,
      xfs, ..., but not reiserfs) fail with error EOPNOTSUPP - which is
      misleading.  Therefore test for a symbolic link explicitly.
      Use fstatat because fstat does not work on O_PATH descriptors
@@ -97,6 +76,7 @@ lchmod (char const *file, mode_t mode)
       return -1;
     }
 
+# if defined __linux__ || defined __ANDROID__
   static char const fmt[] = "/proc/self/fd/%d";
   char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];
   sprintf (buf, fmt, fd);
@@ -110,12 +90,10 @@ lchmod (char const *file, mode_t mode)
       errno = chmod_errno;
       return chmod_result;
     }
-  /* /proc is not mounted.  */
-  /* Fall back on chmod, despite the race.  */
-  return chmod (file, mode);
+# endif
+  /* /proc is not mounted or would not work as in GNU/Linux.  */
+
 #elif HAVE_LSTAT
-# if (NEED_LCHMOD_NONSYMLINK_FIX && (defined __linux__ || defined __ANDROID__)) \
-     || !HAVE_LCHMOD                               /* older Linux, Solaris 10 */
   struct stat st;
   int lstat_result = lstat (file, &st);
   if (lstat_result != 0)
@@ -125,12 +103,8 @@ lchmod (char const *file, mode_t mode)
       errno = EOPNOTSUPP;
       return -1;
     }
-  /* Fall back on chmod, despite the race.  */
-  return chmod (file, mode);
-# else               /* GNU/kFreeBSD, GNU/Hurd, macOS, FreeBSD, NetBSD, HP-UX */
-  return orig_lchmod (file, mode);
-# endif
-#else                                                       /* native Windows */
-  return chmod (file, mode);
 #endif
+
+  /* Fall back on chmod, despite a possible race.  */
+  return chmod (file, mode);
 }
diff --git a/lib/sys_stat.in.h b/lib/sys_stat.in.h
index a27a79a7f..dc8881e6f 100644
--- a/lib/sys_stat.in.h
+++ b/lib/sys_stat.in.h
@@ -518,23 +518,11 @@ _GL_WARN_ON_USE (futimens, "futimens is not portable - "
 #if @GNULIB_LCHMOD@
 /* Change the mode of FILENAME to MODE, without dereferencing it if FILENAME
    denotes a symbolic link.  */
-# if @REPLACE_LCHMOD@
-#  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
-#   undef lchmod
-#   define lchmod rpl_lchmod
-#  endif
-_GL_FUNCDECL_RPL (lchmod, int,
-                  (char const *filename, mode_t mode)
-                  _GL_ARG_NONNULL ((1)));
-_GL_CXXALIAS_RPL (lchmod, int,
-                  (char const *filename, mode_t mode));
-# else
-#  if !@HAVE_LCHMOD@ || defined __hpux
+# if !@HAVE_LCHMOD@ || defined __hpux
 _GL_FUNCDECL_SYS (lchmod, int, (const char *filename, mode_t mode)
                                _GL_ARG_NONNULL ((1)));
-#  endif
-_GL_CXXALIAS_SYS (lchmod, int, (const char *filename, mode_t mode));
 # endif
+_GL_CXXALIAS_SYS (lchmod, int, (const char *filename, mode_t mode));
 _GL_CXXALIASWARN (lchmod);
 #elif defined GNULIB_POSIXCHECK
 # undef lchmod
diff --git a/m4/lchmod.m4 b/m4/lchmod.m4
index 61e3f1122..b9e8a97cb 100644
--- a/m4/lchmod.m4
+++ b/m4/lchmod.m4
@@ -1,4 +1,4 @@
-#serial 6
+#serial 7
 
 dnl Copyright (C) 2005-2006, 2008-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
@@ -17,62 +17,9 @@ AC_DEFUN([gl_FUNC_LCHMOD],
 
   AC_REQUIRE([AC_CANONICAL_HOST]) dnl for cross-compiles
 
-  AC_CHECK_FUNCS_ONCE([fchmodat lchmod lstat])
+  AC_CHECK_FUNCS_ONCE([lchmod lstat])
   if test "$ac_cv_func_lchmod" = no; then
     HAVE_LCHMOD=0
-  else
-    AC_CACHE_CHECK([whether lchmod works on non-symlinks],
-      [gl_cv_func_lchmod_works],
-      [AC_RUN_IFELSE(
-         [AC_LANG_PROGRAM(
-            [
-              AC_INCLUDES_DEFAULT[
-              #ifndef S_IRUSR
-               #define S_IRUSR 0400
-              #endif
-              #ifndef S_IWUSR
-               #define S_IWUSR 0200
-              #endif
-              #ifndef S_IRWXU
-               #define S_IRWXU 0700
-              #endif
-              #ifndef S_IRWXG
-               #define S_IRWXG 0070
-              #endif
-              #ifndef S_IRWXO
-               #define S_IRWXO 0007
-              #endif
-            ]],
-            [[
-              int permissive = S_IRWXU | S_IRWXG | S_IRWXO;
-              int desired = S_IRUSR | S_IWUSR;
-              static char const f[] = "conftest.lchmod";
-              struct stat st;
-              if (creat (f, permissive) < 0)
-                return 1;
-              if (lchmod (f, desired) != 0)
-                return 1;
-              if (stat (f, &st) != 0)
-                return 1;
-              return ! ((st.st_mode & permissive) == desired);
-            ]])],
-         [gl_cv_func_lchmod_works=yes],
-         [gl_cv_func_lchmod_works=no],
-         [case "$host_os" in
-            dnl Guess no on Linux with glibc, yes otherwise.
-            linux-gnu*) gl_cv_func_lchmod_works="guessing no" ;;
-            *)          gl_cv_func_lchmod_works="$gl_cross_guess_normal" ;;
-          esac
-         ])
-       rm -f conftest.lchmod])
-    case $gl_cv_func_lchmod_works in
-      *yes) ;;
-      *)
-        AC_DEFINE([NEED_LCHMOD_NONSYMLINK_FIX], [1],
-          [Define to 1 if lchmod does not work right on non-symlinks.])
-        REPLACE_LCHMOD=1
-        ;;
-    esac
   fi
 ])
 
diff --git a/m4/sys_stat_h.m4 b/m4/sys_stat_h.m4
index 30d60d920..3efba5a7b 100644
--- a/m4/sys_stat_h.m4
+++ b/m4/sys_stat_h.m4
@@ -94,7 +94,6 @@ AC_DEFUN([gl_SYS_STAT_H_DEFAULTS],
   REPLACE_FSTAT=0;      AC_SUBST([REPLACE_FSTAT])
   REPLACE_FSTATAT=0;    AC_SUBST([REPLACE_FSTATAT])
   REPLACE_FUTIMENS=0;   AC_SUBST([REPLACE_FUTIMENS])
-  REPLACE_LCHMOD=0;     AC_SUBST([REPLACE_LCHMOD])
   REPLACE_LSTAT=0;      AC_SUBST([REPLACE_LSTAT])
   REPLACE_MKDIR=0;      AC_SUBST([REPLACE_MKDIR])
   REPLACE_MKFIFO=0;     AC_SUBST([REPLACE_MKFIFO])
diff --git a/modules/lchmod b/modules/lchmod
index c3e0a166d..ae67c03b6 100644
--- a/modules/lchmod
+++ b/modules/lchmod
@@ -6,18 +6,17 @@ lib/lchmod.c
 m4/lchmod.m4
 
 Depends-on:
-errno         [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
+errno         [test $HAVE_LCHMOD = 0]
 extensions
-fcntl-h       [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
-fchmodat      [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
-intprops      [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
-lstat         [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
+fcntl-h       [test $HAVE_LCHMOD = 0]
+intprops      [test $HAVE_LCHMOD = 0]
+lstat         [test $HAVE_LCHMOD = 0]
 sys_stat
-unistd        [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
+unistd        [test $HAVE_LCHMOD = 0]
 
 configure.ac:
 gl_FUNC_LCHMOD
-if test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1; then
+if test $HAVE_LCHMOD = 0; then
   AC_LIBOBJ([lchmod])
   gl_PREREQ_LCHMOD
 fi
diff --git a/modules/sys_stat b/modules/sys_stat
index 93e0cf07e..4783c7efb 100644
--- a/modules/sys_stat
+++ b/modules/sys_stat
@@ -63,7 +63,6 @@ sys/stat.h: sys_stat.in.h $(top_builddir)/config.status $(CXXDEFS_H) $(ARG_NONNU
 	      -e 's|@''REPLACE_FSTAT''@|$(REPLACE_FSTAT)|g' \
 	      -e 's|@''REPLACE_FSTATAT''@|$(REPLACE_FSTATAT)|g' \
 	      -e 's|@''REPLACE_FUTIMENS''@|$(REPLACE_FUTIMENS)|g' \
-	      -e 's|@''REPLACE_LCHMOD''@|$(REPLACE_LCHMOD)|g' \
 	      -e 's|@''REPLACE_LSTAT''@|$(REPLACE_LSTAT)|g' \
 	      -e 's|@''REPLACE_MKDIR''@|$(REPLACE_MKDIR)|g' \
 	      -e 's|@''REPLACE_MKFIFO''@|$(REPLACE_MKFIFO)|g' \
-- 
2.17.1

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Bruno Haible]

Hi Paul,

> found that I had trouble reading the code

That's probably because I prefer the "one code for each platform" approach when
suitable - because when debugging problems I don't like to rely to
  $CC ... -E file.c > file.i
to tell me which code is actually enabled -, whereas you seem to prefer a
"global minimum of lines" approach. It's understandable that I find your code
hard to understand and vice versa.

I'm not saying that one is better than the other; just trying to explain why
we find each other's code odd here.

> Instead of making it more complicated still I snipped away everything I 
> couldn't figure out the need for, and installed the attached.
> 
> Perhaps I went too far and some of the complications need to be brought back, 
> but I hope not....

The fchmodat part looks right.

On the other hand, the lchmod part looks wrong: You stripped away the
  test $REPLACE_LCHMOD = 1
case. However, the file doc/glibc-functions/lchmod.texi still says

  Portability problems fixed by Gnulib:
  @itemize
  ...
  @item
  This function always fails with @code{errno} set to @code{ENOSYS},
  even when the file is not a symbolic link:
  GNU/Linux with glibc 2.31.

Overriding the system's lchmod function requires the case REPLACE_LCHMOD=1.

Bruno

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Paul Eggert]

On 2/23/20 2:58 AM, Bruno Haible wrote:
> the file doc/glibc-functions/lchmod.texi still says
> 
>    ...
>    This function always fails with @code{errno} set to @code{ENOSYS},
>    even when the file is not a symbolic link:
>    GNU/Linux with glibc 2.31.
> 
> Overriding the system's lchmod function requires the case REPLACE_LCHMOD=1.

Hmm, why? 'configure' says that GNU/Linux functions that always fail with 
errno==ENOSYS do not exist, i.e., 'configure' sets HAVE_LCHMOD=0 on GNU/Linux. 
This convention is used often by Autoconf and Gnulib. And if HAVE_LCHMOD=0 why 
would we want REPLACE_LCHMOD=1?

Re: overriding glibc stub functions

[Bruno Haible]

Paul Eggert wrote:
> On 2/23/20 2:58 AM, Bruno Haible wrote:
> > the file doc/glibc-functions/lchmod.texi still says
> > 
> >    ...
> >    This function always fails with @code{errno} set to @code{ENOSYS},
> >    even when the file is not a symbolic link:
> >    GNU/Linux with glibc 2.31.
> > 
> > Overriding the system's lchmod function requires the case REPLACE_LCHMOD=1.
> 
> Hmm, why? 'configure' says that GNU/Linux functions that always fail with 
> errno==ENOSYS do not exist, i.e., 'configure' sets HAVE_LCHMOD=0 on GNU/Linux. 
> This convention is used often by Autoconf and Gnulib. And if HAVE_LCHMOD=0 why 
> would we want REPLACE_LCHMOD=1?

Ouch. I had completely forgotten about this.

Yes, lchmod is defined as a stub in glibc/Linux:

$ nm --dynamic /lib/x86_64-linux-gnu/libc.so.6 | grep lchmod
00000000000f6f40 T lchmod
$ grep lchmod /usr/include/x86_64-linux-gnu/gnu/stubs-64.h
#define __stub_lchmod

And, as you say, this leads to HAVE_LCHMOD=0.

But why do we to distinguish 'lchmod' and 'rpl_lchmod' when we are overriding
lchmod? For three reasons:

  1) So that there is no conflict when linking statically.

  2) So that the behaviour is reliable, and does not break when '-lc' occurs
     in the link command line (or as dependency of other shared libraries on
     the link command line).

  3) So that when debugging with gdb, you know where you are.

Test case for 1):

$ gcc -Wall -O2 -c main.c
$ gcc -Wall -O2 -c lib.c
$ gcc -static main.c lib.c -o main1
$ gcc -static main.c lib.c -lc -o main2
$ gcc -static main.c -lc lib.c -o main3
/tmp/ccPmrK5B.o: In function `main':
main.c:(.text+0x1a): warning: lchmod is not implemented and will always fail
/tmp/ccJXO5KR.o: In function `lchmod':
lib.c:(.text+0x0): multiple definition of `lchmod'
/usr/lib/gcc/x86_64-linux-gnu/5/../../../x86_64-linux-gnu/libc.a(lchmod.o):(.text+0x0): first defined here
collect2: error: ld returned 1 exit status

Test case for 2):

$ gcc -Wall -O2 -c main.c
$ gcc -Wall -O2 -fPIC -c lib.c
$ gcc -shared -o libfoo.so lib.o
$ gcc main.c -o main1
$ gcc main.c -L. -lfoo -o main2
$ gcc main.c -L. -lc -lfoo -o main3
$ touch foo
$ ./main1  =>  lchmod -> -1
$ LD_LIBRARY_PATH=. ./main2  =>  lchmod -> 0
$ LD_LIBRARY_PATH=. ./main3  =>  lchmod -> -1

(main.c and lib.c are attached below.)

3) is relatively unimportant, but
* 1) is important if we don't want to receive bug reports from people who
     do embedded systems (OpenWRT and such), and
* 2) is important for projects that use gnulib in shared libraries
     (from gettext to guile) and don't use the [expensive] renaming of
     symbols in the shared library. (For example, in libunistring,
     lchmod would be renamed to libunistring_lchmod, thus eliminating
     the conflict. But not all libraries do this.)

Therefore I think we need to do two things:

  * For those functions that may be stubs in glibc, define HAVE_FUNC to 1
    instead of 0 if glibc implements it as a stub.
  * Reinstall (at least partially) the code for REPLACE_LCHMOD=1.

Bruno

=================================== main.c ===================================
#include <stdio.h>
#include <sys/stat.h>

int
main (int argc, char *argv[])
{
  int ret = lchmod ("foo", 0444);
  printf ("lchmod -> %d\n", ret);
}

=================================== lib.c ===================================
#include <sys/stat.h>

/* This function is intended to override the libc function.  */
int
lchmod (const char *file, mode_t mode)
{
  return chmod (file, mode);
}

Re: overriding glibc stub functions

[Paul Eggert]

On 2/23/20 6:27 PM, Bruno Haible wrote:
> Therefore I think we need to do two things:
> 
>    * For those functions that may be stubs in glibc, define HAVE_FUNC to 1
>      instead of 0 if glibc implements it as a stub.

This sounds like it could lead to trouble, as it runs afoul of longstanding 
Autoconf tradition. Even within Gnulib, we'd have to investigate and probably 
rewrite code using HAVE_LCHMOD, HAVE_REVOKE, and perhaps other such symbols. And 
we'd also have to warn all downstream users that the HAVE_* tradition is 
changing (at least for glibc symbols), and downstream users would need to change 
their own code that uses HAVE_LCHMOD or whatever. And what about downstream code 
that uses AC_CHECK_FUNCS([lchmod]) or AC_CHECK_FUNCS([sigreturn]) already, and 
gets the longstanding Autoconf interpretation for symbols it checks directly? 
Won't this make things too confusing?

Perhaps it would be better to use HAVE_LCHMOD=0 and REPLACE_LCHMOD=1 in this 
problematic situation, i.e., to bring back the REPLACE_LCHMOD code without 
altering HAVE_LCHMOD. This would be less likely to break other uses, because it 
would not conflict with the Autoconf tradition.

>   * Reinstall (at least partially) the code for REPLACE_LCHMOD=1.

Yes, it sounds like we'll need to do that to support static linking and -lc.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Pádraig Brady]

On 13/02/2020 18:42, Paul Eggert wrote:
> Problem reported by Florian Weimer in:
> https://www.sourceware.org/ml/libc-alpha/2020-02/msg00534.html

A very similar "ENOTSUP" problem is being reported with coreutils-8.32
with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora rawhide in a chroot.
https://bugzilla.redhat.com/1811038

BTW I see a possible small issue in lchmod.c in this code:

+  if (chmod_errno != ENOENT)
+    {
+      errno = chmod_errno;
+      return chmod_result;
+    }

Shouldn't that also check for other lookup errors like ENOTDIR and ELOOP ?

Also mknod() in coreutils is calling lchmod().
Shouldn't it be just calling chmod() as if mknod() was passed a
symlink, then it would have already failed with EEXIST?

cheers,
Pádraig

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Paul Eggert]

On 3/9/20 10:30 AM, Pádraig Brady wrote:

> A very similar "ENOTSUP" problem is being reported with coreutils-8.32
> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora 
> rawhide in a chroot.
> https://bugzilla.redhat.com/1811038

I don't understand that bug report. The strace diff you mentioned in 
Comment 4 looks like the new mknod command is working. And yet the 
original bug report says new mknod command is complaining "Operation not 
supported".

Is the problem that some filesystems don't work with the chmod 
/proc/self/fd/NNN trick, and that it worked for you (the strace diff) 
but not for Mohan (the original bug report)?

> BTW I see a possible small issue in lchmod.c in this code:
> 
> +  if (chmod_errno != ENOENT)
> +    {
> +      errno = chmod_errno;
> +      return chmod_result;
> +    }
> 
> Shouldn't that also check for other lookup errors like ENOTDIR and ELOOP ?

If /proc is mounted you can't get those lookup errors. If /proc is not 
mounted then there shouldn't be anything other than an empty directory 
at /proc, in which case ENOENT is what you'll get. It is true that if 
you put something at /proc other than an empty directory you could get 
arbitrary errno values, but we don't support that sort of misconfiguration.

> Also mknod() in coreutils is calling lchmod().
> Shouldn't it be just calling chmod() as if mknod() was passed a
> symlink, then it would have already failed with EEXIST?

That would still be vulnerable to a race if someone else replaces the 
newly-created special file with a symlink between the time we call 
mknode and chmod.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Pádraig Brady]

On 09/03/2020 18:51, Paul Eggert wrote:
> On 3/9/20 10:30 AM, Pádraig Brady wrote:
> 
>> A very similar "ENOTSUP" problem is being reported with coreutils-8.32
>> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora
>> rawhide in a chroot.
>> https://bugzilla.redhat.com/1811038
> 
> I don't understand that bug report. The strace diff you mentioned in
> Comment 4 looks like the new mknod command is working. And yet the
> original bug report says new mknod command is complaining "Operation not
> supported".
> 
> Is the problem that some filesystems don't work with the chmod
> /proc/self/fd/NNN trick, and that it worked for you (the strace diff)
> but not for Mohan (the original bug report)?

Right, the strace is from my working mknod(1)
to show the differences between 8.31 and 8.32.

I've requested an strace from the failing system.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Florian Weimer]

* Pádraig Brady:

> On 09/03/2020 18:51, Paul Eggert wrote:
>> On 3/9/20 10:30 AM, Pádraig Brady wrote:
>> 
>>> A very similar "ENOTSUP" problem is being reported with coreutils-8.32
>>> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora
>>> rawhide in a chroot.
>>> https://bugzilla.redhat.com/1811038
>> 
>> I don't understand that bug report. The strace diff you mentioned in
>> Comment 4 looks like the new mknod command is working. And yet the
>> original bug report says new mknod command is complaining "Operation not
>> supported".
>> 
>> Is the problem that some filesystems don't work with the chmod
>> /proc/self/fd/NNN trick, and that it worked for you (the strace diff)
>> but not for Mohan (the original bug report)?
>
> Right, the strace is from my working mknod(1)
> to show the differences between 8.31 and 8.32.
>
> I've requested an strace from the failing system.

I guess it's possible that just isn't mounted at this point.

The glibc implementation will definitely *not* add racy fallback in
case /proc is not available, so this will not work until someone
implements the required system call.

It's not clear to my why the mknod command would need fchmodat at all.
With the -m argument, it should simply set the umask to 0, and pass
the mode bits to the mknod function.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Kamil Dudka]

On Tuesday, March 10, 2020 12:52:14 PM CET Florian Weimer wrote:
> * Pádraig Brady:
> > I've requested an strace from the failing system.

The strace in the failing case looks like this:

[...]
umask(000)                              = 022
umask(022)                              = 000
mknod("/dev/random", S_IFCHR|0666, makedev(0x1, 0x8)) = 0
openat(AT_FDCWD, "/dev/random", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3
newfstatat(3, "", {st_mode=S_IFCHR|0644, st_rdev=makedev(0x1, 0x8), ...}, AT_EMPTY_PATH) = 0
chmod("/proc/self/fd/3", 0666)          = -1 ENOENT (No such file or directory)
close(3)                                = 0
write(2, "/bin/mknod: ", 12/bin/mknod: )            = 12
write(2, "cannot set permissions of '/dev/"..., 39cannot set permissions of '/dev/random') = 39
write(2, ": Operation not supported", 25: Operation not supported) = 25
write(2, "\n", 1
)                       = 1
close(1)                                = 0
close(2)                                = 0
exit_group(1)                           = ?

See https://bugzilla.redhat.com/1811038#c11 for a minimal example.

> I guess it's possible that just isn't mounted at this point.

Yes, this happens when /proc is not mounted.  The problem is that mknod used
to succeed in this case with the previous version of coreutils (and gnulib).

> The glibc implementation will definitely *not* add racy fallback in
> case /proc is not available, so this will not work until someone
> implements the required system call.
> 
> It's not clear to my why the mknod command would need fchmodat at all.
> With the -m argument, it should simply set the umask to 0, and pass
> the mode bits to the mknod function.

The call in question was introduced by the following commit:

https://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.21-51-ge7198a67b

But the referred bug report does not seem to mention mknod at all:

https://debbugs.gnu.org/14371

Kamil

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Pádraig Brady]

On 10/03/2020 11:52, Florian Weimer wrote:
> * Pádraig Brady:
> 
>> On 09/03/2020 18:51, Paul Eggert wrote:
>>> On 3/9/20 10:30 AM, Pádraig Brady wrote:
>>>
>>>> A very similar "ENOTSUP" problem is being reported with coreutils-8.32
>>>> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora
>>>> rawhide in a chroot.
>>>> https://bugzilla.redhat.com/1811038
>>>
>>> I don't understand that bug report. The strace diff you mentioned in
>>> Comment 4 looks like the new mknod command is working. And yet the
>>> original bug report says new mknod command is complaining "Operation not
>>> supported".
>>>
>>> Is the problem that some filesystems don't work with the chmod
>>> /proc/self/fd/NNN trick, and that it worked for you (the strace diff)
>>> but not for Mohan (the original bug report)?
>>
>> Right, the strace is from my working mknod(1)
>> to show the differences between 8.31 and 8.32.
>>
>> I've requested an strace from the failing system.
> 
> I guess it's possible that just isn't mounted at this point.
> 
> The glibc implementation will definitely *not* add racy fallback in
> case /proc is not available, so this will not work until someone
> implements the required system call.
> 
> It's not clear to my why the mknod command would need fchmodat at all.
> With the -m argument, it should simply set the umask to 0, and pass
> the mode bits to the mknod function.

umask is not used so as to cater for discrepancies between process and default ACL masks:
https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.21-51-ge7198a67b

An update re this issue.
The strace was supplied in https://bugzilla.redhat.com/1811038
which shows there is no fallback to chmod() in lchmod().
Now the gnulib code does fallback so this issue must be in the glibc implementation.
I also downloaded the problematic rpm, and used nm on the mknod binary
to confirm is was calling out to the glibc implementation.

cheers,
Pádraig

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Florian Weimer]

* Pádraig Brady:

> On 10/03/2020 11:52, Florian Weimer wrote:
>> * Pádraig Brady:
>> 
>>> On 09/03/2020 18:51, Paul Eggert wrote:
>>>> On 3/9/20 10:30 AM, Pádraig Brady wrote:
>>>>
>>>>> A very similar "ENOTSUP" problem is being reported with coreutils-8.32
>>>>> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora
>>>>> rawhide in a chroot.
>>>>> https://bugzilla.redhat.com/1811038
>>>>
>>>> I don't understand that bug report. The strace diff you mentioned in
>>>> Comment 4 looks like the new mknod command is working. And yet the
>>>> original bug report says new mknod command is complaining "Operation not
>>>> supported".
>>>>
>>>> Is the problem that some filesystems don't work with the chmod
>>>> /proc/self/fd/NNN trick, and that it worked for you (the strace diff)
>>>> but not for Mohan (the original bug report)?
>>>
>>> Right, the strace is from my working mknod(1)
>>> to show the differences between 8.31 and 8.32.
>>>
>>> I've requested an strace from the failing system.
>> 
>> I guess it's possible that just isn't mounted at this point.
>> 
>> The glibc implementation will definitely *not* add racy fallback in
>> case /proc is not available, so this will not work until someone
>> implements the required system call.
>> 
>> It's not clear to my why the mknod command would need fchmodat at all.
>> With the -m argument, it should simply set the umask to 0, and pass
>> the mode bits to the mknod function.
>
> umask is not used so as to cater for discrepancies between process and default ACL masks:
> https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.21-51-ge7198a67b

I just don't understand this explanation.  Is the concern here that
you would get a different mode from the requested one if you use
umask+mknod and not mknod+some form of chmod?

> An update re this issue.
> The strace was supplied in https://bugzilla.redhat.com/1811038
> which shows there is no fallback to chmod() in lchmod().
> Now the gnulib code does fallback so this issue must be in the glibc implementation.

The glibc implementation needs /proc to avoid the race.  There is no
way around that, otherwise we introduce a security vulnerability.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Paul Eggert]

On 3/10/20 12:30 PM, Florian Weimer wrote:
> The glibc implementation needs /proc to avoid the race.  There is no
> way around that, otherwise we introduce a security vulnerability.

It is unfortunate that we have dueling paranoia here. coreutils mknod is 
paranoid so it uses lchmod to avoid a race, and then glibc lchmod is paranoid so 
it refuses to work with lchmod unless /proc is mounted.

Since we apparently cannot avoid a race unless /proc is mounted, I suppose we 
could change gnulib lchmod to consider the current glibc behavior to be a bug, 
and to fall back on lstat+chmod when /proc is not mounted. This would fix 
coreutils and every other Gnulib-using program that uses lchmod or fchmodat. But 
on the whole it would be somewhat cleaner if glibc lchmod and fchmodat were 
merely documented to have races when /proc is not mounted; that'd be simpler 
than manually adjusting all programs that use glibc lchmod so that they all 
explicitly have races when /proc is not mounted.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Paul Eggert]

On 3/10/20 12:30 PM, Florian Weimer wrote:

> Is the concern here that
> you would get a different mode from the requested one if you use
> umask+mknod and not mknod+some form of chmod?

Yes. See:

https://bugs.gnu.org/14371

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Florian Weimer]

* Paul Eggert:

> On 3/10/20 12:30 PM, Florian Weimer wrote:
>
>> Is the concern here that
>> you would get a different mode from the requested one if you use
>> umask+mknod and not mknod+some form of chmod?
>
> Yes. See:
>
> https://bugs.gnu.org/14371

Wrong bug?  There is no --mode argument involved in that bug report,
as far as I can see.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Florian Weimer]

* Paul Eggert:

> On 3/10/20 12:30 PM, Florian Weimer wrote:
>> The glibc implementation needs /proc to avoid the race.  There is no
>> way around that, otherwise we introduce a security vulnerability.
>
> It is unfortunate that we have dueling paranoia here. coreutils mknod is 
> paranoid so it uses lchmod to avoid a race, and then glibc lchmod is paranoid so 
> it refuses to work with lchmod unless /proc is mounted.

I now wonder if neither gnulib nor glibc should pretend that they can
implement lchmod and fchmodat on Linux in a usable fashion.

I added the emulation to glibc mostly because it was in gnulib.
Otherwise, I would have insisted that a proper system call be
implemented first.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Kamil Dudka]

On Tuesday, March 10, 2020 8:30:36 PM CET Florian Weimer wrote:
> * Pádraig Brady:
> > On 10/03/2020 11:52, Florian Weimer wrote:
> >> * Pádraig Brady:
> >>> On 09/03/2020 18:51, Paul Eggert wrote:
> >>>> On 3/9/20 10:30 AM, Pádraig Brady wrote:
> >>>>> A very similar "ENOTSUP" problem is being reported with coreutils-8.32
> >>>>> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora
> >>>>> rawhide in a chroot.
> >>>>> https://bugzilla.redhat.com/1811038
> >>>> 
> >>>> I don't understand that bug report. The strace diff you mentioned in
> >>>> Comment 4 looks like the new mknod command is working. And yet the
> >>>> original bug report says new mknod command is complaining "Operation
> >>>> not
> >>>> supported".
> >>>> 
> >>>> Is the problem that some filesystems don't work with the chmod
> >>>> /proc/self/fd/NNN trick, and that it worked for you (the strace diff)
> >>>> but not for Mohan (the original bug report)?
> >>> 
> >>> Right, the strace is from my working mknod(1)
> >>> to show the differences between 8.31 and 8.32.
> >>> 
> >>> I've requested an strace from the failing system.
> >> 
> >> I guess it's possible that just isn't mounted at this point.
> >> 
> >> The glibc implementation will definitely *not* add racy fallback in
> >> case /proc is not available, so this will not work until someone
> >> implements the required system call.
> >> 
> >> It's not clear to my why the mknod command would need fchmodat at all.
> >> With the -m argument, it should simply set the umask to 0, and pass
> >> the mode bits to the mknod function.
> > 
> > umask is not used so as to cater for discrepancies between process and
> > default ACL masks:
> > https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.21-51-ge
> > 7198a67b
> I just don't understand this explanation.  Is the concern here that
> you would get a different mode from the requested one if you use
> umask+mknod and not mknod+some form of chmod?
> 
> > An update re this issue.
> > The strace was supplied in https://bugzilla.redhat.com/1811038
> > which shows there is no fallback to chmod() in lchmod().
> > Now the gnulib code does fallback so this issue must be in the glibc
> > implementation.
> The glibc implementation needs /proc to avoid the race.  There is no
> way around that, otherwise we introduce a security vulnerability.

As I understand it, the change of lchmod() behavior in glibc is intended.
We do not want to revert it for obvious reasons.  On the other hand, the 
change of mknod behavior is unexpected and breaks existing software.

Would not it make sense to fix this in mknod by turning the EOPNOTSUPP
failure into a warning only?

Kamil

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Paul Eggert]

On 3/11/20 1:45 AM, Florian Weimer wrote:
> I now wonder if neither gnulib nor glibc should pretend that they can
> implement lchmod and fchmodat on Linux in a usable fashion.

That ship sailed long ago for Gnulib, since it long emulated lchmod by using 
chmod (with no symlink check!), and Gnulib-using programs rely on this: some 
know about the lack of a symlink check and therefore have races of there own, 
whereas others don't know and have more-serious bugs. At the very least we can 
improve on this by adding a symlink check to Gnulib lchmod. There will still be 
races but some Gnulib-using programs will be better off than before.

Portable programs really need the ability to do lchmod to avoid races, and as 
lchmod works on non-Linux kernels we don't want to lose this capability.

I agree that Linux should have a proper lchmod system call. I am puzzled as to 
why this wasn't added long ago. I suppose the kernel folks thought that lchmod 
was nonsense because symlink permissions are nonsense, and missed the point that 
lchmod avoids races.

For glibc I think it'd be better to keep the change to lchmod and fchmodat. 
Problems occur only when /proc is not mounted, and the problems are the same as 
before the change so this is not a regression. However, I don't feel strongly 
about this.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Paul Eggert]

On 3/11/20 2:04 AM, Kamil Dudka wrote:
> Would not it make sense to fix this in mknod by turning the EOPNOTSUPP
> failure into a warning only?

No, because that would be a regression. mknod used to work in this case, and now 
it doesn't.

Formerly, Gnulib-using programs like mknod worked around the problem that glibc 
lchmod always failed, by silently substituting chmod for lchmod in all cases and 
not bothering to call lchmod. Now that glibc lchmod merely *sometimes* fails, 
Gnulib-using problems need a fancier workaround.

Most likely this workaround will consist of changing the 'configure'-time test 
for lchmod. Currently, the 'configure'-time test checks whether lchmod fails on 
a non-symlink. The simplest fix is to change the 'configure'-time test so that 
it always fails if the Linux kernel is being used. (Presumably we'll do the same 
for Cygwin and for Android.)