On 6/12/22 08:03, Bruno Haible wrote:
Two tests now fail, that succeeded before yesterday's patch.
Thanks for reporting that. Although I don't have MS-Windows I stared at the code a bit and I think I see what might be the problem. I found some other potential issues too (basically, I didn't switch from fstatat/lstat to readlinkat/readlink often enough so the code is still prone to EOVERFLOW problems). I installed the attached further patch, which I hope fixes things on MS-Windows and fixes the other issues too.
From d682f8de7f9d384f4cfc482a3ba2960329a8db21 Mon Sep 17 00:00:00 2001 From: Paul Eggert <egg...@cs.ucla.edu> Date: Sun, 12 Jun 2022 13:46:52 -0700 Subject: [PATCH] fchmodat: port better to MS-Windows etc. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MS-Windows problem reported by Bruno Haible in: https://lists.gnu.org/r/bug-gnulib/2022-06/msg00041.html Although I don’t use MS-Windows I see some related fstatat etc. problems and am trying to fix them with this further patch. * lib/fchmodat.c (fchmodat): * lib/lchmod.c (lchmod): * lib/lchown.c (lchown) [!HAVE_LCHOWN && HAVE_CHOWN && !CHOWN_MODIFIES_SYMLINK]: * lib/renameatu.c (renameatu) [HAVE_RENAME && RENAME_TRAILING_SLASH_SOURCE_BUG]: Use readlinkat/readlink instead of fstatat/lstat to test merely whether a string names a symlink, as this avoids problems with EOVERFLOW. Also, I hope it works around the MS-Windows issues that Bruno noted. * m4/fchmodat.m4 (gl_PREREQ_FCHMODAT): Check for readlinkat, not lchmod. * m4/lchmod.m4 (gl_FUNC_LCHMOD): Do not require AC_CANONICAL_HOST or check for lstat. (gl_PREREQ_LCHMOD): Check for readlink. * modules/lchown (Depends-on): Add readlink. Do not depend on lstat merely because !HAVE_LCHOWN. * modules/renameatu (Depends-on): Add fstatat, readlinkat. --- ChangeLog | 26 ++++++++++++++++++++++++++ lib/fchmodat.c | 16 ++++++++-------- lib/lchmod.c | 17 +++++++---------- lib/lchown.c | 4 ++-- lib/renameatu.c | 7 ++++--- m4/fchmodat.m4 | 4 ++-- m4/lchmod.m4 | 7 +++---- modules/lchown | 3 ++- modules/renameatu | 2 ++ 9 files changed, 56 insertions(+), 30 deletions(-) diff --git a/ChangeLog b/ChangeLog index 2d0340b933..2daa6d8c81 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,29 @@ +2022-06-12 Paul Eggert <egg...@cs.ucla.edu> + + fchmodat: port better to MS-Windows etc. + MS-Windows problem reported by Bruno Haible in: + https://lists.gnu.org/r/bug-gnulib/2022-06/msg00041.html + Although I don’t use MS-Windows I see some related fstatat etc. + problems and am trying to fix them with this further patch. + * lib/fchmodat.c (fchmodat): + * lib/lchmod.c (lchmod): + * lib/lchown.c (lchown) + [!HAVE_LCHOWN && HAVE_CHOWN && !CHOWN_MODIFIES_SYMLINK]: + * lib/renameatu.c (renameatu) + [HAVE_RENAME && RENAME_TRAILING_SLASH_SOURCE_BUG]: + Use readlinkat/readlink instead of fstatat/lstat to test merely + whether a string names a symlink, as this avoids problems + with EOVERFLOW. Also, I hope it works around the MS-Windows + issues that Bruno noted. + * m4/fchmodat.m4 (gl_PREREQ_FCHMODAT): + Check for readlinkat, not lchmod. + * m4/lchmod.m4 (gl_FUNC_LCHMOD): Do not require AC_CANONICAL_HOST + or check for lstat. + (gl_PREREQ_LCHMOD): Check for readlink. + * modules/lchown (Depends-on): Add readlink. Do not depend on + lstat merely because !HAVE_LCHOWN. + * modules/renameatu (Depends-on): Add fstatat, readlinkat. + 2022-06-12 Bruno Haible <br...@clisp.org> doc: Update O_PATH platforms list. diff --git a/lib/fchmodat.c b/lib/fchmodat.c index b233c366de..8ed4cb7398 100644 --- a/lib/fchmodat.c +++ b/lib/fchmodat.c @@ -83,9 +83,10 @@ 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 HAVE_READLINKAT + char readlink_buf[1]; -# ifdef O_PATH +# ifdef O_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. @@ -96,7 +97,7 @@ fchmodat (int dir, char const *file, mode_t mode, int flags) int err; char buf[1]; - if (0 <= readlinkat (fd, "", buf, sizeof buf)) + if (0 <= readlinkat (fd, "", readlink_buf, sizeof readlink_buf)) err = EOPNOTSUPP; else if (errno == EINVAL) { @@ -113,17 +114,16 @@ fchmodat (int dir, char const *file, mode_t mode, int flags) errno = err; if (0 <= err) return err == 0 ? 0 : -1; -# endif +# endif /* O_PATH + /proc is not supported. */ - int fstatat_result = fstatat (dir, file, &st, AT_SYMLINK_NOFOLLOW); - if (fstatat_result != 0) - return fstatat_result; - if (S_ISLNK (st.st_mode)) + + if (0 <= readlinkat (dir, file, readlink_buf, sizeof readlink_buf)) { errno = EOPNOTSUPP; return -1; } +# endif /* Fall back on orig_fchmodat with no flags, despite a possible race. */ flags = 0; diff --git a/lib/lchmod.c b/lib/lchmod.c index b4cc0a8176..f21bf7f652 100644 --- a/lib/lchmod.c +++ b/lib/lchmod.c @@ -45,7 +45,10 @@ int lchmod (char const *file, mode_t mode) { -#ifdef O_PATH +#if HAVE_READLINK + char readlink_buf[1]; + +# ifdef O_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. @@ -55,8 +58,7 @@ lchmod (char const *file, mode_t mode) return fd; int err; - char buf[1]; - if (0 <= readlinkat (fd, "", buf, sizeof buf)) + if (0 <= readlinkat (fd, "", readlink_buf, sizeof readlink_buf)) err = EOPNOTSUPP; else if (errno == EINVAL) { @@ -73,16 +75,11 @@ lchmod (char const *file, mode_t mode) errno = err; if (0 <= err) return err == 0 ? 0 : -1; -#endif +# endif /* O_PATH + /proc is not supported. */ -#if HAVE_LSTAT - struct stat st; - int lstat_result = lstat (file, &st); - if (lstat_result != 0) - return lstat_result; - if (S_ISLNK (st.st_mode)) + if (0 <= readlink (file, readlink_buf, sizeof readlink_buf)) { errno = EOPNOTSUPP; return -1; diff --git a/lib/lchown.c b/lib/lchown.c index 105c2d9990..8b0d871a27 100644 --- a/lib/lchown.c +++ b/lib/lchown.c @@ -45,9 +45,9 @@ lchown (const char *file, uid_t uid, gid_t gid) { # if HAVE_CHOWN # if ! CHOWN_MODIFIES_SYMLINK - struct stat stats; + char readlink_buf[1]; - if (lstat (file, &stats) == 0 && S_ISLNK (stats.st_mode)) + if (0 <= readlink (file, readlink_buf, sizeof readlink_buf)) { errno = EOPNOTSUPP; return -1; diff --git a/lib/renameatu.c b/lib/renameatu.c index b4e317d6e5..7ba186cae7 100644 --- a/lib/renameatu.c +++ b/lib/renameatu.c @@ -214,15 +214,16 @@ renameatu (int fd1, char const *src, int fd2, char const *dst, goto out; } strip_trailing_slashes (dst_temp); - if (fstatat (fd2, dst_temp, &dst_st, AT_SYMLINK_NOFOLLOW)) + char readlink_buf[1]; + if (readlinkat (fd2, dst_temp, readlink_buf, sizeof readlink_buf) < 0) { - if (errno != ENOENT) + if (errno != ENOENT && errno != EINVAL) { rename_errno = errno; goto out; } } - else if (S_ISLNK (dst_st.st_mode)) + else goto out; } # endif /* RENAME_TRAILING_SLASH_SOURCE_BUG */ diff --git a/m4/fchmodat.m4 b/m4/fchmodat.m4 index a5cf95a88b..f743ce1b02 100644 --- a/m4/fchmodat.m4 +++ b/m4/fchmodat.m4 @@ -1,4 +1,4 @@ -# fchmodat.m4 serial 6 +# fchmodat.m4 serial 7 dnl Copyright (C) 2004-2022 Free Software Foundation, Inc. dnl This file is free software; the Free Software Foundation dnl gives unlimited permission to copy and/or distribute it, @@ -97,6 +97,6 @@ AC_DEFUN([gl_FUNC_FCHMODAT], # Prerequisites of lib/fchmodat.c. AC_DEFUN([gl_PREREQ_FCHMODAT], [ - AC_CHECK_FUNCS_ONCE([lchmod]) + AC_CHECK_FUNCS_ONCE([readlinkat]) : ]) diff --git a/m4/lchmod.m4 b/m4/lchmod.m4 index 5baee738ef..bfc925fbe4 100644 --- a/m4/lchmod.m4 +++ b/m4/lchmod.m4 @@ -1,4 +1,4 @@ -#serial 8 +#serial 9 dnl Copyright (C) 2005-2006, 2008-2022 Free Software Foundation, Inc. dnl This file is free software; the Free Software Foundation @@ -15,9 +15,7 @@ AC_DEFUN([gl_FUNC_LCHMOD], dnl Persuade glibc <sys/stat.h> to declare lchmod(). AC_REQUIRE([AC_USE_SYSTEM_EXTENSIONS]) - AC_REQUIRE([AC_CANONICAL_HOST]) dnl for cross-compiles - - AC_CHECK_FUNCS_ONCE([lchmod lstat]) + AC_CHECK_FUNCS_ONCE([lchmod]) if test "$ac_cv_func_lchmod" = no; then HAVE_LCHMOD=0 fi @@ -26,5 +24,6 @@ AC_DEFUN([gl_FUNC_LCHMOD], # Prerequisites of lib/lchmod.c. AC_DEFUN([gl_PREREQ_LCHMOD], [ + AC_CHECK_FUNCS_ONCE([readlink]) : ]) diff --git a/modules/lchown b/modules/lchown index 335d191f9f..217dd75cce 100644 --- a/modules/lchown +++ b/modules/lchown @@ -7,11 +7,12 @@ m4/lchown.m4 Depends-on: unistd +readlink [test $HAVE_LCHOWN = 0] chown [test $HAVE_LCHOWN = 0 || test $REPLACE_LCHOWN = 1] errno [test $HAVE_LCHOWN = 0 || test $REPLACE_LCHOWN = 1] -lstat [test $HAVE_LCHOWN = 0 || test $REPLACE_LCHOWN = 1] stdbool [test $HAVE_LCHOWN = 0 || test $REPLACE_LCHOWN = 1] sys_stat [test $HAVE_LCHOWN = 0 || test $REPLACE_LCHOWN = 1] +lstat [test $REPLACE_LCHOWN = 1] configure.ac: gl_FUNC_LCHOWN diff --git a/modules/renameatu b/modules/renameatu index e0c1ce3176..3fc68a91dc 100644 --- a/modules/renameatu +++ b/modules/renameatu @@ -13,6 +13,8 @@ extensions fcntl-h filenamecat-lgpl [test $HAVE_RENAMEAT = 0 || test $REPLACE_RENAMEAT = 1] openat-h [test $HAVE_RENAMEAT = 0 || test $REPLACE_RENAMEAT = 1] +fstatat [test $REPLACE_RENAMEAT = 1] +readlinkat [test $REPLACE_RENAMEAT = 1] stdbool [test $REPLACE_RENAMEAT = 1] at-internal [test $HAVE_RENAMEAT = 0] filename [test $HAVE_RENAMEAT = 0] -- 2.34.1
