>From ad0024b3fefea56629941e6533705fa3a4820064 Mon Sep 17 00:00:00 2001 From: Paul Eggert 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 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 + + 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 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 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 -/* If the user's config.h happens to include , let it include only - the system's here, so that orig_fchmodat doesn't recurse to - rpl_fchmodat. */ -#define __need_system_sys_stat_h -#include - /* Specification. */ #include -#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 #include @@ -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. */ - 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