Re: bug#39236: [musl] coreutils cp mishandles error return from lchmod

Re: bug#39236: [musl] coreutils cp mishandles error return from lchmod

[Paul Eggert]

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

On 1/22/20 2:05 PM, Rich Felker wrote:
> I think we're approaching a consensus that glibc should fix this too,
> so then it would just be gnulib matching the fix.

I installed the attached patch to Gnulib in preparation for the upcoming 
glibc fix. The patch causes fchmodat with AT_SYMLINK_NOFOLLOW to work on 
non-symlinks, and similarly for lchmod on non-symlinks. The idea is to 
avoid this sort of problem in the future, and to let Coreutils etc. work 
on older platforms as if glibc 2.32 (or whatever) is already in place.

[-- Attachment #2: 0001-fchmodat-AT_SYMLINK_NOFOLLOW-fix-for-non-symlinks.patch --]
[-- Type: text/x-patch, Size: 24627 bytes --]

From b16a04394121e7396569a13161dba02c6752b19f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 7 Feb 2020 16:34:12 -0800
Subject: [PATCH] fchmodat: AT_SYMLINK_NOFOLLOW fix for non-symlinks

Fix lchmod, and fchmodat with AT_SYMLINK_NOFOLLOW, so that
they act like chmod on non-symlinks.
* NEWS:
* doc/glibc-functions/lchmod.texi (lchmod):
* doc/posix-functions/fchmodat.texi (fchmodat):
Mention this.
* lib/fchmodat.c: Define __need_system_sys_stat_h before including
config.h, and undef it after including sys/stat.h the first time.
Include fcntl.h, stdio.h, unistd.h, intprops.h, and include
sys/stat.h a second time after defining orig_fchmodat.
(orig_fchmodat) [HAVE_FCHMODAT]: New function.
(fchmodat) [HAVE_FCHMODAT]: Work around the AT_SYMLINK_NOFOLLOW bug.
* lib/lchmod.c: New file.
* lib/sys_stat.in.h (fchmodat, lchmod):
Support replacing these functions.
* m4/fchmodat.m4 (gl_FUNC_FCHMODAT): If fchmodat exists,
test that AT_SYMLINK_NOFOLLOW works on non-symlinks.
* m4/lchmod.m4 (gl_FUNC_LCHMOD): Check for lstat.
Test that lchmod works on non-symlinks.
* m4/sys_stat_h.m4 (gl_SYS_STAT_H_DEFAULTS):
Default REPLACE_FCHMODAT and REPLACE_LCHMOD to 0.
* modules/fchmodat (Depends-on): Add fstatat, intprops, lchmod, unistd.
(Depends-on, configure.ac): Check REPLACE_FCHMODAT too.
* modules/lchmod (Files): Add lib/lchmod.c.
(Depends-on): Add errno, fcntl-h, fchmodat, intprops, lstat, unistd.
(configure.ac): Compile lchmod.c if needed.
(lib_SOURCES): Add lchmod.c.
* modules/sys_stat (sys/stat.h): Substitute REPLACE_FCHMODAT
and REPLACE_LCHMOD.
* tests/test-fchmodat.c: Include fcntl.h, sys/stat.h.
(main): Test fchmodat with AT_SYMLINK_NOFOLLOW on non-symlinks.
---
 ChangeLog                         | 35 ++++++++++++
 NEWS                              |  7 +++
 doc/glibc-functions/lchmod.texi   |  4 ++
 doc/posix-functions/fchmodat.texi | 11 ++--
 lib/fchmodat.c                    | 89 +++++++++++++++++++++++++------
 lib/lchmod.c                      | 72 +++++++++++++++++++++++++
 lib/sys_stat.in.h                 | 41 +++++++-------
 m4/fchmodat.m4                    | 48 ++++++++++++++++-
 m4/lchmod.m4                      | 52 ++++++++++++++++--
 m4/sys_stat_h.m4                  |  4 +-
 modules/fchmodat                  | 10 ++--
 modules/lchmod                    | 13 ++++-
 modules/sys_stat                  |  2 +
 tests/test-fchmodat.c             | 10 ++++
 14 files changed, 348 insertions(+), 50 deletions(-)
 create mode 100644 lib/lchmod.c

diff --git a/ChangeLog b/ChangeLog
index 99e0c2e9e..71dcaba6c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,38 @@
+2020-02-07  Paul Eggert  <eggert@cs.ucla.edu>
+
+	fchmodat: AT_SYMLINK_NOFOLLOW fix for non-symlinks
+	Fix lchmod, and fchmodat with AT_SYMLINK_NOFOLLOW, so that
+	they act like chmod on non-symlinks.
+	* NEWS:
+	* doc/glibc-functions/lchmod.texi (lchmod):
+	* doc/posix-functions/fchmodat.texi (fchmodat):
+	Mention this.
+	* lib/fchmodat.c: Define __need_system_sys_stat_h before including
+	config.h, and undef it after including sys/stat.h the first time.
+	Include fcntl.h, stdio.h, unistd.h, intprops.h, and include
+	sys/stat.h a second time after defining orig_fchmodat.
+	(orig_fchmodat) [HAVE_FCHMODAT]: New function.
+	(fchmodat) [HAVE_FCHMODAT]: Work around the AT_SYMLINK_NOFOLLOW bug.
+	* lib/lchmod.c: New file.
+	* lib/sys_stat.in.h (fchmodat, lchmod):
+	Support replacing these functions.
+	* m4/fchmodat.m4 (gl_FUNC_FCHMODAT): If fchmodat exists,
+	test that AT_SYMLINK_NOFOLLOW works on non-symlinks.
+	* m4/lchmod.m4 (gl_FUNC_LCHMOD): Check for lstat.
+	Test that lchmod works on non-symlinks.
+	* m4/sys_stat_h.m4 (gl_SYS_STAT_H_DEFAULTS):
+	Default REPLACE_FCHMODAT and REPLACE_LCHMOD to 0.
+	* modules/fchmodat (Depends-on): Add fstatat, intprops, lchmod, unistd.
+	(Depends-on, configure.ac): Check REPLACE_FCHMODAT too.
+	* modules/lchmod (Files): Add lib/lchmod.c.
+	(Depends-on): Add errno, fcntl-h, fchmodat, intprops, lstat, unistd.
+	(configure.ac): Compile lchmod.c if needed.
+	(lib_SOURCES): Add lchmod.c.
+	* modules/sys_stat (sys/stat.h): Substitute REPLACE_FCHMODAT
+	and REPLACE_LCHMOD.
+	* tests/test-fchmodat.c: Include fcntl.h, sys/stat.h.
+	(main): Test fchmodat with AT_SYMLINK_NOFOLLOW on non-symlinks.
+
 2020-02-05  Marc Dionne  <marc.dionne@auristor.com>  (tiny change)
 
 	mountlist: Consider AFS filesystems as remote
diff --git a/NEWS b/NEWS
index dc5cc71f9..bc81dfc28 100644
--- a/NEWS
+++ b/NEWS
@@ -58,6 +58,13 @@ User visible incompatible changes
 
 Date        Modules         Changes
 
+2020-02-07  fchmodat        When applied to non-symlinks, these now act like
+            lchmod          chmod (the BSD behavior, which POSIX requires for
+                            fchmodat + AT_SYMLINK_NOFOLLOW), instead of failing
+                            (the GNU/Linux behavior through glibc 2.31).
+                            Future versions of GNU/Linux are planned to act as
+                            per POSIX and BSD.
+
 2020-01-15  gc-pbkdf2-sha1  This module is deprecated.  Use gc-pbkdf2 instead.
 
 2019-12-12  dfa             Its API now uses ptrdiff_t instead of size_t.
diff --git a/doc/glibc-functions/lchmod.texi b/doc/glibc-functions/lchmod.texi
index aea782dc2..6cc48b453 100644
--- a/doc/glibc-functions/lchmod.texi
+++ b/doc/glibc-functions/lchmod.texi
@@ -9,6 +9,10 @@ Portability problems fixed by Gnulib:
 @item
 This function is missing on some platforms:
 OpenBSD 3.8, Minix 3.1.8, AIX 5.1, IRIX 6.5, Solaris 11.4, Cygwin, mingw, MSVC 14, Android 9.0.
+@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.
 @end itemize
 
 Portability problems not fixed by Gnulib:
diff --git a/doc/posix-functions/fchmodat.texi b/doc/posix-functions/fchmodat.texi
index 6add5ba89..4d190310e 100644
--- a/doc/posix-functions/fchmodat.texi
+++ b/doc/posix-functions/fchmodat.texi
@@ -9,6 +9,11 @@ Gnulib module: fchmodat
 Portability problems fixed by Gnulib:
 @itemize
 @item
+When given the @code{AT_SYMLINK_NOFOLLOW} flag,
+this function fails with @code{errno} set to @code{ENOTSUP},
+even when the file is not a symbolic link:
+GNU/Linux and Cygwin with glibc 2.31.
+@item
 This function is missing on some platforms:
 glibc 2.3.6, Mac OS X 10.5, FreeBSD 6.0, NetBSD 5.0, OpenBSD 3.8, Minix 3.1.8,
 AIX 5.1, HP-UX 11, IRIX 6.5, Solaris 10, Cygwin 1.5.x, mingw, MSVC 14.
@@ -19,9 +24,5 @@ Portability problems not fixed by Gnulib:
 @itemize
 @item
 Some platforms do not allow changing the access bits on symbolic
-links.  POSIX states that @code{fchmodat(@dots{},AT_SYMLINK_NOFOLLOW)}
-may fail with @code{EOPNOTSUPP} when called on a symlink, but some
-platforms, as well as the gnulib replacement, fail for any use of
-AT_SYMLINK_NOFOLLOW even if the target was not a symlink:
-glibc, Cygwin.
+links.
 @end itemize
diff --git a/lib/fchmodat.c b/lib/fchmodat.c
index 0d4891b3e..c6b8ef768 100644
--- a/lib/fchmodat.c
+++ b/lib/fchmodat.c
@@ -16,29 +16,42 @@
 
 /* written by Jim Meyering */
 
+/* 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
 
 #include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
 #include <stdlib.h>
+#include <unistd.h>
 
-#ifndef HAVE_LCHMOD
-/* Use a different name, to avoid conflicting with any
-   system-supplied declaration.  */
-# undef lchmod
-# define lchmod lchmod_rpl
+#if HAVE_FCHMODAT
 static int
-lchmod (char const *f _GL_UNUSED, mode_t m _GL_UNUSED)
+orig_fchmodat (int dir, char const *file, mode_t mode, int flags)
 {
-  errno = ENOSYS;
-  return -1;
+  return fchmodat (dir, file, mode, flags);
 }
 #endif
 
-/* Solaris 10 has no function like this.
-   Invoke chmod or lchmod on file, FILE, using mode MODE, in the directory
+#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>
+
+/* Invoke chmod or lchmod on FILE, using mode MODE, in the directory
    open on descriptor FD.  If possible, do it without changing the
    working directory.  Otherwise, resort to using save_cwd/fchdir,
    then (chmod|lchmod)/restore_cwd.  If either the save_cwd or the
@@ -46,10 +59,52 @@ lchmod (char const *f _GL_UNUSED, mode_t m _GL_UNUSED)
    Note that an attempt to use a FLAG value of AT_SYMLINK_NOFOLLOW
    on a system without lchmod support causes this function to fail.  */
 
-#define AT_FUNC_NAME fchmodat
-#define AT_FUNC_F1 lchmod
-#define AT_FUNC_F2 chmod
-#define AT_FUNC_USE_F1_COND AT_SYMLINK_NOFOLLOW
-#define AT_FUNC_POST_FILE_PARAM_DECLS , mode_t mode, int flag
-#define AT_FUNC_POST_FILE_ARGS        , mode
-#include "at-func.c"
+#if HAVE_FCHMODAT
+int
+fchmodat (int dir, char const *file, mode_t mode, int flags)
+{
+  if (flags & AT_SYMLINK_NOFOLLOW)
+    {
+# ifdef O_PATH
+      int fd = openat (dir, file, O_PATH | O_NOFOLLOW | O_CLOEXEC);
+      if (fd < 0)
+        return fd;
+      static char const fmt[] = "/proc/self/fd/%d";
+      char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];
+      sprintf (buf, fmt, fd);
+      int chmod_result = chmod (buf, mode);
+      int chmod_errno = errno;
+      close (fd);
+      if (chmod_result == 0)
+        return chmod_result;
+      if (chmod_errno != ENOENT)
+        {
+          errno = chmod_errno;
+          return chmod_result;
+        }
+      /* /proc is not mounted; fall back on racy implementation.  */
+# endif
+
+      struct stat st;
+      int fstatat_result = fstatat (dir, file, &st, AT_SYMLINK_NOFOLLOW);
+      if (fstatat_result != 0)
+        return fstatat_result;
+      if (S_ISLNK (st.st_mode))
+        {
+          errno = EOPNOTSUPP;
+          return -1;
+        }
+      flags &= ~AT_SYMLINK_NOFOLLOW;
+    }
+
+  return orig_fchmodat (dir, file, mode, flags);
+}
+#else
+# define AT_FUNC_NAME fchmodat
+# define AT_FUNC_F1 lchmod
+# define AT_FUNC_F2 chmod
+# define AT_FUNC_USE_F1_COND AT_SYMLINK_NOFOLLOW
+# define AT_FUNC_POST_FILE_PARAM_DECLS , mode_t mode, int flag
+# define AT_FUNC_POST_FILE_ARGS        , mode
+# include "at-func.c"
+#endif
diff --git a/lib/lchmod.c b/lib/lchmod.c
new file mode 100644
index 000000000..cc260ce4d
--- /dev/null
+++ b/lib/lchmod.c
@@ -0,0 +1,72 @@
+/* Implement lchmod on platforms where it does not work correctly.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program 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 General Public License for more details.
+
+   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 Paul Eggert */
+
+#include <config.h>
+
+#include <sys/stat.h>
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <unistd.h>
+
+#include <intprops.h>
+
+/* Work like lchmod, except when FILE is a symbolic link.
+   In that case, set errno to EOPNOTSUPP and return -1.  */
+
+int
+lchmod (char const *file, mode_t mode)
+{
+#if HAVE_FCHMODAT
+  return fchmodat (AT_FDCWD, file, mode, AT_SYMLINK_NOFOLLOW);
+#elif defined O_PATH && defined AT_FDCWD
+  int fd = openat (AT_FDCWD, file, O_PATH | O_NOFOLLOW | O_CLOEXEC);
+  if (fd < 0)
+    return fd;
+  static char const fmt[] = "/proc/self/fd/%d";
+  char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)];
+  sprintf (buf, fmt, fd);
+  int chmod_result = chmod (buf, mode);
+  int chmod_errno = errno;
+  close (fd);
+  if (chmod_result == 0)
+    return chmod_result;
+  if (chmod_errno != ENOENT)
+    {
+      errno = chmod_errno;
+      return chmod_result;
+    }
+  /* /proc is not mounted; fall back on racy implementation.  */
+#endif
+
+#if HAVE_LSTAT
+  struct stat st;
+  int lstat_result = lstat (file, &st);
+  if (lstat_result != 0)
+    return lstat_result;
+  if (S_ISLNK (st.st_mode))
+    {
+      errno = EOPNOTSUPP;
+      return -1;
+    }
+#endif
+
+  return chmod (file, mode);
+}
diff --git a/lib/sys_stat.in.h b/lib/sys_stat.in.h
index 5c891e7b8..4f9eb5976 100644
--- a/lib/sys_stat.in.h
+++ b/lib/sys_stat.in.h
@@ -392,13 +392,25 @@ struct stat
 
 
 #if @GNULIB_FCHMODAT@
