* [PATCH] fchmodat, lchmod: port to buggy Linux filesystems @ 2020-02-13 18:42 Paul Eggert 2020-02-14 3:29 ` Bruno Haible ` (3 more replies) 0 siblings, 4 replies; 31+ messages in thread From: Paul Eggert @ 2020-02-13 18:42 UTC (permalink / raw) To: bug-gnulib; +Cc: Paul Eggert Problem reported by Florian Weimer in: https://www.sourceware.org/ml/libc-alpha/2020-02/msg00534.html * lib/fchmodat.c (fchmodat): * lib/lchmod.c (lchmod): Don’t assume that chmod on the O_PATH-opened fd will do the right thing on a symbolic link. * lib/fchmodat.c (fchmodat): Don’t attempt to special-case any flag value other than AT_SYMLINK_NOFOLLOW. --- ChangeLog | 13 +++++++++++++ lib/fchmodat.c | 33 ++++++++++++++++++++++++++------- lib/lchmod.c | 27 ++++++++++++++++++++++----- 3 files changed, 61 insertions(+), 12 deletions(-) diff --git a/ChangeLog b/ChangeLog index b5ef9b3a1..8bcf1bf0a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,16 @@ +2020-02-13 Paul Eggert <eggert@cs.ucla.edu> + + fchmodat, lchmod: port to buggy Linux filesystems + Problem reported by Florian Weimer in: + https://www.sourceware.org/ml/libc-alpha/2020-02/msg00534.html + * lib/fchmodat.c (fchmodat): + * lib/lchmod.c (lchmod): + Don’t assume that chmod on the O_PATH-opened fd will do + the right thing on a symbolic link. + * lib/fchmodat.c (fchmodat): + Don’t attempt to special-case + any flag value other than AT_SYMLINK_NOFOLLOW. + 2020-02-11 Paul Eggert <eggert@cs.ucla.edu> lchmod: pacify Coverity CID 1491216 diff --git a/lib/fchmodat.c b/lib/fchmodat.c index 87aa0d191..02e2da956 100644 --- a/lib/fchmodat.c +++ b/lib/fchmodat.c @@ -63,12 +63,31 @@ orig_fchmodat (int dir, char const *file, mode_t mode, int flags) int fchmodat (int dir, char const *file, mode_t mode, int flags) { - if (flags & AT_SYMLINK_NOFOLLOW) + if (flags == AT_SYMLINK_NOFOLLOW) { -# ifdef O_PATH + struct stat st; + +# if defined O_PATH && defined AT_EMPTY_PATH int fd = openat (dir, file, O_PATH | O_NOFOLLOW | O_CLOEXEC); if (fd < 0) return fd; + + /* Use fstatat because fstat does not work on O_PATH descriptors + before Linux 3.6. */ + if (fstatat (fd, "", &st, AT_EMPTY_PATH) != 0) + { + int stat_errno = errno; + close (fd); + errno = stat_errno; + return -1; + } + if (S_ISLNK (st.st_mode)) + { + close (fd); + errno = EOPNOTSUPP; + return -1; + } + static char const fmt[] = "/proc/self/fd/%d"; char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)]; sprintf (buf, fmt, fd); @@ -82,10 +101,8 @@ fchmodat (int dir, char const *file, mode_t mode, int flags) errno = chmod_errno; return chmod_result; } - /* /proc is not mounted; fall back on racy implementation. */ -# endif - - struct stat st; + /* /proc is not mounted. */ +# else int fstatat_result = fstatat (dir, file, &st, AT_SYMLINK_NOFOLLOW); if (fstatat_result != 0) return fstatat_result; @@ -94,7 +111,9 @@ fchmodat (int dir, char const *file, mode_t mode, int flags) errno = EOPNOTSUPP; return -1; } - flags &= ~AT_SYMLINK_NOFOLLOW; +# endif + /* Fall back on chmod, despite the race. */ + flags = 0; } return orig_fchmodat (dir, file, mode, flags); diff --git a/lib/lchmod.c b/lib/lchmod.c index 5fc658023..c7191c07d 100644 --- a/lib/lchmod.c +++ b/lib/lchmod.c @@ -37,10 +37,28 @@ lchmod (char const *file, mode_t mode) #if HAVE_FCHMODAT return fchmodat (AT_FDCWD, file, mode, AT_SYMLINK_NOFOLLOW); #else -# if defined O_PATH && defined AT_FDCWD +# if defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH 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 + before Linux 3.6. */ + struct stat st; + if (fstatat (fd, "", &st, AT_EMPTY_PATH) != 0) + { + int stat_errno = errno; + close (fd); + errno = stat_errno; + return -1; + } + if (S_ISLNK (st.st_mode)) + { + close (fd); + errno = EOPNOTSUPP; + return -1; + } + static char const fmt[] = "/proc/self/fd/%d"; char buf[sizeof fmt - sizeof "%d" + INT_BUFSIZE_BOUND (int)]; sprintf (buf, fmt, fd); @@ -54,10 +72,8 @@ lchmod (char const *file, mode_t mode) errno = chmod_errno; return chmod_result; } - /* /proc is not mounted; fall back on racy implementation. */ -# endif - -# if HAVE_LSTAT + /* /proc is not mounted. */ +# elif HAVE_LSTAT struct stat st; int lstat_result = lstat (file, &st); if (lstat_result != 0) @@ -69,6 +85,7 @@ lchmod (char const *file, mode_t mode) } # endif + /* Fall back on chmod, despite the race. */ return chmod (file, mode); #endif } -- 2.24.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-13 18:42 [PATCH] fchmodat, lchmod: port to buggy Linux filesystems Paul Eggert @ 2020-02-14 3:29 ` Bruno Haible 2020-02-16 17:24 ` Bruno Haible 2020-02-14 3:46 ` Bruno Haible ` (2 subsequent siblings) 3 siblings, 1 reply; 31+ messages in thread From: Bruno Haible @ 2020-02-14 3:29 UTC (permalink / raw) To: bug-gnulib; +Cc: Paul Eggert The original thread's subject "XFS reports lchmod failure, but changes file system contents" gave the impression that it's somehow XFS specific. But what I observe is that the behaviour is the same on all major file systems on Linux. Tested on Linux 5.3.7 (fc31), with the trivial program below, on - ext4 - btrfs - jfs - reiserfs - xfs Before your latest patch: mode changed, but error "Operation not supported". After your latest patch: mode not changed, error "Operation not supported". Bruno ============================ lchmod.c ============================ #include <config.h> #include <stdio.h> #include <stdlib.h> #include <sys/stat.h> int main (int argc, char *argv[]) { int mode = strtol (argv[1], NULL, 8); char *path = argv[2]; int ret = lchmod (path, mode); if (ret < 0) perror ("lchmod"); } ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-14 3:29 ` Bruno Haible @ 2020-02-16 17:24 ` Bruno Haible 2020-02-16 18:31 ` Paul Eggert 0 siblings, 1 reply; 31+ messages in thread From: Bruno Haible @ 2020-02-16 17:24 UTC (permalink / raw) To: bug-gnulib; +Cc: Paul Eggert I wrote: > But what I observe is that the behaviour is the same on all major file > systems on Linux. > > Tested on Linux 5.3.7 (fc31), with the trivial program below, on > - ext4 > - btrfs > - jfs > - reiserfs > - xfs This are different when using the code which goes through the /proc file system: =============================================================================== #define _GNU_SOURCE 1 #include <fcntl.h> #include <stdio.h> #include <stdlib.h> #include <sys/stat.h> int main (int argc, char *argv[]) { int mode = strtol (argv[1], NULL, 8); char* path = argv[2]; int ret; int fd = openat (AT_FDCWD, path, O_PATH | O_NOFOLLOW | O_CLOEXEC); if (fd < 0) ret = -1; else { char buf[100]; sprintf (buf, "/proc/self/fd/%d", fd); ret = chmod (buf, mode); } if (ret < 0) perror ("lchmod"); } =============================================================================== In this case: - On ext4, btrfs, jfs, xfs: mode changed, but error "Operation not supported". - On reiserfs: mode changed, success. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-16 17:24 ` Bruno Haible @ 2020-02-16 18:31 ` Paul Eggert 2020-02-16 18:58 ` Bruno Haible 0 siblings, 1 reply; 31+ messages in thread From: Paul Eggert @ 2020-02-16 18:31 UTC (permalink / raw) To: Bruno Haible; +Cc: bug-gnulib On 2/16/20 9:24 AM, Bruno Haible wrote: > In this case: > - On ext4, btrfs, jfs, xfs: > mode changed, but error "Operation not supported". > - On reiserfs: > mode changed, success. Thanks for checking. I hope all this stuff is worked around by the latest patch. One other possible bug is that the mode could change temporarily and then spontaneously revert later, because the updated mode was cached but not committed to stable storage. Filesystem bugs can be nasty. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-16 18:31 ` Paul Eggert @ 2020-02-16 18:58 ` Bruno Haible 0 siblings, 0 replies; 31+ messages in thread From: Bruno Haible @ 2020-02-16 18:58 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib Hi Paul, > One other possible bug is that the mode could change temporarily and then > spontaneously revert later, because the updated mode was cached but not > committed to stable storage. Filesystem bugs can be nasty. The S_ISLNK check that you put in bypasses this dark/fuzzy area in the kernel. Therefore I won't check against this possible bug in a unit test. Let the kernel people do this. Bruno ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-13 18:42 [PATCH] fchmodat, lchmod: port to buggy Linux filesystems Paul Eggert 2020-02-14 3:29 ` Bruno Haible @ 2020-02-14 3:46 ` Bruno Haible 2020-02-14 21:02 ` Paul Eggert 2020-02-23 1:35 ` [PATCH] fchmodat, lchmod: port to buggy Linux filesystems Bruno Haible 2020-03-09 17:30 ` Pádraig Brady 3 siblings, 1 reply; 31+ messages in thread From: Bruno Haible @ 2020-02-14 3:46 UTC (permalink / raw) To: bug-gnulib; +Cc: Paul Eggert Hi Paul, > + fchmodat, lchmod: port to buggy Linux filesystems > + Problem reported by Florian Weimer in: > + https://www.sourceware.org/ml/libc-alpha/2020-02/msg00534.html > + * lib/fchmodat.c (fchmodat): > + * lib/lchmod.c (lchmod): > + Don’t assume that chmod on the O_PATH-opened fd will do > + the right thing on a symbolic link. > + * lib/fchmodat.c (fchmodat): > + Don’t attempt to special-case > + any flag value other than AT_SYMLINK_NOFOLLOW. What about OSes other than Linux? 1) The layout of the /proc file system is specific to Linux. It looks different on BSD and other systems. Therefore the code block that makes assumptions about /proc ought to be guarded with 'defined __linux__', no? Here is a proposed patch: diff --git a/lib/fchmodat.c b/lib/fchmodat.c index 02e2da9..180b95d 100644 --- a/lib/fchmodat.c +++ b/lib/fchmodat.c @@ -67,7 +67,7 @@ fchmodat (int dir, char const *file, mode_t mode, int flags) { struct stat st; -# if defined O_PATH && defined AT_EMPTY_PATH +# if defined O_PATH && defined AT_EMPTY_PATH && defined __linux__ int fd = openat (dir, file, O_PATH | O_NOFOLLOW | O_CLOEXEC); if (fd < 0) return fd; diff --git a/lib/lchmod.c b/lib/lchmod.c index c7191c0..6802774 100644 --- a/lib/lchmod.c +++ b/lib/lchmod.c @@ -37,7 +37,7 @@ lchmod (char const *file, mode_t mode) #if HAVE_FCHMODAT return fchmodat (AT_FDCWD, file, mode, AT_SYMLINK_NOFOLLOW); #else -# if defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH +# if defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH && defined __linux__ int fd = openat (AT_FDCWD, file, O_PATH | O_NOFOLLOW | O_CLOEXEC); if (fd < 0) return fd; 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. Bruno ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-14 3:46 ` Bruno Haible @ 2020-02-14 21:02 ` Paul Eggert 2020-02-16 21:38 ` Bruno Haible 0 siblings, 1 reply; 31+ messages in thread From: Paul Eggert @ 2020-02-14 21:02 UTC (permalink / raw) To: Bruno Haible, bug-gnulib On 2/13/20 7:46 PM, Bruno Haible wrote: > Here is a proposed patch: Thanks, looks good. > 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. 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-14 21:02 ` Paul Eggert @ 2020-02-16 21:38 ` Bruno Haible 2020-02-16 22:28 ` Bruno Haible 2020-02-22 23:46 ` Bruno Haible 0 siblings, 2 replies; 31+ messages in thread From: Bruno Haible @ 2020-02-16 21:38 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib [-- Attachment #1: Type: text/plain, Size: 3160 bytes --] 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 <bruno@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. [-- Attachment #2: 0001-lchmod-Fix-buggy-override-on-macOS-HP-UX-regression-.patch --] [-- Type: text/x-patch, Size: 1183 bytes --] From 4efc18c4a2927074bf0653ba8bd79230661b8238 Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@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 <bruno@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 <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/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 [-- Attachment #3: 0002-lchmod-Make-more-future-proof.patch --] [-- Type: text/x-patch, Size: 6102 bytes --] From ea91b32b0719d46354c1d995c1d1b1c36842a2eb Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@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 <bruno@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 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-16 21:38 ` Bruno Haible @ 2020-02-16 22:28 ` Bruno Haible 2020-02-22 23:46 ` Bruno Haible 1 sibling, 0 replies; 31+ messages in thread From: Bruno Haible @ 2020-02-16 22:28 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib [-- Attachment #1: Type: text/plain, Size: 1323 bytes --] > > Perhaps lchmod.m4 and fchmodat.m4 should define a symbol > > HAVE_LCHMOD_BUG_WITH_NON_SYMLINKS and the C code could refer to that. > 2020-02-16 Bruno Haible <bruno@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. And here's a similar change for 'fchmodat'. It has one or two code branches that currently are dead code, but this is on purpose: this is code that will becomes active when either the NEED_FCHMODAT_NONSYMLINK_FIX test will trigger on more platforms, or some more autoconf tests will be added to fchmodat.m4. 2020-02-16 Bruno Haible <bruno@clisp.org> fchmodat: Make more future-proof. * m4/fchmodat.m4 (gl_FUNC_FCHMODAT): Define NEED_FCHMODAT_NONSYMLINK_FIX. (gl_PREREQ_FCHMODAT): New macro. * lib/fchmodat.c (fchmodat): Test NEED_FCHMODAT_NONSYMLINK_FIX. Access /proc only on Linux. Return EOPNOTSUPP only on Linux and on platforms without lchmod function. * modules/fchmodat (configure.ac): Invoke gl_PREREQ_FCHMODAT. [-- Attachment #2: 0001-fchmodat-Make-more-future-proof.patch --] [-- Type: text/x-patch, Size: 5635 bytes --] From f4693b0166bab83ab232dcd3cfd95906411d1110 Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@clisp.org> Date: Sun, 16 Feb 2020 23:01:08 +0100 Subject: [PATCH] fchmodat: Make more future-proof. * m4/fchmodat.m4 (gl_FUNC_FCHMODAT): Define NEED_FCHMODAT_NONSYMLINK_FIX. (gl_PREREQ_FCHMODAT): New macro. * lib/fchmodat.c (fchmodat): Test NEED_FCHMODAT_NONSYMLINK_FIX. Access /proc only on Linux. Return EOPNOTSUPP only on Linux and on platforms without lchmod function. * modules/fchmodat (configure.ac): Invoke gl_PREREQ_FCHMODAT. --- ChangeLog | 11 +++++++++++ lib/fchmodat.c | 30 +++++++++++++++++++++++------- m4/fchmodat.m4 | 15 +++++++++++++-- modules/fchmodat | 1 + 4 files changed, 48 insertions(+), 9 deletions(-) diff --git a/ChangeLog b/ChangeLog index 30cc425..8215147 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,16 @@ 2020-02-16 Bruno Haible <bruno@clisp.org> + fchmodat: Make more future-proof. + * m4/fchmodat.m4 (gl_FUNC_FCHMODAT): Define + NEED_FCHMODAT_NONSYMLINK_FIX. + (gl_PREREQ_FCHMODAT): New macro. + * lib/fchmodat.c (fchmodat): Test NEED_FCHMODAT_NONSYMLINK_FIX. Access + /proc only on Linux. Return EOPNOTSUPP only on Linux and on platforms + without lchmod function. + * modules/fchmodat (configure.ac): Invoke gl_PREREQ_FCHMODAT. + +2020-02-16 Bruno Haible <bruno@clisp.org> + lchmod: Make more future-proof. * m4/lchmod.m4 (gl_FUNC_LCHMOD): Define NEED_LCHMOD_NONSYMLINK_FIX. (gl_PREREQ_LCHMOD): New macro. diff --git a/lib/fchmodat.c b/lib/fchmodat.c index 02e2da9..bb48b44 100644 --- a/lib/fchmodat.c +++ b/lib/fchmodat.c @@ -14,7 +14,7 @@ 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 Jim Meyering */ +/* written by Jim Meyering and Paul Eggert */ /* 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 @@ -63,16 +63,27 @@ orig_fchmodat (int dir, char const *file, mode_t mode, int flags) int fchmodat (int dir, char const *file, mode_t mode, int flags) { +# if NEED_FCHMODAT_NONSYMLINK_FIX if (flags == AT_SYMLINK_NOFOLLOW) { struct stat st; -# if defined O_PATH && defined AT_EMPTY_PATH +# if 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 (dir, 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. */ if (fstatat (fd, "", &st, AT_EMPTY_PATH) != 0) { @@ -102,7 +113,9 @@ fchmodat (int dir, char const *file, mode_t mode, int flags) return chmod_result; } /* /proc is not mounted. */ -# else + /* Fall back on orig_fchmodat, despite the race. */ + return orig_fchmodat (dir, file, mode, 0); +# elif (defined __linux__ || defined __ANDROID__) || !HAVE_LCHMOD int fstatat_result = fstatat (dir, file, &st, AT_SYMLINK_NOFOLLOW); if (fstatat_result != 0) return fstatat_result; @@ -111,10 +124,13 @@ fchmodat (int dir, char const *file, mode_t mode, int flags) errno = EOPNOTSUPP; return -1; } -# endif - /* Fall back on chmod, despite the race. */ - flags = 0; + /* Fall back on orig_fchmodat, despite the race. */ + return orig_fchmodat (dir, file, mode, 0); +# else + return orig_fchmodat (dir, file, mode, 0); +# endif } +# endif return orig_fchmodat (dir, file, mode, flags); } diff --git a/m4/fchmodat.m4 b/m4/fchmodat.m4 index f284485..e3f2f04 100644 --- a/m4/fchmodat.m4 +++ b/m4/fchmodat.m4 @@ -1,4 +1,4 @@ -# fchmodat.m4 serial 3 +# fchmodat.m4 serial 4 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, @@ -65,7 +65,18 @@ AC_DEFUN([gl_FUNC_FCHMODAT], rm -f conftest.fchmodat]) case $gl_cv_func_fchmodat_works in *yes) ;; - *) REPLACE_FCHMODAT=1;; + *) + AC_DEFINE([NEED_FCHMODAT_NONSYMLINK_FIX], [1], + [Define to 1 if fchmodat+AT_SYMLINK_NOFOLLOW does not work right on non-symlinks.]) + REPLACE_FCHMODAT=1 + ;; esac fi ]) + +# Prerequisites of lib/fchmodat.c. +AC_DEFUN([gl_PREREQ_FCHMODAT], +[ + AC_CHECK_FUNCS_ONCE([lchmod]) + : +]) diff --git a/modules/fchmodat b/modules/fchmodat index b0f06e8..f0dac9d 100644 --- a/modules/fchmodat +++ b/modules/fchmodat @@ -28,6 +28,7 @@ configure.ac: gl_FUNC_FCHMODAT if test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1; then AC_LIBOBJ([fchmodat]) + gl_PREREQ_FCHMODAT fi gl_MODULE_INDICATOR([fchmodat]) dnl for lib/openat.h gl_SYS_STAT_MODULE_INDICATOR([fchmodat]) -- 2.7.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-16 21:38 ` Bruno Haible 2020-02-16 22:28 ` Bruno Haible @ 2020-02-22 23:46 ` Bruno Haible 2020-02-23 8:15 ` Paul Eggert 1 sibling, 1 reply; 31+ messages in thread From: Bruno Haible @ 2020-02-22 23:46 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib > 2020-02-16 Bruno Haible <bruno@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. Oops, this change is broken: It causes a link error on Solaris 10: cc -xarch=generic64 -O -D_STDC_C99= -g -L/home/haible/prefix-x86_64/lib -o test-areadlinkat test-areadlinkat.o libtests.a ../gllib/libgnu.a libtests.a Undefined first referenced symbol in file orig_lchmod ../gllib/libgnu.a(lchmod.o) ld: fatal: symbol referencing errors. No output written to test-areadlinkat *** Error code 1 This patch fixes it. 2020-02-22 Bruno Haible <bruno@clisp.org> lchmod: Fix link error on Solaris 10 (regression from 2020-02-16). * lib/lchmod.c (lchmod): Use the code with lstat and chmod also when NEED_LCHMOD_NONSYMLINK_FIX is not defined. diff --git a/lib/lchmod.c b/lib/lchmod.c index 57f75da..e62111c 100644 --- a/lib/lchmod.c +++ b/lib/lchmod.c @@ -64,9 +64,9 @@ lchmod (char const *file, mode_t mode) /* 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 -# if defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH \ - && (defined __linux__ || defined __ANDROID__) +#elif NEED_LCHMOD_NONSYMLINK_FIX \ + && defined AT_FDCWD && defined O_PATH && defined AT_EMPTY_PATH \ + && (defined __linux__ || defined __ANDROID__) /* newer Linux */ /* 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. @@ -113,8 +113,9 @@ lchmod (char const *file, mode_t mode) /* /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 +#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) @@ -126,13 +127,10 @@ lchmod (char const *file, mode_t mode) } /* Fall back on chmod, despite the race. */ return chmod (file, mode); -# else /* GNU/kFreeBSD, GNU/Hurd, macOS, FreeBSD, NetBSD, HP-UX */ +# 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); +#else /* native Windows */ + return chmod (file, mode); #endif } ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-22 23:46 ` Bruno Haible @ 2020-02-23 8:15 ` Paul Eggert 2020-02-23 10:58 ` Bruno Haible 0 siblings, 1 reply; 31+ messages in thread From: Paul Eggert @ 2020-02-23 8:15 UTC (permalink / raw) To: Bruno Haible; +Cc: bug-gnulib [-- 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 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-23 8:15 ` Paul Eggert @ 2020-02-23 10:58 ` Bruno Haible 2020-02-23 23:56 ` Paul Eggert 0 siblings, 1 reply; 31+ messages in thread From: Bruno Haible @ 2020-02-23 10:58 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib Hi Paul, > found that I had trouble reading the code That's probably because I prefer the "one code for each platform" approach when suitable - because when debugging problems I don't like to rely to $CC ... -E file.c > file.i to tell me which code is actually enabled -, whereas you seem to prefer a "global minimum of lines" approach. It's understandable that I find your code hard to understand and vice versa. I'm not saying that one is better than the other; just trying to explain why we find each other's code odd here. > 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.... The fchmodat part looks right. On the other hand, the lchmod part looks wrong: You stripped away the test $REPLACE_LCHMOD = 1 case. However, the file doc/glibc-functions/lchmod.texi still says Portability problems fixed by Gnulib: @itemize ... @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. Overriding the system's lchmod function requires the case REPLACE_LCHMOD=1. Bruno ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-23 10:58 ` Bruno Haible @ 2020-02-23 23:56 ` Paul Eggert 2020-02-24 2:27 ` overriding glibc stub functions Bruno Haible 0 siblings, 1 reply; 31+ messages in thread From: Paul Eggert @ 2020-02-23 23:56 UTC (permalink / raw) To: Bruno Haible; +Cc: bug-gnulib On 2/23/20 2:58 AM, Bruno Haible wrote: > the file doc/glibc-functions/lchmod.texi still says > > ... > 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. > > Overriding the system's lchmod function requires the case REPLACE_LCHMOD=1. Hmm, why? 'configure' says that GNU/Linux functions that always fail with errno==ENOSYS do not exist, i.e., 'configure' sets HAVE_LCHMOD=0 on GNU/Linux. This convention is used often by Autoconf and Gnulib. And if HAVE_LCHMOD=0 why would we want REPLACE_LCHMOD=1? ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: overriding glibc stub functions 2020-02-23 23:56 ` Paul Eggert @ 2020-02-24 2:27 ` Bruno Haible 2020-02-24 7:44 ` Paul Eggert 0 siblings, 1 reply; 31+ messages in thread From: Bruno Haible @ 2020-02-24 2:27 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib Paul Eggert wrote: > On 2/23/20 2:58 AM, Bruno Haible wrote: > > the file doc/glibc-functions/lchmod.texi still says > > > > ... > > 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. > > > > Overriding the system's lchmod function requires the case REPLACE_LCHMOD=1. > > Hmm, why? 'configure' says that GNU/Linux functions that always fail with > errno==ENOSYS do not exist, i.e., 'configure' sets HAVE_LCHMOD=0 on GNU/Linux. > This convention is used often by Autoconf and Gnulib. And if HAVE_LCHMOD=0 why > would we want REPLACE_LCHMOD=1? Ouch. I had completely forgotten about this. Yes, lchmod is defined as a stub in glibc/Linux: $ nm --dynamic /lib/x86_64-linux-gnu/libc.so.6 | grep lchmod 00000000000f6f40 T lchmod $ grep lchmod /usr/include/x86_64-linux-gnu/gnu/stubs-64.h #define __stub_lchmod And, as you say, this leads to HAVE_LCHMOD=0. But why do we to distinguish 'lchmod' and 'rpl_lchmod' when we are overriding lchmod? For three reasons: 1) So that there is no conflict when linking statically. 2) So that the behaviour is reliable, and does not break when '-lc' occurs in the link command line (or as dependency of other shared libraries on the link command line). 3) So that when debugging with gdb, you know where you are. Test case for 1): $ gcc -Wall -O2 -c main.c $ gcc -Wall -O2 -c lib.c $ gcc -static main.c lib.c -o main1 $ gcc -static main.c lib.c -lc -o main2 $ gcc -static main.c -lc lib.c -o main3 /tmp/ccPmrK5B.o: In function `main': main.c:(.text+0x1a): warning: lchmod is not implemented and will always fail /tmp/ccJXO5KR.o: In function `lchmod': lib.c:(.text+0x0): multiple definition of `lchmod' /usr/lib/gcc/x86_64-linux-gnu/5/../../../x86_64-linux-gnu/libc.a(lchmod.o):(.text+0x0): first defined here collect2: error: ld returned 1 exit status Test case for 2): $ gcc -Wall -O2 -c main.c $ gcc -Wall -O2 -fPIC -c lib.c $ gcc -shared -o libfoo.so lib.o $ gcc main.c -o main1 $ gcc main.c -L. -lfoo -o main2 $ gcc main.c -L. -lc -lfoo -o main3 $ touch foo $ ./main1 => lchmod -> -1 $ LD_LIBRARY_PATH=. ./main2 => lchmod -> 0 $ LD_LIBRARY_PATH=. ./main3 => lchmod -> -1 (main.c and lib.c are attached below.) 3) is relatively unimportant, but * 1) is important if we don't want to receive bug reports from people who do embedded systems (OpenWRT and such), and * 2) is important for projects that use gnulib in shared libraries (from gettext to guile) and don't use the [expensive] renaming of symbols in the shared library. (For example, in libunistring, lchmod would be renamed to libunistring_lchmod, thus eliminating the conflict. But not all libraries do this.) Therefore I think we need to do two things: * For those functions that may be stubs in glibc, define HAVE_FUNC to 1 instead of 0 if glibc implements it as a stub. * Reinstall (at least partially) the code for REPLACE_LCHMOD=1. Bruno =================================== main.c =================================== #include <stdio.h> #include <sys/stat.h> int main (int argc, char *argv[]) { int ret = lchmod ("foo", 0444); printf ("lchmod -> %d\n", ret); } =================================== lib.c =================================== #include <sys/stat.h> /* This function is intended to override the libc function. */ int lchmod (const char *file, mode_t mode) { return chmod (file, mode); } ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: overriding glibc stub functions 2020-02-24 2:27 ` overriding glibc stub functions Bruno Haible @ 2020-02-24 7:44 ` Paul Eggert 0 siblings, 0 replies; 31+ messages in thread From: Paul Eggert @ 2020-02-24 7:44 UTC (permalink / raw) To: Bruno Haible; +Cc: bug-gnulib On 2/23/20 6:27 PM, Bruno Haible wrote: > Therefore I think we need to do two things: > > * For those functions that may be stubs in glibc, define HAVE_FUNC to 1 > instead of 0 if glibc implements it as a stub. This sounds like it could lead to trouble, as it runs afoul of longstanding Autoconf tradition. Even within Gnulib, we'd have to investigate and probably rewrite code using HAVE_LCHMOD, HAVE_REVOKE, and perhaps other such symbols. And we'd also have to warn all downstream users that the HAVE_* tradition is changing (at least for glibc symbols), and downstream users would need to change their own code that uses HAVE_LCHMOD or whatever. And what about downstream code that uses AC_CHECK_FUNCS([lchmod]) or AC_CHECK_FUNCS([sigreturn]) already, and gets the longstanding Autoconf interpretation for symbols it checks directly? Won't this make things too confusing? Perhaps it would be better to use HAVE_LCHMOD=0 and REPLACE_LCHMOD=1 in this problematic situation, i.e., to bring back the REPLACE_LCHMOD code without altering HAVE_LCHMOD. This would be less likely to break other uses, because it would not conflict with the Autoconf tradition. > * Reinstall (at least partially) the code for REPLACE_LCHMOD=1. Yes, it sounds like we'll need to do that to support static linking and -lc. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-13 18:42 [PATCH] fchmodat, lchmod: port to buggy Linux filesystems Paul Eggert 2020-02-14 3:29 ` Bruno Haible 2020-02-14 3:46 ` Bruno Haible @ 2020-02-23 1:35 ` Bruno Haible 2020-02-23 7:22 ` Paul Eggert 2020-03-09 17:30 ` Pádraig Brady 3 siblings, 1 reply; 31+ messages in thread From: Bruno Haible @ 2020-02-23 1:35 UTC (permalink / raw) To: bug-gnulib; +Cc: Paul Eggert Hi Paul, > + if (S_ISLNK (st.st_mode)) > + { > + close (fd); > + errno = EOPNOTSUPP; > + return -1; > + } Why EOPNOTSUPP? Why not ENOTSUP? On Solaris, gnulib's lchmod on a symbolic link produces lchmod: Operation not supported on transport endpoint "transport endpoint" is misleading. Likewise, in POSIX [1] EOPNOTSUPP means "Operation not supported on socket". I would find ENOTSUP a better choice. Bruno [1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-23 1:35 ` [PATCH] fchmodat, lchmod: port to buggy Linux filesystems Bruno Haible @ 2020-02-23 7:22 ` Paul Eggert 0 siblings, 0 replies; 31+ messages in thread From: Paul Eggert @ 2020-02-23 7:22 UTC (permalink / raw) To: Bruno Haible; +Cc: bug-gnulib On 2/22/20 5:35 PM, Bruno Haible wrote: > Why EOPNOTSUPP? Why not ENOTSUP? glibc will use EOPNOTSUPP, because POSIX says fchmodat with AT_SYMLINK_NOFOLLOW uses EOPNOTSUPP in this situation. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-02-13 18:42 [PATCH] fchmodat, lchmod: port to buggy Linux filesystems Paul Eggert ` (2 preceding siblings ...) 2020-02-23 1:35 ` [PATCH] fchmodat, lchmod: port to buggy Linux filesystems Bruno Haible @ 2020-03-09 17:30 ` Pádraig Brady 2020-03-09 18:51 ` Paul Eggert 3 siblings, 1 reply; 31+ messages in thread From: Pádraig Brady @ 2020-03-09 17:30 UTC (permalink / raw) To: Paul Eggert, bug-gnulib, Florian Weimer, Kamil Dudka On 13/02/2020 18:42, Paul Eggert wrote: > Problem reported by Florian Weimer in: > https://www.sourceware.org/ml/libc-alpha/2020-02/msg00534.html A very similar "ENOTSUP" problem is being reported with coreutils-8.32 with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora rawhide in a chroot. https://bugzilla.redhat.com/1811038 BTW I see a possible small issue in lchmod.c in this code: + if (chmod_errno != ENOENT) + { + errno = chmod_errno; + return chmod_result; + } Shouldn't that also check for other lookup errors like ENOTDIR and ELOOP ? Also mknod() in coreutils is calling lchmod(). Shouldn't it be just calling chmod() as if mknod() was passed a symlink, then it would have already failed with EEXIST? cheers, Pádraig ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-09 17:30 ` Pádraig Brady @ 2020-03-09 18:51 ` Paul Eggert 2020-03-09 23:45 ` Pádraig Brady 0 siblings, 1 reply; 31+ messages in thread From: Paul Eggert @ 2020-03-09 18:51 UTC (permalink / raw) To: Pádraig Brady, bug-gnulib, Florian Weimer, Kamil Dudka On 3/9/20 10:30 AM, Pádraig Brady wrote: > A very similar "ENOTSUP" problem is being reported with coreutils-8.32 > with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora > rawhide in a chroot. > https://bugzilla.redhat.com/1811038 I don't understand that bug report. The strace diff you mentioned in Comment 4 looks like the new mknod command is working. And yet the original bug report says new mknod command is complaining "Operation not supported". Is the problem that some filesystems don't work with the chmod /proc/self/fd/NNN trick, and that it worked for you (the strace diff) but not for Mohan (the original bug report)? > BTW I see a possible small issue in lchmod.c in this code: > > + if (chmod_errno != ENOENT) > + { > + errno = chmod_errno; > + return chmod_result; > + } > > Shouldn't that also check for other lookup errors like ENOTDIR and ELOOP ? If /proc is mounted you can't get those lookup errors. If /proc is not mounted then there shouldn't be anything other than an empty directory at /proc, in which case ENOENT is what you'll get. It is true that if you put something at /proc other than an empty directory you could get arbitrary errno values, but we don't support that sort of misconfiguration. > Also mknod() in coreutils is calling lchmod(). > Shouldn't it be just calling chmod() as if mknod() was passed a > symlink, then it would have already failed with EEXIST? That would still be vulnerable to a race if someone else replaces the newly-created special file with a symlink between the time we call mknode and chmod. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-09 18:51 ` Paul Eggert @ 2020-03-09 23:45 ` Pádraig Brady 2020-03-10 11:52 ` Florian Weimer 0 siblings, 1 reply; 31+ messages in thread From: Pádraig Brady @ 2020-03-09 23:45 UTC (permalink / raw) To: Paul Eggert, bug-gnulib, Florian Weimer, Kamil Dudka On 09/03/2020 18:51, Paul Eggert wrote: > On 3/9/20 10:30 AM, Pádraig Brady wrote: > >> A very similar "ENOTSUP" problem is being reported with coreutils-8.32 >> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora >> rawhide in a chroot. >> https://bugzilla.redhat.com/1811038 > > I don't understand that bug report. The strace diff you mentioned in > Comment 4 looks like the new mknod command is working. And yet the > original bug report says new mknod command is complaining "Operation not > supported". > > Is the problem that some filesystems don't work with the chmod > /proc/self/fd/NNN trick, and that it worked for you (the strace diff) > but not for Mohan (the original bug report)? Right, the strace is from my working mknod(1) to show the differences between 8.31 and 8.32. I've requested an strace from the failing system. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-09 23:45 ` Pádraig Brady @ 2020-03-10 11:52 ` Florian Weimer 2020-03-10 15:09 ` Kamil Dudka 2020-03-10 19:27 ` Pádraig Brady 0 siblings, 2 replies; 31+ messages in thread From: Florian Weimer @ 2020-03-10 11:52 UTC (permalink / raw) To: Pádraig Brady; +Cc: Kamil Dudka, Paul Eggert, bug-gnulib * Pádraig Brady: > On 09/03/2020 18:51, Paul Eggert wrote: >> On 3/9/20 10:30 AM, Pádraig Brady wrote: >> >>> A very similar "ENOTSUP" problem is being reported with coreutils-8.32 >>> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora >>> rawhide in a chroot. >>> https://bugzilla.redhat.com/1811038 >> >> I don't understand that bug report. The strace diff you mentioned in >> Comment 4 looks like the new mknod command is working. And yet the >> original bug report says new mknod command is complaining "Operation not >> supported". >> >> Is the problem that some filesystems don't work with the chmod >> /proc/self/fd/NNN trick, and that it worked for you (the strace diff) >> but not for Mohan (the original bug report)? > > Right, the strace is from my working mknod(1) > to show the differences between 8.31 and 8.32. > > I've requested an strace from the failing system. I guess it's possible that just isn't mounted at this point. The glibc implementation will definitely *not* add racy fallback in case /proc is not available, so this will not work until someone implements the required system call. It's not clear to my why the mknod command would need fchmodat at all. With the -m argument, it should simply set the umask to 0, and pass the mode bits to the mknod function. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-10 11:52 ` Florian Weimer @ 2020-03-10 15:09 ` Kamil Dudka 2020-03-10 19:27 ` Pádraig Brady 1 sibling, 0 replies; 31+ messages in thread From: Kamil Dudka @ 2020-03-10 15:09 UTC (permalink / raw) To: Florian Weimer; +Cc: bug-gnulib, Paul Eggert On Tuesday, March 10, 2020 12:52:14 PM CET Florian Weimer wrote: > * Pádraig Brady: > > I've requested an strace from the failing system. The strace in the failing case looks like this: [...] umask(000) = 022 umask(022) = 000 mknod("/dev/random", S_IFCHR|0666, makedev(0x1, 0x8)) = 0 openat(AT_FDCWD, "/dev/random", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3 newfstatat(3, "", {st_mode=S_IFCHR|0644, st_rdev=makedev(0x1, 0x8), ...}, AT_EMPTY_PATH) = 0 chmod("/proc/self/fd/3", 0666) = -1 ENOENT (No such file or directory) close(3) = 0 write(2, "/bin/mknod: ", 12/bin/mknod: ) = 12 write(2, "cannot set permissions of '/dev/"..., 39cannot set permissions of '/dev/random') = 39 write(2, ": Operation not supported", 25: Operation not supported) = 25 write(2, "\n", 1 ) = 1 close(1) = 0 close(2) = 0 exit_group(1) = ? See https://bugzilla.redhat.com/1811038#c11 for a minimal example. > I guess it's possible that just isn't mounted at this point. Yes, this happens when /proc is not mounted. The problem is that mknod used to succeed in this case with the previous version of coreutils (and gnulib). > The glibc implementation will definitely *not* add racy fallback in > case /proc is not available, so this will not work until someone > implements the required system call. > > It's not clear to my why the mknod command would need fchmodat at all. > With the -m argument, it should simply set the umask to 0, and pass > the mode bits to the mknod function. The call in question was introduced by the following commit: https://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.21-51-ge7198a67b But the referred bug report does not seem to mention mknod at all: https://debbugs.gnu.org/14371 Kamil ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-10 11:52 ` Florian Weimer 2020-03-10 15:09 ` Kamil Dudka @ 2020-03-10 19:27 ` Pádraig Brady 2020-03-10 19:30 ` Florian Weimer 1 sibling, 1 reply; 31+ messages in thread From: Pádraig Brady @ 2020-03-10 19:27 UTC (permalink / raw) To: Florian Weimer; +Cc: Kamil Dudka, Paul Eggert, bug-gnulib On 10/03/2020 11:52, Florian Weimer wrote: > * Pádraig Brady: > >> On 09/03/2020 18:51, Paul Eggert wrote: >>> On 3/9/20 10:30 AM, Pádraig Brady wrote: >>> >>>> A very similar "ENOTSUP" problem is being reported with coreutils-8.32 >>>> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora >>>> rawhide in a chroot. >>>> https://bugzilla.redhat.com/1811038 >>> >>> I don't understand that bug report. The strace diff you mentioned in >>> Comment 4 looks like the new mknod command is working. And yet the >>> original bug report says new mknod command is complaining "Operation not >>> supported". >>> >>> Is the problem that some filesystems don't work with the chmod >>> /proc/self/fd/NNN trick, and that it worked for you (the strace diff) >>> but not for Mohan (the original bug report)? >> >> Right, the strace is from my working mknod(1) >> to show the differences between 8.31 and 8.32. >> >> I've requested an strace from the failing system. > > I guess it's possible that just isn't mounted at this point. > > The glibc implementation will definitely *not* add racy fallback in > case /proc is not available, so this will not work until someone > implements the required system call. > > It's not clear to my why the mknod command would need fchmodat at all. > With the -m argument, it should simply set the umask to 0, and pass > the mode bits to the mknod function. umask is not used so as to cater for discrepancies between process and default ACL masks: https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.21-51-ge7198a67b An update re this issue. The strace was supplied in https://bugzilla.redhat.com/1811038 which shows there is no fallback to chmod() in lchmod(). Now the gnulib code does fallback so this issue must be in the glibc implementation. I also downloaded the problematic rpm, and used nm on the mknod binary to confirm is was calling out to the glibc implementation. cheers, Pádraig ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-10 19:27 ` Pádraig Brady @ 2020-03-10 19:30 ` Florian Weimer 2020-03-11 8:03 ` Paul Eggert ` (2 more replies) 0 siblings, 3 replies; 31+ messages in thread From: Florian Weimer @ 2020-03-10 19:30 UTC (permalink / raw) To: Pádraig Brady; +Cc: Kamil Dudka, Paul Eggert, bug-gnulib * Pádraig Brady: > On 10/03/2020 11:52, Florian Weimer wrote: >> * Pádraig Brady: >> >>> On 09/03/2020 18:51, Paul Eggert wrote: >>>> On 3/9/20 10:30 AM, Pádraig Brady wrote: >>>> >>>>> A very similar "ENOTSUP" problem is being reported with coreutils-8.32 >>>>> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora >>>>> rawhide in a chroot. >>>>> https://bugzilla.redhat.com/1811038 >>>> >>>> I don't understand that bug report. The strace diff you mentioned in >>>> Comment 4 looks like the new mknod command is working. And yet the >>>> original bug report says new mknod command is complaining "Operation not >>>> supported". >>>> >>>> Is the problem that some filesystems don't work with the chmod >>>> /proc/self/fd/NNN trick, and that it worked for you (the strace diff) >>>> but not for Mohan (the original bug report)? >>> >>> Right, the strace is from my working mknod(1) >>> to show the differences between 8.31 and 8.32. >>> >>> I've requested an strace from the failing system. >> >> I guess it's possible that just isn't mounted at this point. >> >> The glibc implementation will definitely *not* add racy fallback in >> case /proc is not available, so this will not work until someone >> implements the required system call. >> >> It's not clear to my why the mknod command would need fchmodat at all. >> With the -m argument, it should simply set the umask to 0, and pass >> the mode bits to the mknod function. > > umask is not used so as to cater for discrepancies between process and default ACL masks: > https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.21-51-ge7198a67b I just don't understand this explanation. Is the concern here that you would get a different mode from the requested one if you use umask+mknod and not mknod+some form of chmod? > An update re this issue. > The strace was supplied in https://bugzilla.redhat.com/1811038 > which shows there is no fallback to chmod() in lchmod(). > Now the gnulib code does fallback so this issue must be in the glibc implementation. The glibc implementation needs /proc to avoid the race. There is no way around that, otherwise we introduce a security vulnerability. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-10 19:30 ` Florian Weimer @ 2020-03-11 8:03 ` Paul Eggert 2020-03-11 8:45 ` Florian Weimer 2020-03-11 8:25 ` Paul Eggert 2020-03-11 9:04 ` Kamil Dudka 2 siblings, 1 reply; 31+ messages in thread From: Paul Eggert @ 2020-03-11 8:03 UTC (permalink / raw) To: Florian Weimer, Pádraig Brady; +Cc: Kamil Dudka, bug-gnulib On 3/10/20 12:30 PM, Florian Weimer wrote: > The glibc implementation needs /proc to avoid the race. There is no > way around that, otherwise we introduce a security vulnerability. It is unfortunate that we have dueling paranoia here. coreutils mknod is paranoid so it uses lchmod to avoid a race, and then glibc lchmod is paranoid so it refuses to work with lchmod unless /proc is mounted. Since we apparently cannot avoid a race unless /proc is mounted, I suppose we could change gnulib lchmod to consider the current glibc behavior to be a bug, and to fall back on lstat+chmod when /proc is not mounted. This would fix coreutils and every other Gnulib-using program that uses lchmod or fchmodat. But on the whole it would be somewhat cleaner if glibc lchmod and fchmodat were merely documented to have races when /proc is not mounted; that'd be simpler than manually adjusting all programs that use glibc lchmod so that they all explicitly have races when /proc is not mounted. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-11 8:03 ` Paul Eggert @ 2020-03-11 8:45 ` Florian Weimer 2020-03-11 16:04 ` Paul Eggert 0 siblings, 1 reply; 31+ messages in thread From: Florian Weimer @ 2020-03-11 8:45 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib, Kamil Dudka * Paul Eggert: > On 3/10/20 12:30 PM, Florian Weimer wrote: >> The glibc implementation needs /proc to avoid the race. There is no >> way around that, otherwise we introduce a security vulnerability. > > It is unfortunate that we have dueling paranoia here. coreutils mknod is > paranoid so it uses lchmod to avoid a race, and then glibc lchmod is paranoid so > it refuses to work with lchmod unless /proc is mounted. I now wonder if neither gnulib nor glibc should pretend that they can implement lchmod and fchmodat on Linux in a usable fashion. I added the emulation to glibc mostly because it was in gnulib. Otherwise, I would have insisted that a proper system call be implemented first. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-11 8:45 ` Florian Weimer @ 2020-03-11 16:04 ` Paul Eggert 0 siblings, 0 replies; 31+ messages in thread From: Paul Eggert @ 2020-03-11 16:04 UTC (permalink / raw) To: Florian Weimer; +Cc: bug-gnulib, Kamil Dudka On 3/11/20 1:45 AM, Florian Weimer wrote: > I now wonder if neither gnulib nor glibc should pretend that they can > implement lchmod and fchmodat on Linux in a usable fashion. That ship sailed long ago for Gnulib, since it long emulated lchmod by using chmod (with no symlink check!), and Gnulib-using programs rely on this: some know about the lack of a symlink check and therefore have races of there own, whereas others don't know and have more-serious bugs. At the very least we can improve on this by adding a symlink check to Gnulib lchmod. There will still be races but some Gnulib-using programs will be better off than before. Portable programs really need the ability to do lchmod to avoid races, and as lchmod works on non-Linux kernels we don't want to lose this capability. I agree that Linux should have a proper lchmod system call. I am puzzled as to why this wasn't added long ago. I suppose the kernel folks thought that lchmod was nonsense because symlink permissions are nonsense, and missed the point that lchmod avoids races. For glibc I think it'd be better to keep the change to lchmod and fchmodat. Problems occur only when /proc is not mounted, and the problems are the same as before the change so this is not a regression. However, I don't feel strongly about this. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-10 19:30 ` Florian Weimer 2020-03-11 8:03 ` Paul Eggert @ 2020-03-11 8:25 ` Paul Eggert 2020-03-11 8:40 ` Florian Weimer 2020-03-11 9:04 ` Kamil Dudka 2 siblings, 1 reply; 31+ messages in thread From: Paul Eggert @ 2020-03-11 8:25 UTC (permalink / raw) To: Florian Weimer, Pádraig Brady; +Cc: Kamil Dudka, bug-gnulib On 3/10/20 12:30 PM, Florian Weimer wrote: > Is the concern here that > you would get a different mode from the requested one if you use > umask+mknod and not mknod+some form of chmod? Yes. See: https://bugs.gnu.org/14371 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-11 8:25 ` Paul Eggert @ 2020-03-11 8:40 ` Florian Weimer 0 siblings, 0 replies; 31+ messages in thread From: Florian Weimer @ 2020-03-11 8:40 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib, Kamil Dudka * Paul Eggert: > On 3/10/20 12:30 PM, Florian Weimer wrote: > >> Is the concern here that >> you would get a different mode from the requested one if you use >> umask+mknod and not mknod+some form of chmod? > > Yes. See: > > https://bugs.gnu.org/14371 Wrong bug? There is no --mode argument involved in that bug report, as far as I can see. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-10 19:30 ` Florian Weimer 2020-03-11 8:03 ` Paul Eggert 2020-03-11 8:25 ` Paul Eggert @ 2020-03-11 9:04 ` Kamil Dudka 2020-03-11 16:36 ` Paul Eggert 2 siblings, 1 reply; 31+ messages in thread From: Kamil Dudka @ 2020-03-11 9:04 UTC (permalink / raw) To: Florian Weimer; +Cc: Paul Eggert, bug-gnulib On Tuesday, March 10, 2020 8:30:36 PM CET Florian Weimer wrote: > * Pádraig Brady: > > On 10/03/2020 11:52, Florian Weimer wrote: > >> * Pádraig Brady: > >>> On 09/03/2020 18:51, Paul Eggert wrote: > >>>> On 3/9/20 10:30 AM, Pádraig Brady wrote: > >>>>> A very similar "ENOTSUP" problem is being reported with coreutils-8.32 > >>>>> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora > >>>>> rawhide in a chroot. > >>>>> https://bugzilla.redhat.com/1811038 > >>>> > >>>> I don't understand that bug report. The strace diff you mentioned in > >>>> Comment 4 looks like the new mknod command is working. And yet the > >>>> original bug report says new mknod command is complaining "Operation > >>>> not > >>>> supported". > >>>> > >>>> Is the problem that some filesystems don't work with the chmod > >>>> /proc/self/fd/NNN trick, and that it worked for you (the strace diff) > >>>> but not for Mohan (the original bug report)? > >>> > >>> Right, the strace is from my working mknod(1) > >>> to show the differences between 8.31 and 8.32. > >>> > >>> I've requested an strace from the failing system. > >> > >> I guess it's possible that just isn't mounted at this point. > >> > >> The glibc implementation will definitely *not* add racy fallback in > >> case /proc is not available, so this will not work until someone > >> implements the required system call. > >> > >> It's not clear to my why the mknod command would need fchmodat at all. > >> With the -m argument, it should simply set the umask to 0, and pass > >> the mode bits to the mknod function. > > > > umask is not used so as to cater for discrepancies between process and > > default ACL masks: > > https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.21-51-ge > > 7198a67b > I just don't understand this explanation. Is the concern here that > you would get a different mode from the requested one if you use > umask+mknod and not mknod+some form of chmod? > > > An update re this issue. > > The strace was supplied in https://bugzilla.redhat.com/1811038 > > which shows there is no fallback to chmod() in lchmod(). > > Now the gnulib code does fallback so this issue must be in the glibc > > implementation. > The glibc implementation needs /proc to avoid the race. There is no > way around that, otherwise we introduce a security vulnerability. As I understand it, the change of lchmod() behavior in glibc is intended. We do not want to revert it for obvious reasons. On the other hand, the change of mknod behavior is unexpected and breaks existing software. Would not it make sense to fix this in mknod by turning the EOPNOTSUPP failure into a warning only? Kamil ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems 2020-03-11 9:04 ` Kamil Dudka @ 2020-03-11 16:36 ` Paul Eggert 0 siblings, 0 replies; 31+ messages in thread From: Paul Eggert @ 2020-03-11 16:36 UTC (permalink / raw) To: Kamil Dudka, Florian Weimer; +Cc: bug-gnulib On 3/11/20 2:04 AM, Kamil Dudka wrote: > Would not it make sense to fix this in mknod by turning the EOPNOTSUPP > failure into a warning only? No, because that would be a regression. mknod used to work in this case, and now it doesn't. Formerly, Gnulib-using programs like mknod worked around the problem that glibc lchmod always failed, by silently substituting chmod for lchmod in all cases and not bothering to call lchmod. Now that glibc lchmod merely *sometimes* fails, Gnulib-using problems need a fancier workaround. Most likely this workaround will consist of changing the 'configure'-time test for lchmod. Currently, the 'configure'-time test checks whether lchmod fails on a non-symlink. The simplest fix is to change the 'configure'-time test so that it always fails if the Linux kernel is being used. (Presumably we'll do the same for Cygwin and for Android.) ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-03-11 16:36 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-13 18:42 [PATCH] fchmodat, lchmod: port to buggy Linux filesystems Paul Eggert 2020-02-14 3:29 ` Bruno Haible 2020-02-16 17:24 ` Bruno Haible 2020-02-16 18:31 ` Paul Eggert 2020-02-16 18:58 ` Bruno Haible 2020-02-14 3:46 ` Bruno Haible 2020-02-14 21:02 ` Paul Eggert 2020-02-16 21:38 ` Bruno Haible 2020-02-16 22:28 ` Bruno Haible 2020-02-22 23:46 ` Bruno Haible 2020-02-23 8:15 ` Paul Eggert 2020-02-23 10:58 ` Bruno Haible 2020-02-23 23:56 ` Paul Eggert 2020-02-24 2:27 ` overriding glibc stub functions Bruno Haible 2020-02-24 7:44 ` Paul Eggert 2020-02-23 1:35 ` [PATCH] fchmodat, lchmod: port to buggy Linux filesystems Bruno Haible 2020-02-23 7:22 ` Paul Eggert 2020-03-09 17:30 ` Pádraig Brady 2020-03-09 18:51 ` Paul Eggert 2020-03-09 23:45 ` Pádraig Brady 2020-03-10 11:52 ` Florian Weimer 2020-03-10 15:09 ` Kamil Dudka 2020-03-10 19:27 ` Pádraig Brady 2020-03-10 19:30 ` Florian Weimer 2020-03-11 8:03 ` Paul Eggert 2020-03-11 8:45 ` Florian Weimer 2020-03-11 16:04 ` Paul Eggert 2020-03-11 8:25 ` Paul Eggert 2020-03-11 8:40 ` Florian Weimer 2020-03-11 9:04 ` Kamil Dudka 2020-03-11 16:36 ` Paul Eggert
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).