From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS22989 209.51.188.0/24 X-Spam-Status: No, score=-4.2 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_LOW, SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 837371F619 for ; Sun, 23 Feb 2020 08:15:18 +0000 (UTC) Received: from localhost ([::1]:50636 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1j5mQ1-0007cp-2D for normalperson@yhbt.net; Sun, 23 Feb 2020 03:15:17 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:44574) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1j5mPt-0007cX-2o for bug-gnulib@gnu.org; Sun, 23 Feb 2020 03:15:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1j5mPp-0001jm-IT for bug-gnulib@gnu.org; Sun, 23 Feb 2020 03:15:08 -0500 Received: from zimbra.cs.ucla.edu ([131.179.128.68]:54730) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1j5mPp-0001iu-6d for bug-gnulib@gnu.org; Sun, 23 Feb 2020 03:15:05 -0500 Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id BEF51160081; Sun, 23 Feb 2020 00:15:03 -0800 (PST) Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id r55Owm5Ghnyh; Sun, 23 Feb 2020 00:15:01 -0800 (PST) Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id C0810160087; Sun, 23 Feb 2020 00:15:01 -0800 (PST) X-Virus-Scanned: amavisd-new at zimbra.cs.ucla.edu Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id jYzhkJH4BfmE; Sun, 23 Feb 2020 00:15:01 -0800 (PST) Received: from [192.168.1.9] (cpe-23-242-74-103.socal.res.rr.com [23.242.74.103]) by zimbra.cs.ucla.edu (Postfix) with ESMTPSA id 88EB9160081; Sun, 23 Feb 2020 00:15:01 -0800 (PST) Subject: Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems To: Bruno Haible References: <20200213184209.34020-1-eggert@cs.ucla.edu> <3279179.fFYm18bb12@omega> <2681568.EWPfcCKDUL@omega> From: Paul Eggert Organization: UCLA Computer Science Department Message-ID: <9cfa2499-f466-065f-4f32-7f68523bde91@cs.ucla.edu> Date: Sun, 23 Feb 2020 00:15:01 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <2681568.EWPfcCKDUL@omega> Content-Type: multipart/mixed; boundary="------------0F4365A32FF7ABF9521D06E8" Content-Language: en-US X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 131.179.128.68 X-BeenThere: bug-gnulib@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Gnulib discussion list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: bug-gnulib@gnu.org Errors-To: bug-gnulib-bounces+normalperson=yhbt.net@gnu.org Sender: "bug-gnulib" This is a multi-part message in MIME format. --------------0F4365A32FF7ABF9521D06E8 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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.... --------------0F4365A32FF7ABF9521D06E8 Content-Type: text/x-patch; charset=UTF-8; name="0001-fchmodat-lchmod-simplify.patch" Content-Disposition: attachment; filename="0001-fchmodat-lchmod-simplify.patch" Content-Transfer-Encoding: quoted-printable >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=3DUTF-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=E2=80=99t know about. * lib/fchmodat.c (fchmodat): * lib/lchmod.c (lchmod): Put the =E2=80=98defined __linux__ || defined __ANDROID__=E2=80=99 #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 =E2=80=98open=E2=80=99 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=E2=80=99t 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=E2=80=99t know about. + * lib/fchmodat.c (fchmodat): + * lib/lchmod.c (lchmod): + Put the =E2=80=98defined __linux__ || defined __ANDROID__=E2=80=99 #ifd= ef 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 =E2=80=98open=E2=80=99 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=E2=80=99t happe= n. + * 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 =20 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 f= lags) { struct stat st; =20 -# 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 f= lags) =20 /* 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 li= nk - - which is undersired - and on many file systems (ext4, btrfs, = jfs, + - which is undesired - and on many file systems (ext4, btrfs, j= fs, 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 f= lags) return -1; } =20 +# if defined __linux__ || defined __ANDROID__ static char const fmt[] =3D "/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, i= nt flags) errno =3D 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 =3D fstatat (dir, file, &st, AT_SYMLINK_NOFOLLO= W); if (fstatat_result !=3D 0) return fstatat_result; @@ -124,11 +124,10 @@ fchmodat (int dir, char const *file, mode_t mode, i= nt flags) errno =3D 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 ra= ce. */ + flags =3D 0; } # endif =20 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 @@ =20 #include =20 -/* If the user's config.h happens to include , let it includ= e 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 =20 #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 =3D openat (AT_FDCWD, file, O_PATH | O_NOFOLLOW | O_CLOEXEC); + int fd =3D open (file, O_PATH | O_NOFOLLOW | O_CLOEXEC); if (fd < 0) return fd; =20 /* 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; } =20 +# if defined __linux__ || defined __ANDROID__ static char const fmt[] =3D "/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 =3D 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 __ANDR= OID__)) \ - || !HAVE_LCHMOD /* older Linux, Solar= is 10 */ struct stat st; int lstat_result =3D lstat (file, &st); if (lstat_result !=3D 0) @@ -125,12 +103,8 @@ lchmod (char const *file, mode_t mode) errno =3D 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 Wi= ndows */ - 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 portabl= e - " #if @GNULIB_LCHMOD@ /* Change the mode of FILENAME to MODE, without dereferencing it if FILE= NAME 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 =20 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], =20 AC_REQUIRE([AC_CANONICAL_HOST]) dnl for cross-compiles =20 - AC_CHECK_FUNCS_ONCE([fchmodat lchmod lstat]) + AC_CHECK_FUNCS_ONCE([lchmod lstat]) if test "$ac_cv_func_lchmod" =3D no; then HAVE_LCHMOD=3D0 - 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 =3D S_IRWXU | S_IRWXG | S_IRWXO; - int desired =3D S_IRUSR | S_IWUSR; - static char const f[] =3D "conftest.lchmod"; - struct stat st; - if (creat (f, permissive) < 0) - return 1; - if (lchmod (f, desired) !=3D 0) - return 1; - if (stat (f, &st) !=3D 0) - return 1; - return ! ((st.st_mode & permissive) =3D=3D desired); - ]])], - [gl_cv_func_lchmod_works=3Dyes], - [gl_cv_func_lchmod_works=3Dno], - [case "$host_os" in - dnl Guess no on Linux with glibc, yes otherwise. - linux-gnu*) gl_cv_func_lchmod_works=3D"guessing no" ;; - *) gl_cv_func_lchmod_works=3D"$gl_cross_guess_norma= l" ;; - 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=3D1 - ;; - esac fi ]) =20 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=3D0; AC_SUBST([REPLACE_FSTAT]) REPLACE_FSTATAT=3D0; AC_SUBST([REPLACE_FSTATAT]) REPLACE_FUTIMENS=3D0; AC_SUBST([REPLACE_FUTIMENS]) - REPLACE_LCHMOD=3D0; AC_SUBST([REPLACE_LCHMOD]) REPLACE_LSTAT=3D0; AC_SUBST([REPLACE_LSTAT]) REPLACE_MKDIR=3D0; AC_SUBST([REPLACE_MKDIR]) REPLACE_MKFIFO=3D0; 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 =20 Depends-on: -errno [test $HAVE_LCHMOD =3D 0 || test $REPLACE_LCHMOD =3D 1] +errno [test $HAVE_LCHMOD =3D 0] extensions -fcntl-h [test $HAVE_LCHMOD =3D 0 || test $REPLACE_LCHMOD =3D 1] -fchmodat [test $HAVE_LCHMOD =3D 0 || test $REPLACE_LCHMOD =3D 1] -intprops [test $HAVE_LCHMOD =3D 0 || test $REPLACE_LCHMOD =3D 1] -lstat [test $HAVE_LCHMOD =3D 0 || test $REPLACE_LCHMOD =3D 1] +fcntl-h [test $HAVE_LCHMOD =3D 0] +intprops [test $HAVE_LCHMOD =3D 0] +lstat [test $HAVE_LCHMOD =3D 0] sys_stat -unistd [test $HAVE_LCHMOD =3D 0 || test $REPLACE_LCHMOD =3D 1] +unistd [test $HAVE_LCHMOD =3D 0] =20 configure.ac: gl_FUNC_LCHMOD -if test $HAVE_LCHMOD =3D 0 || test $REPLACE_LCHMOD =3D 1; then +if test $HAVE_LCHMOD =3D 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' \ --=20 2.17.1 --------------0F4365A32FF7ABF9521D06E8--