-# if !@HAVE_FCHMODAT@
+# if @REPLACE_FCHMODAT@
+#  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
+#   undef fchmodat
+#   define fchmodat rpl_fchmodat
+#  endif
+_GL_FUNCDECL_RPL (fchmodat, int,
+                  (int fd, char const *file, mode_t mode, int flag)
+                  _GL_ARG_NONNULL ((2)));
+_GL_CXXALIAS_RPL (fchmodat, int,
+                  (int fd, char const *file, mode_t mode, int flag));
+# else
+#  if !@HAVE_FCHMODAT@
 _GL_FUNCDECL_SYS (fchmodat, int,
                   (int fd, char const *file, mode_t mode, int flag)
                   _GL_ARG_NONNULL ((2)));
-# endif
+#  endif
 _GL_CXXALIAS_SYS (fchmodat, int,
                   (int fd, char const *file, mode_t mode, int flag));
+# endif
 _GL_CXXALIASWARN (fchmodat);
 #elif defined GNULIB_POSIXCHECK
 # undef fchmodat
@@ -502,31 +514,24 @@ _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 !@HAVE_LCHMOD@
-/* The lchmod replacement follows symbolic links.  Callers should take
-   this into account; lchmod should be applied only to arguments that
-   are known to not be symbolic links.  On hosts that lack lchmod,
-   this can lead to race conditions between the check and the
-   invocation of lchmod, but we know of no workarounds that are
-   reliable in general.  You might try requesting support for lchmod
-   from your operating system supplier.  */
+# if @REPLACE_LCHMOD@
 #  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
