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

[Paul Eggert <eggert@cs.ucla.edu>]

[-- 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