From c8bd4dc8212e43b2f9af08b80df97f90cdb0df4f Mon Sep 17 00:00:00 2001 From: Jonathan Wakely Date: Sun, 23 Jan 2022 21:45:16 +0000 Subject: [PATCH] libstdc++: Avoid symlink race in filesystem::remove_all [PR104161] This adds a new internal flag to the filesystem::directory_iterator constructor that makes it fail if the path is a symlink that resolves to a directory. This prevents filesystem::remove_all from following a symlink to a directory, rather than deleting the symlink itself. We can also use that new flag in recursive_directory_iterator to ensure that we don't follow symlinks if the follow_directory_symlink option is not set. This also moves an error check in filesystem::remove_all after the while loop, so that errors from the directory_iterator constructor are reproted, instead of continuing to the filesystem::remove call below. libstdc++-v3/ChangeLog: PR libstdc++/104161 * acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Check for fdopendir. * config.h.in: Regenerate. * configure: Regenerate. * src/c++17/fs_dir.cc (_Dir): Add nofollow flag to constructor and pass it to base class constructor. (directory_iterator): Pass nofollow flag to _Dir constructor. (fs::recursive_directory_iterator::increment): Likewise. * src/c++17/fs_ops.cc (do_remove_all): Use nofollow option for directory_iterator constructor. Move error check outside loop. * src/filesystem/dir-common.h (_Dir_base): Add nofollow flag to constructor and when it's set use ::open with O_NOFOLLOW and O_DIRECTORY. * src/filesystem/dir.cc (_Dir): Add nofollow flag to constructor and pass it to base class constructor. (directory_iterator): Pass nofollow flag to _Dir constructor. (fs::recursive_directory_iterator::increment): Likewise. * src/filesystem/ops.cc (remove_all): Use nofollow option for directory_iterator constructor. Move error check outside loop. --- libstdc++-v3/acinclude.m4 | 12 +++++++ libstdc++-v3/config.h.in | 3 ++ libstdc++-v3/configure | 55 ++++++++++++++++++++++++++++++++ libstdc++-v3/src/c++17/fs_dir.cc | 13 +++++--- libstdc++-v3/src/c++17/fs_ops.cc | 12 +++---- libstdc++-v3/src/filesystem/dir-common.h | 48 +++++++++++++++++++++------- libstdc++-v3/src/filesystem/dir.cc | 13 +++++--- libstdc++-v3/src/filesystem/ops.cc | 6 ++-- 8 files changed, 134 insertions(+), 28 deletions(-) diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4 index d996477..7b6b807 100644 --- a/libstdc++-v3/acinclude.m4 +++ b/libstdc++-v3/acinclude.m4 @@ -4736,6 +4736,18 @@ dnl AC_DEFINE(HAVE_TRUNCATE, 1, [Define if truncate is available in .]) fi dnl + AC_CACHE_CHECK([for fdopendir], + glibcxx_cv_fdopendir, [dnl + GCC_TRY_COMPILE_OR_LINK( + [#include ], + [::fdopendir(1);], + [glibcxx_cv_fdopendir=yes], + [glibcxx_cv_fdopendir=no]) + ]) + if test $glibcxx_cv_truncate = yes; then + AC_DEFINE(HAVE_FDOPENDIR, 1, [Define if fdopendir is available in .]) + fi +dnl CXXFLAGS="$ac_save_CXXFLAGS" AC_LANG_RESTORE ]) diff --git a/libstdc++-v3/config.h.in b/libstdc++-v3/config.h.in index 235d256..e25b7de 100644 --- a/libstdc++-v3/config.h.in +++ b/libstdc++-v3/config.h.in @@ -100,6 +100,9 @@ /* Define to 1 if you have the header file. */ #undef HAVE_FCNTL_H +/* Define if fdopendir is available in . */ +#undef HAVE_FDOPENDIR + /* Define to 1 if you have the header file. */ #undef HAVE_FENV_H diff --git a/libstdc++-v3/configure b/libstdc++-v3/configure index 4c20c66..5c6e51f 100755 --- a/libstdc++-v3/configure +++ b/libstdc++-v3/configure @@ -77030,6 +77030,61 @@ $as_echo "$glibcxx_cv_truncate" >&6; } $as_echo "#define HAVE_TRUNCATE 1" >>confdefs.h fi + { $as_echo "$as_me:${as_lineno-$LINENO}: checking for fdopendir" >&5 +$as_echo_n "checking for fdopendir... " >&6; } +if ${glibcxx_cv_fdopendir+:} false; then : + $as_echo_n "(cached) " >&6 +else + if test x$gcc_no_link = xyes; then + cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +#include +int +main () +{ +::fdopendir(1); + ; + return 0; +} +_ACEOF +if ac_fn_cxx_try_compile "$LINENO"; then : + glibcxx_cv_fdopendir=yes +else + glibcxx_cv_fdopendir=no +fi +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext +else + if test x$gcc_no_link = xyes; then + as_fn_error $? "Link tests are not allowed after GCC_NO_EXECUTABLES." "$LINENO" 5 +fi +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +#include +int +main () +{ +::fdopendir(1); + ; + return 0; +} +_ACEOF +if ac_fn_cxx_try_link "$LINENO"; then : + glibcxx_cv_fdopendir=yes +else + glibcxx_cv_fdopendir=no +fi +rm -f core conftest.err conftest.$ac_objext \ + conftest$ac_exeext conftest.$ac_ext +fi + +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $glibcxx_cv_fdopendir" >&5 +$as_echo "$glibcxx_cv_fdopendir" >&6; } + if test $glibcxx_cv_truncate = yes; then + +$as_echo "#define HAVE_FDOPENDIR 1" >>confdefs.h + + fi CXXFLAGS="$ac_save_CXXFLAGS" ac_ext=c ac_cpp='$CPP $CPPFLAGS' diff --git a/libstdc++-v3/src/c++17/fs_dir.cc b/libstdc++-v3/src/c++17/fs_dir.cc index eaa9cd7..e050304 100644 --- a/libstdc++-v3/src/c++17/fs_dir.cc +++ b/libstdc++-v3/src/c++17/fs_dir.cc @@ -44,8 +44,9 @@ template class std::__shared_ptr; struct fs::_Dir : _Dir_base { - _Dir(const fs::path& p, bool skip_permission_denied, error_code& ec) - : _Dir_base(p.c_str(), skip_permission_denied, ec) + _Dir(const fs::path& p, bool skip_permission_denied, bool nofollow, + error_code& ec) + : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec) { if (!ec) path = p; @@ -128,11 +129,15 @@ namespace fs::directory_iterator:: directory_iterator(const path& p, directory_options options, error_code* ecptr) { + // Do not report an error for permission denied errors. const bool skip_permission_denied = is_set(options, directory_options::skip_permission_denied); + // Do not allow opening a symlink (used by filesystem::remove_all) + const bool nofollow + = is_set(options, __directory_iterator_nofollow); error_code ec; - _Dir dir(p, skip_permission_denied, ec); + _Dir dir(p, skip_permission_denied, nofollow, ec); if (dir.dirp) { @@ -287,7 +292,7 @@ fs::recursive_directory_iterator::increment(error_code& ec) if (std::exchange(_M_dirs->pending, true) && top.should_recurse(follow, ec)) { - _Dir dir(top.entry.path(), skip_permission_denied, ec); + _Dir dir(top.entry.path(), skip_permission_denied, !follow, ec); if (ec) { _M_dirs.reset(); diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc index cf780fc..1d3693a 100644 --- a/libstdc++-v3/src/c++17/fs_ops.cc +++ b/libstdc++-v3/src/c++17/fs_ops.cc @@ -1332,7 +1332,7 @@ namespace uintmax_t count = 0; if (s.type() == file_type::directory) { - directory_iterator d(p, ec), end; + directory_iterator d(p, directory_options{99}, ec), end; while (d != end) { const auto removed = fs::do_remove_all(d->path(), err); @@ -1341,11 +1341,11 @@ namespace count += removed; d.increment(ec); - if (ec) - { - err.report(ec, p); - return -1; - } + } + if (ec) + { + err.report(ec, p); + return -1; } } diff --git a/libstdc++-v3/src/filesystem/dir-common.h b/libstdc++-v3/src/filesystem/dir-common.h index d266d1e..4bfdae4 100644 --- a/libstdc++-v3/src/filesystem/dir-common.h +++ b/libstdc++-v3/src/filesystem/dir-common.h @@ -36,6 +36,10 @@ # endif # include #endif +#ifdef _GLIBCXX_HAVE_FCNTL_H +# include // O_NOFOLLOW, O_DIRECTORY +# include // close +#endif namespace std _GLIBCXX_VISIBILITY(default) { @@ -76,21 +80,40 @@ struct _Dir_base _Dir_base(posix::DIR* dirp = nullptr) : dirp(dirp) { } // If no error occurs then dirp is non-null, - // otherwise null (whether error ignored or not). + // otherwise null (even if an EACCES error is ignored). _Dir_base(const posix::char_type* pathname, bool skip_permission_denied, - error_code& ec) noexcept - : dirp(posix::opendir(pathname)) + [[maybe_unused]] bool nofollow, error_code& ec) noexcept + : dirp(nullptr) { - if (dirp) +#if defined O_RDONLY && O_NOFOLLOW && defined O_DIRECTORY && defined O_CLOEXEC \ + && defined _GLIBCXX_HAVE_FDOPENDIR && !_GLIBCXX_FILESYSTEM_IS_WINDOWS + if (nofollow) + { + // Do not allow opening a symlink (used by filesystem::remove_all) + const int flags = O_RDONLY | O_NOFOLLOW | O_DIRECTORY | O_CLOEXEC; + int fd = ::open(pathname, flags); + if (fd != -1) + { + if ((dirp = ::fdopendir(fd))) + { + ec.clear(); + return; + } + } + if (errno == EACCES && skip_permission_denied) + ec.clear(); + else + ec.assign(errno, std::generic_category()); + return; + } +#endif + + if ((dirp = posix::opendir(pathname))) + ec.clear(); + else if (errno == EACCES && skip_permission_denied) ec.clear(); else - { - const int err = errno; - if (err == EACCES && skip_permission_denied) - ec.clear(); - else - ec.assign(err, std::generic_category()); - } + ec.assign(errno, std::generic_category()); } _Dir_base(_Dir_base&& d) : dirp(std::exchange(d.dirp, nullptr)) { } @@ -187,6 +210,9 @@ get_file_type(const std::filesystem::__gnu_posix::dirent& d [[gnu::unused]]) return file_type::none; #endif } + +constexpr directory_options __directory_iterator_nofollow{99}; + _GLIBCXX_END_NAMESPACE_FILESYSTEM _GLIBCXX_END_NAMESPACE_VERSION diff --git a/libstdc++-v3/src/filesystem/dir.cc b/libstdc++-v3/src/filesystem/dir.cc index 10a764a..f1bf96a 100644 --- a/libstdc++-v3/src/filesystem/dir.cc +++ b/libstdc++-v3/src/filesystem/dir.cc @@ -47,8 +47,9 @@ namespace posix = std::filesystem::__gnu_posix; struct fs::_Dir : std::filesystem::_Dir_base { - _Dir(const fs::path& p, bool skip_permission_denied, error_code& ec) - : _Dir_base(p.c_str(), skip_permission_denied, ec) + _Dir(const fs::path& p, bool skip_permission_denied, bool nofollow, + error_code& ec) + : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec) { if (!ec) path = p; @@ -126,11 +127,15 @@ namespace fs::directory_iterator:: directory_iterator(const path& p, directory_options options, error_code* ecptr) { + // Do not report an error for permission denied errors. const bool skip_permission_denied = is_set(options, directory_options::skip_permission_denied); + // Do not allow opening a symlink (used by filesystem::remove_all) + const bool nofollow + = is_set(options, __directory_iterator_nofollow); error_code ec; - _Dir dir(p, skip_permission_denied, ec); + _Dir dir(p, skip_permission_denied, nofollow, ec); if (dir.dirp) { @@ -270,7 +275,7 @@ fs::recursive_directory_iterator::increment(error_code& ec) noexcept if (std::exchange(_M_pending, true) && top.should_recurse(follow, ec)) { - _Dir dir(top.entry.path(), skip_permission_denied, ec); + _Dir dir(top.entry.path(), skip_permission_denied, !follow, ec); if (ec) { _M_dirs.reset(); diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc index c0e25c5..324c6af 100644 --- a/libstdc++-v3/src/filesystem/ops.cc +++ b/libstdc++-v3/src/filesystem/ops.cc @@ -1105,7 +1105,7 @@ fs::remove_all(const path& p, error_code& ec) noexcept uintmax_t count = 0; if (s.type() == file_type::directory) { - directory_iterator d(p, ec), end; + directory_iterator d(p, directory_options{99}, ec), end; while (!ec && d != end) { const auto removed = fs::remove_all(d->path(), ec); @@ -1113,9 +1113,9 @@ fs::remove_all(const path& p, error_code& ec) noexcept return -1; count += removed; d.increment(ec); - if (ec) - return -1; } + if (ec) + return -1; } if (fs::remove(p, ec)) -- 2.7.4