-#   define lchmod chmod
+#   undef lchmod
+#   define lchmod rpl_lchmod
 #  endif
-/* Need to cast, because on mingw, the second parameter of chmod is
-                                                int mode.  */
-_GL_CXXALIAS_RPL_CAST_1 (lchmod, chmod, int,
-                         (const char *filename, mode_t mode));
+_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 0 /* assume already declared */
+#  if !@HAVE_LCHMOD@
 _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
-# if @HAVE_LCHMOD@
 _GL_CXXALIASWARN (lchmod);
-# endif
 #elif defined GNULIB_POSIXCHECK
 # undef lchmod
 # if HAVE_RAW_DECL_LCHMOD
diff --git a/m4/fchmodat.m4 b/m4/fchmodat.m4
index 460a4dc17..8195ef668 100644
--- a/m4/fchmodat.m4
+++ b/m4/fchmodat.m4
@@ -1,4 +1,4 @@
-# fchmodat.m4 serial 1
+# fchmodat.m4 serial 2
 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,
@@ -13,5 +13,51 @@ AC_DEFUN([gl_FUNC_FCHMODAT],
   AC_CHECK_FUNCS_ONCE([fchmodat lchmod])
   if test $ac_cv_func_fchmodat != yes; then
     HAVE_FCHMODAT=0
+  else
+    AC_CACHE_CHECK(
+      [whether fchmodat+AT_SYMLINK_NOFOLLOW works on non-symlinks],
+      [gl_cv_func_fchmodat_works],
+      [AC_RUN_IFELSE(
+         [AC_LANG_PROGRAM(
+            [
+              AC_INCLUDES_DEFAULT[
+              #include <fcntl.h>
+              #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.fchmodat";
+              struct stat st;
+              if (creat (f, permissive) < 0)
+                return 1;
+              if (fchmodat (AT_FDCWD, f, desired, AT_SYMLINK_NOFOLLOW) != 0)
+                return 1;
+              if (stat (f, &st) != 0)
+                return 1;
+              return ! ((st.st_mode & permissive) == desired);
+            ]])],
+         [gl_cv_func_fchmodat_works=yes],
+         [gl_cv_func_fchmodat_works=no],
+         [gl_cv_func_fchmodat_works=$gl_cross_guess_normal])
+       rm -f conftest.fchmodat])
+    case $gl_cv_func_fchmodat_works in
+      *yes) ;;
+      *) REPLACE_FCHMODAT=1;;
+    esac
   fi
 ])
