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 <br...@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.
>From 4efc18c4a2927074bf0653ba8bd79230661b8238 Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@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 <br...@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 <br...@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
>From ea91b32b0719d46354c1d995c1d1b1c36842a2eb Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@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 <br...@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
