From patchwork Tue Jan 25 21:09:51 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 50441 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id EA5DA385E037 for ; Tue, 25 Jan 2022 21:11:52 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org EA5DA385E037 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1643145113; bh=oDMfyqyKFXL1c6GOZzZ7no+U9VDwjKnwpA3XNgKIsME=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=RUEHo3KnNcC6ns0RqZGb9YPdgARJXDz1k2yR6WG/KsirD1g1d/bat2L2I8JazNHrt 1uJVriOPYthULiVdzqAS8WkDH1qDkN4LRH/UY5QvQBaaGMK+v/Rd2f88qF9yCdDxwf 3rkA+Z5syC+3XFjETlP0Zc/HStxy4OQQpGhM9BE0= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id C0690385DC35 for ; Tue, 25 Jan 2022 21:09:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C0690385DC35 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-286-iNWUeuPOOUqbN6QRtxXtqQ-1; Tue, 25 Jan 2022 16:09:54 -0500 X-MC-Unique: iNWUeuPOOUqbN6QRtxXtqQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2005D839A44; Tue, 25 Jan 2022 21:09:53 +0000 (UTC) Received: from localhost (unknown [10.33.37.88]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9AED84BC5E; Tue, 25 Jan 2022 21:09:52 +0000 (UTC) To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [committed] libstdc++: Avoid symlink race in filesystem::remove_all [PR104161] Date: Tue, 25 Jan 2022 21:09:51 +0000 Message-Id: <20220125210951.864358-1-jwakely@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-13.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jonathan Wakely via Gcc-patches From: Jonathan Wakely Reply-To: Jonathan Wakely Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" Tested x86_64-linux, pushed to trunk. Backports to follow. 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 d996477254c..7b6b807114a 100644 --- a/libstdc++-v3/acinclude.m4 +++ b/libstdc++-v3/acinclude.m4 @@ -4735,6 +4735,18 @@ dnl if test $glibcxx_cv_truncate = yes; then 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 235d256a1cc..e25b7de318f 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 4c20c669144..5c6e51f5c11 100755 --- a/libstdc++-v3/configure +++ b/libstdc++-v3/configure @@ -77029,6 +77029,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 diff --git a/libstdc++-v3/src/c++17/fs_dir.cc b/libstdc++-v3/src/c++17/fs_dir.cc index eaa9cd73d11..e050304c19a 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 cf780fc0d36..1d3693af06c 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 d266d1e6290..4bfdae4e5a2 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 10a764a965e..f1bf96aab50 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 c0e25c50a85..324c6afc7a9 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))