diff --git a/m4/lchmod.m4 b/m4/lchmod.m4
index eeca7ac20..68dab7a4a 100644
--- a/m4/lchmod.m4
+++ b/m4/lchmod.m4
@@ -1,4 +1,4 @@
-#serial 3
+#serial 4
 
 dnl Copyright (C) 2005-2006, 2008-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
@@ -6,7 +6,7 @@ dnl gives unlimited permission to copy and/or distribute it,
 dnl with or without modifications, as long as this notice is preserved.
 
 dnl From Paul Eggert.
-dnl Provide a replacement for lchmod on hosts that lack it.
+dnl Provide a replacement for lchmod on hosts that lack a working version.
 
 AC_DEFUN([gl_FUNC_LCHMOD],
 [
@@ -15,8 +15,52 @@ AC_DEFUN([gl_FUNC_LCHMOD],
   dnl Persuade glibc <sys/stat.h> to declare lchmod().
   AC_REQUIRE([AC_USE_SYSTEM_EXTENSIONS])
 
-  AC_CHECK_FUNCS_ONCE([lchmod])
-  if test $ac_cv_func_lchmod = no; then
+  AC_CHECK_FUNCS_ONCE([fchmodat 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],
+         [gl_cv_func_lchmod_works=$gl_cross_guess_normal])
+       rm -f conftest.lchmod])
+    case $gl_cv_func_lchmod_works in
+      *yes) ;;
+      *) REPLACE_LCHMOD=1;;
+    esac
   fi
 ])
diff --git a/m4/sys_stat_h.m4 b/m4/sys_stat_h.m4
index d63df9ebf..a5f35d46d 100644
--- a/m4/sys_stat_h.m4
+++ b/m4/sys_stat_h.m4
@@ -1,4 +1,4 @@
-# sys_stat_h.m4 serial 31   -*- Autoconf -*-
+# sys_stat_h.m4 serial 32   -*- Autoconf -*-
 dnl Copyright (C) 2006-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,
@@ -88,9 +88,11 @@ AC_DEFUN([gl_SYS_STAT_H_DEFAULTS],
   HAVE_MKNOD=1;         AC_SUBST([HAVE_MKNOD])
   HAVE_MKNODAT=1;       AC_SUBST([HAVE_MKNODAT])
   HAVE_UTIMENSAT=1;     AC_SUBST([HAVE_UTIMENSAT])
+  REPLACE_FCHMODAT=0;   AC_SUBST([REPLACE_FCHMODAT])
   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/fchmodat b/modules/fchmodat
index 7a5e1a688..b0f06e862 100644
--- a/modules/fchmodat
+++ b/modules/fchmodat
@@ -12,17 +12,21 @@ sys_stat
 extensions
 at-internal     [test $HAVE_FCHMODAT = 0]
 dosname         [test $HAVE_FCHMODAT = 0]
-errno           [test $HAVE_FCHMODAT = 0]
+errno           [test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1]
 extern-inline   [test $HAVE_FCHMODAT = 0]
 fchdir          [test $HAVE_FCHMODAT = 0]
-fcntl-h         [test $HAVE_FCHMODAT = 0]
+fcntl-h         [test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1]
+fstatat         [test $REPLACE_FCHMODAT = 1]
+intprops        [test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1]
+lchmod          [test $HAVE_FCHMODAT = 0]
 openat-die      [test $HAVE_FCHMODAT = 0]
 openat-h        [test $HAVE_FCHMODAT = 0]
 save-cwd        [test $HAVE_FCHMODAT = 0]
+unistd          [test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1]
 
 configure.ac:
 gl_FUNC_FCHMODAT
-if test $HAVE_FCHMODAT = 0; then
+if test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1; then
   AC_LIBOBJ([fchmodat])
 fi
 gl_MODULE_INDICATOR([fchmodat]) dnl for lib/openat.h
diff --git a/modules/lchmod b/modules/lchmod
index a0ac7a534..e1054f6a4 100644
--- a/modules/lchmod
+++ b/modules/lchmod
@@ -2,17 +2,28 @@ Description:
 lchmod that is actually chmod (!) on hosts lacking lchmod
 
 Files:
+lib/lchmod.c
 m4/lchmod.m4
 
 Depends-on:
-sys_stat
+errno         [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
 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]
+sys_stat
+unistd        [test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1]
 
 configure.ac:
 gl_FUNC_LCHMOD
+if test $HAVE_LCHMOD = 0 || test $REPLACE_LCHMOD = 1; then
+  AC_LIBOBJ([lchmod])
+fi
 gl_SYS_STAT_MODULE_INDICATOR([lchmod])
 
 Makefile.am:
+lib_SOURCES += lchmod.c
 
 Include:
 <sys/stat.h>
diff --git a/modules/sys_stat b/modules/sys_stat
index 867cc8549..93e0cf07e 100644
--- a/modules/sys_stat
+++ b/modules/sys_stat
@@ -59,9 +59,11 @@ sys/stat.h: sys_stat.in.h $(top_builddir)/config.status $(CXXDEFS_H) $(ARG_NONNU
 	      -e 's|@''HAVE_MKNOD''@|$(HAVE_MKNOD)|g' \
 	      -e 's|@''HAVE_MKNODAT''@|$(HAVE_MKNODAT)|g' \
 	      -e 's|@''HAVE_UTIMENSAT''@|$(HAVE_UTIMENSAT)|g' \
+	      -e 's|@''REPLACE_FCHMODAT''@|$(REPLACE_FCHMODAT)|g' \
 	      -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' \
diff --git a/tests/test-fchmodat.c b/tests/test-fchmodat.c
index 395b2b78e..df0f604d8 100644
--- a/tests/test-fchmodat.c
+++ b/tests/test-fchmodat.c
@@ -22,6 +22,8 @@
 SIGNATURE_CHECK (fchmodat, int, (int, const char *, mode_t, int));
 
 #include <errno.h>
+#include <fcntl.h>
+#include <sys/stat.h>
 #include <unistd.h>
 
 #include "macros.h"
@@ -42,5 +44,13 @@ main (void)
     ASSERT (errno == EBADF);
   }
 
+  /* Test that fchmodat works on non-symlinks, when given
+     the AT_SYMLINK_NOFOLLOW flag.  */
+  {
+    ASSERT (close (creat ("file", 0600)) == 0);
+    ASSERT (fchmodat (AT_FDCWD, "file", 0700, AT_SYMLINK_NOFOLLOW) == 0);
+    ASSERT (unlink ("file") == 0);
+  }
+
   return 0;
 }
-- 
2.24.1

Re: fchmodat with AT_SYMLINK_NOFOLLOW

[Bruno Haible]

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

Hi Paul,

> I installed the attached patch to Gnulib in preparation for the upcoming 
> glibc fix. The patch causes fchmodat with AT_SYMLINK_NOFOLLOW to work on 
> non-symlinks, and similarly for lchmod on non-symlinks. The idea is to 
> avoid this sort of problem in the future, and to let Coreutils etc. work 
> on older platforms as if glibc 2.32 (or whatever) is already in place.

Some improvements:

1) Improve the cross-compilation guesses. The result of the
"fchmodat+AT_SYMLINK_NOFOLLOW works on non-symlinks" test is:
  - yes on kFreeBSD/glibc, Hurd/glibc, FreeBSD 12, AIX 7.2, Solaris 11, Haiku,
  - no on Linux/glibc, Cygwin 2.9.

2) On Cygwin, the functions fchmodat and lchown crash. The cause is an endless
recursion, because some of the #includes in fchmodat.c includes the full
<sys/stat.h>, including the '#define fchmodat rpl_fchmodat'.

3) Strengthen the unit test, and make sure that it does not write files that
other unit tests could possibly write as well.
This test is a bit tricky, because on native Windows, we cannot do arbitrary
chmods:
  - chmod of 700 is equivalent to 600 (since it does not have an execute bit
    on the file system),
  - chmod of 600 sets the mode to 666 - since there is no distinction between
    users, group, and world with this API.
  - After a chmod of 400 (= read-only), unlink() fails.


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

	fchmodat: Strengthen tests.
	* tests/test-fchmodat.c (BASE): New macro.
	(main): Use it, to avoid conflicts with other unit tests. Verify that
	fchmodat changed the file permission bits.

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

	fchmodat: Fix endless recursion on Cygwin (regression from 2020-02-07).
	* lib/fchmodat.c (orig_fchmodat): Move definition to immediately after
	'#undef __need_system_sys_stat_h'.

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

	fchmodat: Improve cross-compilation guesses.
	* m4/fchmodat.m4 (gl_FUNC_FCHMODAT): Require AC_CANONICAL_HOST. When
	cross-compiling, guess depending on the platform.
	* doc/posix-functions/fchmodat.texi: Clarify.


[-- Attachment #2: 0001-fchmodat-Improve-cross-compilation-guesses.patch --]
[-- Type: text/x-patch, Size: 3950 bytes --]

From a680228fe4d39f749cb819e45202c6fec6ca9d29 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sat, 8 Feb 2020 20:38:01 +0100
Subject: [PATCH 1/3] fchmodat: Improve cross-compilation guesses.

* m4/fchmodat.m4 (gl_FUNC_FCHMODAT): Require AC_CANONICAL_HOST. When
cross-compiling, guess depending on the platform.
* doc/posix-functions/fchmodat.texi: Clarify.
---
 ChangeLog                         |  7 +++++++
 doc/posix-functions/fchmodat.texi | 10 +++++-----
 m4/fchmodat.m4                    | 14 +++++++++++---
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index df48e88..32d9a00 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2020-02-08  Bruno Haible  <bruno@clisp.org>
 
+	fchmodat: Improve cross-compilation guesses.
+	* m4/fchmodat.m4 (gl_FUNC_FCHMODAT): Require AC_CANONICAL_HOST. When
+	cross-compiling, guess depending on the platform.
+	* doc/posix-functions/fchmodat.texi: Clarify.
+
+2020-02-08  Bruno Haible  <bruno@clisp.org>
+
 	Fix compilation errors in a testdir created with --with-c++-tests.
 	* lib/c++defs.h (_GL_CXXALIASWARN1_2): Do not use __typeof__ (func),
 	since it does not work any more with g++ >= 4.4.
diff --git a/doc/posix-functions/fchmodat.texi b/doc/posix-functions/fchmodat.texi
index 4d19031..a295f83 100644
--- a/doc/posix-functions/fchmodat.texi
+++ b/doc/posix-functions/fchmodat.texi
@@ -9,15 +9,15 @@ Gnulib module: fchmodat
 Portability problems fixed by Gnulib:
 @itemize
 @item
-When given the @code{AT_SYMLINK_NOFOLLOW} flag,
-this function fails with @code{errno} set to @code{ENOTSUP},
-even when the file is not a symbolic link:
-GNU/Linux and Cygwin with glibc 2.31.
-@item
 This function is missing on some platforms:
 glibc 2.3.6, Mac OS X 10.5, FreeBSD 6.0, NetBSD 5.0, OpenBSD 3.8, Minix 3.1.8,
 AIX 5.1, HP-UX 11, IRIX 6.5, Solaris 10, Cygwin 1.5.x, mingw, MSVC 14.
 But the replacement function is not safe to be used in libraries and is not multithread-safe.
+@item
+When given the @code{AT_SYMLINK_NOFOLLOW} flag,
+this function fails with @code{errno} set to @code{ENOTSUP},
+even when the file is not a symbolic link:
+GNU/Linux with glibc 2.31, Cygwin 2.9.
 @end itemize
 
 Portability problems not fixed by Gnulib:
diff --git a/m4/fchmodat.m4 b/m4/fchmodat.m4
index 8195ef6..f284485 100644
--- a/m4/fchmodat.m4
+++ b/m4/fchmodat.m4
@@ -1,4 +1,4 @@
-# fchmodat.m4 serial 2
+# fchmodat.m4 serial 3
 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,
@@ -10,6 +10,7 @@ AC_DEFUN([gl_FUNC_FCHMODAT],
 [
   AC_REQUIRE([gl_SYS_STAT_H_DEFAULTS])
   AC_REQUIRE([gl_USE_SYSTEM_EXTENSIONS])
+  AC_REQUIRE([AC_CANONICAL_HOST]) dnl for cross-compiles
   AC_CHECK_FUNCS_ONCE([fchmodat lchmod])
   if test $ac_cv_func_fchmodat != yes; then
     HAVE_FCHMODAT=0
@@ -17,7 +18,9 @@ AC_DEFUN([gl_FUNC_FCHMODAT],
     AC_CACHE_CHECK(
       [whether fchmodat+AT_SYMLINK_NOFOLLOW works on non-symlinks],
       [gl_cv_func_fchmodat_works],
-      [AC_RUN_IFELSE(
+      [dnl This test fails on GNU/Linux with glibc 2.31 (but not on
+       dnl GNU/kFreeBSD nor GNU/Hurd) and Cygwin 2.9.
+       AC_RUN_IFELSE(
          [AC_LANG_PROGRAM(
             [
               AC_INCLUDES_DEFAULT[
@@ -53,7 +56,12 @@ AC_DEFUN([gl_FUNC_FCHMODAT],
             ]])],
          [gl_cv_func_fchmodat_works=yes],
          [gl_cv_func_fchmodat_works=no],
-         [gl_cv_func_fchmodat_works=$gl_cross_guess_normal])
+         [case "$host_os" in
+            dnl Guess no on Linux with glibc and Cygwin, yes otherwise.
+            linux-gnu* | cygwin*) gl_cv_func_fchmodat_works="guessing no" ;;
+            *)                    gl_cv_func_fchmodat_works="$gl_cross_guess_normal" ;;
+          esac
+         ])
        rm -f conftest.fchmodat])
     case $gl_cv_func_fchmodat_works in
       *yes) ;;
-- 
2.7.4


[-- Attachment #3: 0002-fchmodat-Fix-endless-recursion-on-Cygwin-regression-.patch --]
[-- Type: text/x-patch, Size: 1804 bytes --]

From 1bbbce29dc592833e79ab6d21472528a01ab67b2 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sat, 8 Feb 2020 20:41:20 +0100
Subject: [PATCH 2/3] fchmodat: Fix endless recursion on Cygwin (regression
 from 2020-02-07).

* lib/fchmodat.c (orig_fchmodat): Move definition to immediately after
'#undef __need_system_sys_stat_h'.
---
 ChangeLog      |  6 ++++++
 lib/fchmodat.c | 12 ++++++------
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 32d9a00..a5ca210 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2020-02-08  Bruno Haible  <bruno@clisp.org>
 
+	fchmodat: Fix endless recursion on Cygwin (regression from 2020-02-07).
+	* lib/fchmodat.c (orig_fchmodat): Move definition to immediately after
+	'#undef __need_system_sys_stat_h'.
+
+2020-02-08  Bruno Haible  <bruno@clisp.org>
+
 	fchmodat: Improve cross-compilation guesses.
 	* m4/fchmodat.m4 (gl_FUNC_FCHMODAT): Require AC_CANONICAL_HOST. When
 	cross-compiling, guess depending on the platform.
diff --git a/lib/fchmodat.c b/lib/fchmodat.c
index c6b8ef7..87aa0d1 100644
--- a/lib/fchmodat.c
+++ b/lib/fchmodat.c
@@ -26,12 +26,6 @@
 #include <sys/stat.h>
 #undef __need_system_sys_stat_h
 
-#include <errno.h>
-#include <fcntl.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <unistd.h>
-
 #if HAVE_FCHMODAT
 static int
 orig_fchmodat (int dir, char const *file, mode_t mode, int flags)
@@ -40,6 +34,12 @@ orig_fchmodat (int dir, char const *file, mode_t mode, int flags)
 }
 #endif
 
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.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>
-- 
2.7.4


[-- Attachment #4: 0003-fchmodat-Strengthen-tests.patch --]
[-- Type: text/x-patch, Size: 2100 bytes --]

From c2d184186846851f173bec9e60781a05001837c6 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sat, 8 Feb 2020 20:47:58 +0100
Subject: [PATCH 3/3] fchmodat: Strengthen tests.

* tests/test-fchmodat.c (BASE): New macro.
(main): Use it, to avoid conflicts with other unit tests. Verify that
fchmodat changed the file permission bits.
---
 ChangeLog             |  7 +++++++
 tests/test-fchmodat.c | 14 +++++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index a5ca210..fc07914 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2020-02-08  Bruno Haible  <bruno@clisp.org>
 
+	fchmodat: Strengthen tests.
+	* tests/test-fchmodat.c (BASE): New macro.
+	(main): Use it, to avoid conflicts with other unit tests. Verify that
+	fchmodat changed the file permission bits.
+
+2020-02-08  Bruno Haible  <bruno@clisp.org>
+
 	fchmodat: Fix endless recursion on Cygwin (regression from 2020-02-07).
 	* lib/fchmodat.c (orig_fchmodat): Move definition to immediately after
 	'#undef __need_system_sys_stat_h'.
diff --git a/tests/test-fchmodat.c b/tests/test-fchmodat.c
index df0f604..f6b695b 100644
--- a/tests/test-fchmodat.c
+++ b/tests/test-fchmodat.c
@@ -28,6 +28,8 @@ SIGNATURE_CHECK (fchmodat, int, (int, const char *, mode_t, int));
 
 #include "macros.h"
 
+#define BASE "test-fchmodat."
+
 int
 main (void)
 {
@@ -47,9 +49,15 @@ main (void)
   /* Test that fchmodat works on non-symlinks, when given
      the AT_SYMLINK_NOFOLLOW flag.  */
   {
-    ASSERT (close (creat ("file", 0600)) == 0);
-    ASSERT (fchmodat (AT_FDCWD, "file", 0700, AT_SYMLINK_NOFOLLOW) == 0);
-    ASSERT (unlink ("file") == 0);
+    struct stat statbuf;
+    unlink (BASE "file");
+    ASSERT (close (creat (BASE "file", 0600)) == 0);
+    ASSERT (fchmodat (AT_FDCWD, BASE "file", 0400, AT_SYMLINK_NOFOLLOW) == 0);
+    ASSERT (stat (BASE "file", &statbuf) >= 0);
+    ASSERT ((statbuf.st_mode & 0700) == 0400);
+    /* Clean up.  */
+    ASSERT (chmod (BASE "file", 0600) == 0);
+    ASSERT (unlink (BASE "file") == 0);
   }
 
   return 0;
-- 
2.7.4

Re: fchmodat with AT_SYMLINK_NOFOLLOW

[Bruno Haible]

On 2020-02-08 I wrote:
> 1) Improve the cross-compilation guesses. The result of the
> "fchmodat+AT_SYMLINK_NOFOLLOW works on non-symlinks" test is:
>   - yes on kFreeBSD/glibc, Hurd/glibc, FreeBSD 12, AIX 7.2, Solaris 11, Haiku,
>   - no on Linux/glibc, Cygwin 2.9.

A similar improvement can be done for lchmod. The result of the
"lchmod works on non-symlinks" test is:
  - on Linux/glibc it is not executed, because lchmod() does not exist;
    but if it would, it would surely report "no",
  - "yes" on Linux with musl libc, GNU/kFreeBSD, GNU/Hurd, Mac OS X 10.5,
    FreeBSD 12.0, NetBSD 8.0, HP-UX 11.31,
  - "no" on no platform I know of (probably older musl libc versions).


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/m4/lchmod.m4 b/m4/lchmod.m4
index 68dab7a..1646bd7 100644
--- a/m4/lchmod.m4
+++ b/m4/lchmod.m4
@@ -1,4 +1,4 @@
-#serial 4
+#serial 5
 
 dnl Copyright (C) 2005-2006, 2008-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
@@ -15,6 +15,8 @@ AC_DEFUN([gl_FUNC_LCHMOD],
   dnl Persuade glibc <sys/stat.h> to declare lchmod().
   AC_REQUIRE([AC_USE_SYSTEM_EXTENSIONS])
 
+  AC_REQUIRE([AC_CANONICAL_HOST]) dnl for cross-compiles
+
   AC_CHECK_FUNCS_ONCE([fchmodat lchmod lstat])
   if test "$ac_cv_func_lchmod" = no; then
     HAVE_LCHMOD=0
@@ -56,7 +58,12 @@ AC_DEFUN([gl_FUNC_LCHMOD],
             ]])],
          [gl_cv_func_lchmod_works=yes],
          [gl_cv_func_lchmod_works=no],
-         [gl_cv_func_lchmod_works=$gl_cross_guess_normal])
+         [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) ;;

Re: bug#39236: [musl] coreutils cp mishandles error return from lchmod

[Bruno Haible]

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

Hi Paul,

> similarly for lchmod on non-symlinks.

When I see a new autoconf test that activates an override, in order to
guarantee a certain documented behaviour, I like to add a unit test -
because in the past we several times had an override that would not
have passed the autoconf test either.

So I added a unit test for 'lchown', and found that on HP-UX 11.31 the function
could not be used because it is not declared. Which leads to these patches:


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

	lchmod: Add tests.
	* tests/test-lchmod.c: New file.
	* modules/lchmod-tests: New file.

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

	lchmod: Ensure declaration on HP-UX.
	* lib/sys_stat.in.h (lchown): Declare also on HP-UX.
	* doc/glibc-functions/lchmod.texi: Mention the HP-UX problem.


[-- Attachment #2: 0001-lchmod-Ensure-declaration-on-HP-UX.patch --]
[-- Type: text/x-patch, Size: 2099 bytes --]

From d863fc54623c497e0ed51fb0d7011415dc943434 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sat, 8 Feb 2020 21:22:15 +0100
Subject: [PATCH 1/2] lchmod: Ensure declaration on HP-UX.

* lib/sys_stat.in.h (lchown): Declare also on HP-UX.
* doc/glibc-functions/lchmod.texi: Mention the HP-UX problem.
---
 ChangeLog                       | 6 ++++++
 doc/glibc-functions/lchmod.texi | 3 +++
 lib/sys_stat.in.h               | 2 +-
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index fc07914..ffb2096 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2020-02-08  Bruno Haible  <bruno@clisp.org>
 
+	lchmod: Ensure declaration on HP-UX.
+	* lib/sys_stat.in.h (lchown): Declare also on HP-UX.
+	* doc/glibc-functions/lchmod.texi: Mention the HP-UX problem.
+
+2020-02-08  Bruno Haible  <bruno@clisp.org>
+
 	fchmodat: Strengthen tests.
 	* tests/test-fchmodat.c (BASE): New macro.
 	(main): Use it, to avoid conflicts with other unit tests. Verify that
diff --git a/doc/glibc-functions/lchmod.texi b/doc/glibc-functions/lchmod.texi
index 6cc48b4..7a2e9d0 100644
--- a/doc/glibc-functions/lchmod.texi
+++ b/doc/glibc-functions/lchmod.texi
@@ -10,6 +10,9 @@ Portability problems fixed by Gnulib:
 This function is missing on some platforms:
 OpenBSD 3.8, Minix 3.1.8, AIX 5.1, IRIX 6.5, Solaris 11.4, Cygwin, mingw, MSVC 14, Android 9.0.
 @item
+This function is not declared on some platforms:
+HP-UX 11.31.
+@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.
diff --git a/lib/sys_stat.in.h b/lib/sys_stat.in.h
index 4f9eb59..65661f4 100644
--- a/lib/sys_stat.in.h
+++ b/lib/sys_stat.in.h
@@ -525,7 +525,7 @@ _GL_FUNCDECL_RPL (lchmod, int,
 _GL_CXXALIAS_RPL (lchmod, int,
                   (char const *filename, mode_t mode));
 # else
-#  if !@HAVE_LCHMOD@
+#  if !@HAVE_LCHMOD@ || defined __hpux
 _GL_FUNCDECL_SYS (lchmod, int, (const char *filename, mode_t mode)
                                _GL_ARG_NONNULL ((1)));
 #  endif
-- 
2.7.4


[-- Attachment #3: 0002-lchmod-Add-tests.patch --]
[-- Type: text/x-patch, Size: 3522 bytes --]

From 8cc34c349afc79e2728093e109a09f9f0aaa4b50 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sat, 8 Feb 2020 21:24:35 +0100
Subject: [PATCH 2/2] lchmod: Add tests.

* tests/test-lchmod.c: New file.
* modules/lchmod-tests: New file.
---
 ChangeLog            |  6 +++++
 modules/lchmod-tests | 13 ++++++++++
 tests/test-lchmod.c  | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+)
 create mode 100644 modules/lchmod-tests
 create mode 100644 tests/test-lchmod.c

diff --git a/ChangeLog b/ChangeLog
index ffb2096..a4ee3dd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2020-02-08  Bruno Haible  <bruno@clisp.org>
 
+	lchmod: Add tests.
+	* tests/test-lchmod.c: New file.
+	* modules/lchmod-tests: New file.
+
+2020-02-08  Bruno Haible  <bruno@clisp.org>
+
 	lchmod: Ensure declaration on HP-UX.
 	* lib/sys_stat.in.h (lchown): Declare also on HP-UX.
 	* doc/glibc-functions/lchmod.texi: Mention the HP-UX problem.
diff --git a/modules/lchmod-tests b/modules/lchmod-tests
new file mode 100644
index 0000000..cbb2537
--- /dev/null
+++ b/modules/lchmod-tests
@@ -0,0 +1,13 @@
+Files:
+tests/test-lchmod.c
+tests/signature.h
+tests/macros.h
+
+Depends-on:
+
+configure.ac:
+
+Makefile.am:
+TESTS += test-lchmod
+check_PROGRAMS += test-lchmod
+test_lchmod_LDADD = $(LDADD) @LIBINTL@
diff --git a/tests/test-lchmod.c b/tests/test-lchmod.c
new file mode 100644
index 0000000..783e608
--- /dev/null
+++ b/tests/test-lchmod.c
@@ -0,0 +1,67 @@
+/* Test changing the protections of a file.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program 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 General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
+
+#include <config.h>
+
+#include <sys/stat.h>
+
+#include "signature.h"
+SIGNATURE_CHECK (lchmod, int, (const char *, mode_t));
+
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "macros.h"
+
+#define BASE "test-lchmod."
+
+int
+main (void)
+{
+  /* Test that lchmod works on non-symlinks.  */
+  {
+    struct stat statbuf;
+    unlink (BASE "file");
+    ASSERT (close (creat (BASE "file", 0600)) == 0);
+    ASSERT (lchmod (BASE "file", 0400) == 0);
+    ASSERT (stat (BASE "file", &statbuf) >= 0);
+    ASSERT ((statbuf.st_mode & 0700) == 0400);
+    /* Clean up.  */
+    ASSERT (chmod (BASE "file", 0600) == 0);
+    ASSERT (unlink (BASE "file") == 0);
+  }
+
+  /* Test that lchmod on symlinks does not modify the symlink target.  */
+  {
+    unlink (BASE "file");
+    unlink (BASE "link");
+    if (symlink (BASE "file", BASE "link") == 0)
+      {
+        struct stat statbuf;
+        ASSERT (close (creat (BASE "file", 0600)) == 0);
+        lchmod (BASE "link", 0400);
+        ASSERT (stat (BASE "file", &statbuf) >= 0);
+        ASSERT ((statbuf.st_mode & 0700) == 0600);
+      }
+    /* Clean up.  */
+    unlink (BASE "file");
+    unlink (BASE "link");
+  }
+
+  return 0;
+}
-- 
2.7.4

Re: bug#39236: [musl] coreutils cp mishandles error return from lchmod

[Florian Weimer]

* Paul Eggert:

> On 1/22/20 2:05 PM, Rich Felker wrote:
>> I think we're approaching a consensus that glibc should fix this too,
>> so then it would just be gnulib matching the fix.
>
> I installed the attached patch to Gnulib in preparation for the upcoming 
> glibc fix. The patch causes fchmodat with AT_SYMLINK_NOFOLLOW to work on 
> non-symlinks, and similarly for lchmod on non-symlinks. The idea is to 
> avoid this sort of problem in the future, and to let Coreutils etc. work 
> on older platforms as if glibc 2.32 (or whatever) is already in place.

The lchmod implementation based on /proc tickles an XFS bug:

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

Re: bug#39236: [musl] coreutils cp mishandles error return from lchmod

[Rich Felker]

On Wed, Feb 12, 2020 at 12:50:19PM +0100, Florian Weimer wrote:
> * Paul Eggert:
> 
> > On 1/22/20 2:05 PM, Rich Felker wrote:
> >> I think we're approaching a consensus that glibc should fix this too,
> >> so then it would just be gnulib matching the fix.
> >
> > I installed the attached patch to Gnulib in preparation for the upcoming 
> > glibc fix. The patch causes fchmodat with AT_SYMLINK_NOFOLLOW to work on 
> > non-symlinks, and similarly for lchmod on non-symlinks. The idea is to 
> > avoid this sort of problem in the future, and to let Coreutils etc. work 
> > on older platforms as if glibc 2.32 (or whatever) is already in place.
> 
> The lchmod implementation based on /proc tickles an XFS bug:
> 
>   <https://sourceware.org/ml/libc-alpha/2020-02/msg00467.html>

Uhg, why does Linux even let the fs driver see whether the chmod is
being performed via a filename, O_PATH fd, or magic symlink in /proc?
It should just be an operation on the inode.

Rich

Re: bug#39236: [musl] coreutils cp mishandles error return from lchmod

[Rich Felker]

On Wed, Feb 12, 2020 at 08:05:55AM -0500, Rich Felker wrote:
> On Wed, Feb 12, 2020 at 12:50:19PM +0100, Florian Weimer wrote:
> > * Paul Eggert:
> > 
> > > On 1/22/20 2:05 PM, Rich Felker wrote:
> > >> I think we're approaching a consensus that glibc should fix this too,
> > >> so then it would just be gnulib matching the fix.
> > >
> > > I installed the attached patch to Gnulib in preparation for the upcoming 
> > > glibc fix. The patch causes fchmodat with AT_SYMLINK_NOFOLLOW to work on 
> > > non-symlinks, and similarly for lchmod on non-symlinks. The idea is to 
> > > avoid this sort of problem in the future, and to let Coreutils etc. work 
> > > on older platforms as if glibc 2.32 (or whatever) is already in place.
> > 
> > The lchmod implementation based on /proc tickles an XFS bug:
> > 
> >   <https://sourceware.org/ml/libc-alpha/2020-02/msg00467.html>
> 
> Uhg, why does Linux even let the fs driver see whether the chmod is
> being performed via a filename, O_PATH fd, or magic symlink in /proc?
> It should just be an operation on the inode.

OK, I don't think it's actually clear from the test that the use of
the magic symlink is the cause. It's plausible that XFS just always
returns failure on success for this operation, and I don't have XFS to
test with.

Note that in any case, musl's lchmod/fchmodat is not affected since it
always refuses to change symlink modes; I did this because I was
worried that chmod on the magic symlink in /proc might pass through
not just to the symlink it refers to, but to the symlink target if one
exists. With current kernel versions it seems that does not happen; is
it safe to assume it doesn't?

Further, I've found some inconsistent behavior with ext4: chmod on the
magic symlink fails with EOPNOTSUPP as in Florian's test, but fchmod
on the O_PATH fd succeeds and changes the symlink mode. This is with
5.4. Cany anyone else confirm this? Is it a problem?

Rich

Re: bug#39236: [musl] coreutils cp mishandles error return from lchmod

[Florian Weimer]

* Rich Felker:

> Note that in any case, musl's lchmod/fchmodat is not affected since it
> always refuses to change symlink modes; I did this because I was
> worried that chmod on the magic symlink in /proc might pass through
> not just to the symlink it refers to, but to the symlink target if one
> exists. With current kernel versions it seems that does not happen; is
> it safe to assume it doesn't?

I saw it happen with sshfs over FUSE. 8-/

Yet another reason to put in a check before performing the chmod.

> Further, I've found some inconsistent behavior with ext4: chmod on the
> magic symlink fails with EOPNOTSUPP as in Florian's test, but fchmod
> on the O_PATH fd succeeds and changes the symlink mode. This is with
> 5.4. Cany anyone else confirm this? Is it a problem?

Interesting. Let me update the other thread.