* [committed] libstdc++: Fix filesystem::remove_all races [PR104161]
@ 2022-02-04 23:54 Jonathan Wakely
2022-02-05 1:08 ` Jonathan Wakely
0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Wakely @ 2022-02-04 23:54 UTC (permalink / raw)
To: libstdc++, gcc-patches
Tested powerpc64le-linux and powerpc-aix, pushed to trunk.
This fixes the remaining filesystem::remove_all race condition by using
POSIX openat to recurse into sub-directories and using POSIX unlinkat to
remove files. This avoids the remaining race where the directory being
removed is replaced with a symlink after the directory has been opened,
so that the filesystem::remove("subdir/file") resolves to "target/file"
instead, because "subdir" has been removed and replaced with a symlink.
The previous patch only fixed the case where the directory was replaced
with a symlink before we tried to open it, but it still used the full
(potentially compromised) path as an argument to filesystem::remove.
The first part of the fix is to use openat when recursing into a
sub-directory with recursive_directory_iterator. This means that opening
"dir/subdir" uses the file descriptor for "dir", and so is sure to open
"dir/subdir" and not "symlink/subdir". (The previous patch to use
O_NOFOLLOW already ensured we won't open "dir/symlink/" here.)
The second part of the fix is to use unlinkat for the remove_all
operation. Previously we used a directory_iterator to get the name of
each file in a directory and then used filesystem::remove(iter->path())
on that name. This meant that any checks (e.g. O_NOFOLLOW) done by the
iterator could be invalidated before the remove operation on that
pathname. The directory iterator contains an open DIR stream, which we
can use to obtain a file descriptor to pass to unlinkat. This ensures
that the file being deleted really is contained within the directory
we're iterating over, rather than using a pathname that could resolve to
some other file.
The filesystem::remove_all function previously used a (non-recursive)
filesystem::directory_iterator for each directory, and called itself
recursively for sub-directories. The new implementation uses a single
filesystem::recursive_directory_iterator object, and calls a new __erase
member function on that iterator. That new __erase member function does
the actual work of removing a file (or a directory after its contents
have been iterated over and removed) using unlinkat. That means we don't
need to expose the DIR stream or its file descriptor to the remove_all
function, it's still encapuslated by the iterator class.
It would be possible to add a __rewind member to directory iterators
too, to call rewinddir after each modification to the directory. That
would make it more likely for filesystem::remove_all to successfully
remove everything even if files are being written to the directory tree
while removing it. It's unclear if that is actually prefereable, or if
it's better to fail and report an error at the first opportunity.
The necessary APIs (openat, unlinkat, fdopendir, dirfd) are defined in
POSIX.1-2008, and in Glibc since 2.10. But if the target doesn't provide
them, the original code (with race conditions) is still used.
This also reduces the number of small memory allocations needed for
std::filesystem::remove_all, because we do not store the full path to
every directory entry that is iterated over. The new filename_only
option means we only store the filename in the directory entry, as that
is all we need in order to use openat or unlinkat.
Finally, rather than duplicating everything for the Filesystem TS, the
std::experimental::filesystem::remove_all implementation now just calls
std::filesystem::remove_all to do the work.
libstdc++-v3/ChangeLog:
PR libstdc++/104161
* acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Check for dirfd
and unlinkat.
* config.h.in: Regenerate.
* configure: Regenerate.
* include/bits/fs_dir.h (recursive_directory_iterator): Declare
remove_all overloads as friends.
(recursive_directory_iterator::__erase): Declare new member
function.
* include/bits/fs_fwd.h (remove, remove_all): Declare.
* src/c++17/fs_dir.cc (_Dir): Add filename_only parameter to
constructor. Pass file descriptor argument to base constructor.
(_Dir::dir_and_pathname, _Dir::open_subdir, _Dir::do_unlink)
(_Dir::unlink, _Dir::rmdir): Define new member functions.
(directory_iterator): Pass filename_only argument to _Dir
constructor.
(recursive_directory_iterator::_Dir_stack): Adjust constructor
parameters to take a _Dir rvalue instead of creating one.
(_Dir_stack::orig): Add data member for storing original path.
(_Dir_stack::report_error): Define new member function.
(__directory_iterator_nofollow): Move here from dir-common.h and
fix value to be a power of two.
(__directory_iterator_filename_only): Define new constant.
(recursive_directory_iterator): Construct _Dir object and move
into _M_dirs stack. Pass skip_permission_denied argument to first
advance call.
(recursive_directory_iterator::increment): Use _Dir::open_subdir.
(recursive_directory_iterator::__erase): Define new member
function.
* src/c++17/fs_ops.cc (ErrorReporter, do_remove_all): Remove.
(fs::remove_all): Use new recursive_directory_iterator::__erase
member function.
* src/filesystem/dir-common.h (_Dir_base): Add int parameter to
constructor and use openat to implement nofollow semantics.
(_Dir_base::fdcwd, _Dir_base::set_close_on_exec, _Dir_base::openat):
Define new member functions.
(__directory_iterator_nofollow): Move to fs_dir.cc.
* src/filesystem/dir.cc (_Dir): Pass file descriptor argument to
base constructor.
(_Dir::dir_and_pathname, _Dir::open_subdir): Define new member
functions.
(recursive_directory_iterator::_Dir_stack): Adjust constructor
parameters to take a _Dir rvalue instead of creating one.
(recursive_directory_iterator): Check for new nofollow option.
Construct _Dir object and move into _M_dirs stack. Pass
skip_permission_denied argument to first advance call.
(recursive_directory_iterator::increment): Use _Dir::open_subdir.
* src/filesystem/ops.cc (fs::remove_all): Use C++17 remove_all.
---
libstdc++-v3/acinclude.m4 | 27 ++-
libstdc++-v3/config.h.in | 6 +
libstdc++-v3/configure | 116 ++++++++++-
libstdc++-v3/include/bits/fs_dir.h | 8 +
libstdc++-v3/include/bits/fs_fwd.h | 4 +
libstdc++-v3/src/c++17/fs_dir.cc | 251 ++++++++++++++++++++---
libstdc++-v3/src/c++17/fs_ops.cc | 139 +++++--------
libstdc++-v3/src/filesystem/dir-common.h | 145 ++++++++-----
libstdc++-v3/src/filesystem/dir.cc | 77 ++++---
libstdc++-v3/src/filesystem/ops.cc | 31 +--
10 files changed, 573 insertions(+), 231 deletions(-)
diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4
index 066453e2148..7cc52f4db96 100644
--- a/libstdc++-v3/acinclude.m4
+++ b/libstdc++-v3/acinclude.m4
@@ -4748,13 +4748,38 @@ dnl
glibcxx_cv_fdopendir, [dnl
GCC_TRY_COMPILE_OR_LINK(
[#include <dirent.h>],
- [::fdopendir(1);],
+ [::DIR* dir = ::fdopendir(1);],
[glibcxx_cv_fdopendir=yes],
[glibcxx_cv_fdopendir=no])
])
if test $glibcxx_cv_fdopendir = yes; then
AC_DEFINE(HAVE_FDOPENDIR, 1, [Define if fdopendir is available in <dirent.h>.])
fi
+dnl
+ AC_CACHE_CHECK([for dirfd],
+ glibcxx_cv_dirfd, [dnl
+ GCC_TRY_COMPILE_OR_LINK(
+ [#include <dirent.h>],
+ [int fd = ::dirfd((::DIR*)0);],
+ [glibcxx_cv_dirfd=yes],
+ [glibcxx_cv_dirfd=no])
+ ])
+ if test $glibcxx_cv_dirfd = yes; then
+ AC_DEFINE(HAVE_DIRFD, 1, [Define if dirfd is available in <dirent.h>.])
+ fi
+dnl
+ AC_CACHE_CHECK([for unlinkat],
+ glibcxx_cv_unlinkat, [dnl
+ GCC_TRY_COMPILE_OR_LINK(
+ [#include <fcntl.h>
+ #include <unistd.h>],
+ [::unlinkat(AT_FDCWD, "", AT_REMOVEDIR);],
+ [glibcxx_cv_unlinkat=yes],
+ [glibcxx_cv_unlinkat=no])
+ ])
+ if test $glibcxx_cv_unlinkat = yes; then
+ AC_DEFINE(HAVE_UNLINKAT, 1, [Define if unlinkat is available in <fcntl.h>.])
+ fi
dnl
CXXFLAGS="$ac_save_CXXFLAGS"
AC_LANG_RESTORE
diff --git a/libstdc++-v3/config.h.in b/libstdc++-v3/config.h.in
index e25b7de318f..f6212de9268 100644
--- a/libstdc++-v3/config.h.in
+++ b/libstdc++-v3/config.h.in
@@ -73,6 +73,9 @@
/* Define to 1 if you have the <dirent.h> header file. */
#undef HAVE_DIRENT_H
+/* Define if dirfd is available in <dirent.h>. */
+#undef HAVE_DIRFD
+
/* Define to 1 if you have the <dlfcn.h> header file. */
#undef HAVE_DLFCN_H
@@ -486,6 +489,9 @@
/* Define to 1 if you have the <unistd.h> header file. */
#undef HAVE_UNISTD_H
+/* Define if unlinkat is available in <fcntl.h>. */
+#undef HAVE_UNLINKAT
+
/* Define to 1 if you have the `uselocale' function. */
#undef HAVE_USELOCALE
diff --git a/libstdc++-v3/configure b/libstdc++-v3/configure
index ed64b5599c8..ef80912d0b9 100755
--- a/libstdc++-v3/configure
+++ b/libstdc++-v3/configure
@@ -77077,7 +77077,7 @@ else
int
main ()
{
-::fdopendir(1);
+::DIR* dir = ::fdopendir(1);
;
return 0;
}
@@ -77098,7 +77098,7 @@ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
int
main ()
{
-::fdopendir(1);
+::DIR* dir = ::fdopendir(1);
;
return 0;
}
@@ -77119,6 +77119,118 @@ $as_echo "$glibcxx_cv_fdopendir" >&6; }
$as_echo "#define HAVE_FDOPENDIR 1" >>confdefs.h
+ fi
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for dirfd" >&5
+$as_echo_n "checking for dirfd... " >&6; }
+if ${glibcxx_cv_dirfd+:} 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 <dirent.h>
+int
+main ()
+{
+int fd = ::dirfd((::DIR*)0);
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_compile "$LINENO"; then :
+ glibcxx_cv_dirfd=yes
+else
+ glibcxx_cv_dirfd=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 <dirent.h>
+int
+main ()
+{
+int fd = ::dirfd((::DIR*)0);
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_link "$LINENO"; then :
+ glibcxx_cv_dirfd=yes
+else
+ glibcxx_cv_dirfd=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_dirfd" >&5
+$as_echo "$glibcxx_cv_dirfd" >&6; }
+ if test $glibcxx_cv_dirfd = yes; then
+
+$as_echo "#define HAVE_DIRFD 1" >>confdefs.h
+
+ fi
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for unlinkat" >&5
+$as_echo_n "checking for unlinkat... " >&6; }
+if ${glibcxx_cv_unlinkat+:} 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 <fcntl.h>
+ #include <unistd.h>
+int
+main ()
+{
+::unlinkat(AT_FDCWD, "", AT_REMOVEDIR);
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_compile "$LINENO"; then :
+ glibcxx_cv_unlinkat=yes
+else
+ glibcxx_cv_unlinkat=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 <fcntl.h>
+ #include <unistd.h>
+int
+main ()
+{
+::unlinkat(AT_FDCWD, "", AT_REMOVEDIR);
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_link "$LINENO"; then :
+ glibcxx_cv_unlinkat=yes
+else
+ glibcxx_cv_unlinkat=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_unlinkat" >&5
+$as_echo "$glibcxx_cv_unlinkat" >&6; }
+ if test $glibcxx_cv_unlinkat = yes; then
+
+$as_echo "#define HAVE_UNLINKAT 1" >>confdefs.h
+
fi
CXXFLAGS="$ac_save_CXXFLAGS"
ac_ext=c
diff --git a/libstdc++-v3/include/bits/fs_dir.h b/libstdc++-v3/include/bits/fs_dir.h
index 2edd77687fe..ca37952ec17 100644
--- a/libstdc++-v3/include/bits/fs_dir.h
+++ b/libstdc++-v3/include/bits/fs_dir.h
@@ -537,6 +537,14 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
struct _Dir_stack;
std::__shared_ptr<_Dir_stack> _M_dirs;
+
+ recursive_directory_iterator&
+ __erase(error_code* = nullptr);
+
+ friend uintmax_t
+ filesystem::remove_all(const path&, error_code&);
+ friend uintmax_t
+ filesystem::remove_all(const path&);
};
/// @relates std::filesystem::recursive_directory_iterator @{
diff --git a/libstdc++-v3/include/bits/fs_fwd.h b/libstdc++-v3/include/bits/fs_fwd.h
index bc063761080..d8cde5e5eba 100644
--- a/libstdc++-v3/include/bits/fs_fwd.h
+++ b/libstdc++-v3/include/bits/fs_fwd.h
@@ -354,6 +354,10 @@ _GLIBCXX_END_NAMESPACE_CXX11
bool is_regular_file(file_status) noexcept;
bool is_symlink(file_status) noexcept;
+ bool remove(const path&, error_code&) noexcept;
+ uintmax_t remove_all(const path&);
+ uintmax_t remove_all(const path&, error_code&);
+
/// @}
} // namespace filesystem
_GLIBCXX_END_NAMESPACE_VERSION
diff --git a/libstdc++-v3/src/c++17/fs_dir.cc b/libstdc++-v3/src/c++17/fs_dir.cc
index a77aabb6dcc..01b8c0d5693 100644
--- a/libstdc++-v3/src/c++17/fs_dir.cc
+++ b/libstdc++-v3/src/c++17/fs_dir.cc
@@ -45,9 +45,14 @@ template class std::__shared_ptr<fs::recursive_directory_iterator::_Dir_stack>;
struct fs::_Dir : _Dir_base
{
_Dir(const fs::path& p, bool skip_permission_denied, bool nofollow,
- error_code& ec)
- : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec)
+ [[maybe_unused]] bool filename_only, error_code& ec)
+ : _Dir_base(fdcwd(), p.c_str(), skip_permission_denied, nofollow, ec)
{
+#if _GLIBCXX_HAVE_DIRFD
+ if (filename_only)
+ return; // Do not store path p when we aren't going to use it.
+#endif
+
if (!ec)
path = p;
}
@@ -112,7 +117,63 @@ struct fs::_Dir : _Dir_base
return false;
}
- fs::path path;
+ // Return a file descriptor for the directory and current entry's path.
+ // If dirfd is available, use it and return only the filename.
+ // Otherwise, return AT_FDCWD and return the full path.
+ pair<int, const posix::char_type*>
+ dir_and_pathname() const noexcept
+ {
+ const fs::path& p = entry.path();
+#if _GLIBCXX_HAVE_DIRFD
+ if (!p.empty())
+ return {::dirfd(this->dirp), std::prev(p.end())->c_str()};
+#endif
+ return {this->fdcwd(), p.c_str()};
+ }
+
+ // Create a new _Dir for the directory this->entry.path().
+ _Dir
+ open_subdir(bool skip_permission_denied, bool nofollow,
+ error_code& ec) const noexcept
+ {
+ auto [dirfd, pathname] = dir_and_pathname();
+ _Dir_base d(dirfd, pathname, skip_permission_denied, nofollow, ec);
+ // If this->path is empty, the new _Dir should have an empty path too.
+ const fs::path& p = this->path.empty() ? this->path : this->entry.path();
+ return _Dir(std::exchange(d.dirp, nullptr), p);
+ }
+
+ bool
+ do_unlink(bool is_directory, error_code& ec) const noexcept
+ {
+#if _GLIBCXX_HAVE_UNLINKAT
+ auto [dirfd, pathname] = dir_and_pathname();
+ if (::unlinkat(dirfd, pathname, is_directory ? AT_REMOVEDIR : 0) == -1)
+ {
+ ec.assign(errno, std::generic_category());
+ return false;
+ }
+ else
+ {
+ ec.clear();
+ return true;
+ }
+#else
+ return fs::remove(entry.path(), ec);
+#endif
+ }
+
+ // Remove the non-directory that this->entry refers to.
+ bool
+ unlink(error_code& ec) const noexcept
+ { return do_unlink(/* is_directory*/ false, ec); }
+
+ // Remove the directory that this->entry refers to.
+ bool
+ rmdir(error_code& ec) const noexcept
+ { return do_unlink(/* is_directory*/ true, ec); }
+
+ fs::path path; // Empty if only using unlinkat with file descr.
directory_entry entry;
};
@@ -124,6 +185,20 @@ namespace
{
return (obj & bits) != Bitmask::none;
}
+
+// Non-standard directory option flags, currently only for internal use:
+//
+// Do not allow directory iterator to open a symlink.
+// This might seem redundant given directory_options::follow_directory_symlink
+// but that is only checked for recursing into sub-directories, and we need
+// something that controls the initial opendir() call in the constructor.
+constexpr fs::directory_options __directory_iterator_nofollow{64};
+// Do not store full paths in std::filesystem::recursive_directory_iterator.
+// When fs::remove_all uses recursive_directory_iterator::__erase and unlinkat
+// is available in libc, we do not need the parent directory's path, only the
+// filenames of the directory entries (and a file descriptor for the parent).
+// This flag avoids allocating memory for full paths that won't be needed.
+constexpr fs::directory_options __directory_iterator_filename_only{128};
}
fs::directory_iterator::
@@ -132,12 +207,11 @@ 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);
+ // Do not allow opening a symlink.
+ const bool nofollow = is_set(options, __directory_iterator_nofollow);
error_code ec;
- _Dir dir(p, skip_permission_denied, nofollow, ec);
+ _Dir dir(p, skip_permission_denied, nofollow, /*filename only*/false, ec);
if (dir.dirp)
{
@@ -185,48 +259,66 @@ fs::directory_iterator::increment(error_code& ec)
struct fs::recursive_directory_iterator::_Dir_stack : std::stack<_Dir>
{
- _Dir_stack(directory_options opts, posix::DIR* dirp, const path& p)
+ _Dir_stack(directory_options opts, _Dir&& dir)
: options(opts), pending(true)
{
- this->emplace(dirp, p);
+ this->push(std::move(dir));
}
+ path::string_type orig;
const directory_options options;
bool pending;
void clear() { c.clear(); }
+
+ path current_path() const
+ {
+ path p;
+ if (top().path.empty())
+ {
+ // Reconstruct path that failed from dir stack.
+ p = orig;
+ for (auto& d : this->c)
+ p /= d.entry.path();
+ }
+ else
+ p = top().entry.path();
+ return p;
+ }
};
fs::recursive_directory_iterator::
recursive_directory_iterator(const path& p, directory_options options,
error_code* ecptr)
{
- if (posix::DIR* dirp = posix::opendir(p.c_str()))
+ // 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 as the starting directory.
+ const bool nofollow = is_set(options, __directory_iterator_nofollow);
+ // Prefer to store only filenames (not full paths) in directory_entry values.
+ const bool filename_only
+ = is_set(options, __directory_iterator_filename_only);
+
+ error_code ec;
+ _Dir dir(p, skip_permission_denied, nofollow, filename_only, ec);
+
+ if (dir.dirp)
{
- if (ecptr)
- ecptr->clear();
- auto sp = std::__make_shared<_Dir_stack>(options, dirp, p);
- if (ecptr ? sp->top().advance(*ecptr) : sp->top().advance())
- _M_dirs.swap(sp);
- }
- else
- {
- const int err = errno;
- if (fs::is_permission_denied_error(err)
- && is_set(options, fs::directory_options::skip_permission_denied))
+ auto sp = std::__make_shared<_Dir_stack>(options, std::move(dir));
+ if (ecptr ? sp->top().advance(skip_permission_denied, *ecptr)
+ : sp->top().advance(skip_permission_denied))
{
- if (ecptr)
- ecptr->clear();
- return;
+ _M_dirs.swap(sp);
+ if (filename_only) // Need to save original path for error reporting.
+ _M_dirs->orig = p.native();
}
-
- if (!ecptr)
- _GLIBCXX_THROW_OR_ABORT(filesystem_error(
- "recursive directory iterator cannot open directory", p,
- std::error_code(err, std::generic_category())));
-
- ecptr->assign(err, std::generic_category());
}
+ else if (ecptr)
+ *ecptr = ec;
+ else if (ec)
+ _GLIBCXX_THROW_OR_ABORT(fs::filesystem_error(
+ "recursive directory iterator cannot open directory", p, ec));
}
fs::recursive_directory_iterator::~recursive_directory_iterator() = default;
@@ -292,14 +384,14 @@ 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, !follow, ec);
+ _Dir dir = top.open_subdir(skip_permission_denied, !follow, ec);
if (ec)
{
_M_dirs.reset();
return *this;
}
if (dir.dirp)
- _M_dirs->push(std::move(dir));
+ _M_dirs->push(std::move(dir));
}
while (!_M_dirs->top().advance(skip_permission_denied, ec) && !ec)
@@ -362,3 +454,96 @@ fs::recursive_directory_iterator::disable_recursion_pending() noexcept
{
_M_dirs->pending = false;
}
+
+// Used to implement filesystem::remove_all.
+fs::recursive_directory_iterator&
+fs::recursive_directory_iterator::__erase(error_code* ecptr)
+{
+ error_code ec;
+ if (!_M_dirs)
+ {
+ ec = std::make_error_code(errc::invalid_argument);
+ return *this;
+ }
+
+ // We never want to skip permission denied when removing files.
+ const bool skip_permission_denied = false;
+ // We never want to follow directory symlinks when removing files.
+ const bool nofollow = true;
+
+ // Loop until we find something we can remove.
+ while (!ec)
+ {
+ auto& top = _M_dirs->top();
+
+ if (top.entry._M_type == file_type::directory)
+ {
+ _Dir dir = top.open_subdir(skip_permission_denied, nofollow, ec);
+ if (!ec)
+ {
+ __glibcxx_assert(dir.dirp != nullptr);
+ if (dir.advance(skip_permission_denied, ec))
+ {
+ // Non-empty directory, recurse into it.
+ _M_dirs->push(std::move(dir));
+ continue;
+ }
+ if (!ec)
+ {
+ // Directory is empty so we can remove it.
+ if (top.rmdir(ec))
+ break; // Success
+ }
+ }
+ }
+ else if (top.unlink(ec))
+ break; // Success
+ else if (top.entry._M_type == file_type::none)
+ {
+ // We did not have a cached type, so it's possible that top.entry
+ // is actually a directory, and that's why the unlink above failed.
+#ifdef EPERM
+ // POSIX.1-2017 says unlinking a directory returns EPERM,
+ // but LSB allows EISDIR too. Some targets don't even define EPERM.
+ if (ec.value() == EPERM || ec.value() == EISDIR)
+#else
+ if (ec.value() == EISDIR)
+#endif
+ {
+ // Retry, treating it as a directory.
+ top.entry._M_type = file_type::directory;
+ ec.clear();
+ continue;
+ }
+ }
+ }
+
+ if (!ec)
+ {
+ // We successfully removed the current entry, so advance to the next one.
+ if (_M_dirs->top().advance(skip_permission_denied, ec))
+ return *this;
+ else if (!ec)
+ {
+ // Reached the end of the current directory.
+ _M_dirs->pop();
+ if (_M_dirs->empty())
+ _M_dirs.reset();
+ return *this;
+ }
+ }
+
+ // Reset _M_dirs to empty.
+ auto dirs = std::move(_M_dirs);
+
+ // Need to report an error
+ if (ecptr)
+ *ecptr = ec;
+ else
+ _GLIBCXX_THROW_OR_ABORT(fs::filesystem_error("cannot remove all",
+ dirs->orig,
+ dirs->current_path(),
+ ec));
+
+ return *this;
+}
diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc
index 321944d73f7..ae35b0535b3 100644
--- a/libstdc++-v3/src/c++17/fs_ops.cc
+++ b/libstdc++-v3/src/c++17/fs_ops.cc
@@ -1277,105 +1277,62 @@ fs::remove(const path& p, error_code& ec) noexcept
return false;
}
-namespace std::filesystem
-{
-namespace
-{
- struct ErrorReporter
- {
- explicit
- ErrorReporter(error_code& ec) : code(&ec)
- { }
-
- explicit
- ErrorReporter(const char* s, const path& p)
- : code(nullptr), msg(s), path1(&p)
- { }
-
- error_code* code;
- const char* msg;
- const path* path1;
-
- void
- report(const error_code& ec) const
- {
- if (code)
- *code = ec;
- else
- _GLIBCXX_THROW_OR_ABORT(filesystem_error(msg, *path1, ec));
- }
-
- void
- report(const error_code& ec, const path& path2) const
- {
- if (code)
- *code = ec;
- else if (path2 != *path1)
- _GLIBCXX_THROW_OR_ABORT(filesystem_error(msg, *path1, path2, ec));
- else
- _GLIBCXX_THROW_OR_ABORT(filesystem_error(msg, *path1, ec));
- }
- };
-
- uintmax_t
- do_remove_all(const path& p, const ErrorReporter& err)
- {
- error_code ec;
- const auto s = symlink_status(p, ec);
- if (!status_known(s))
- {
- if (ec)
- err.report(ec, p);
- return -1;
- }
-
- ec.clear();
- if (s.type() == file_type::not_found)
- return 0;
-
- uintmax_t count = 0;
- if (s.type() == file_type::directory)
- {
- directory_iterator d(p, directory_options{99}, ec), end;
- while (d != end)
- {
- const auto removed = fs::do_remove_all(d->path(), err);
- if (removed == numeric_limits<uintmax_t>::max())
- return -1;
- count += removed;
-
- d.increment(ec);
- }
- if (ec)
- {
- err.report(ec, p);
- return -1;
- }
- }
-
- if (fs::remove(p, ec))
- ++count;
- if (ec)
- {
- err.report(ec, p);
- return -1;
- }
- return count;
- }
-}
-}
-
std::uintmax_t
fs::remove_all(const path& p)
{
- return fs::do_remove_all(p, ErrorReporter{"cannot remove all", p});
+ uintmax_t count = 0;
+ auto st = filesystem::status(p);
+ if (!exists(st))
+ return 0;
+ if (is_directory(st))
+ {
+ recursive_directory_iterator dir(p, directory_options{64|128}), end;
+ path failed;
+ while (dir != end)
+ {
+ failed = dir->path();
+ dir.__erase();
+ ++count;
+ }
+ }
+ return count + fs::remove(p);
}
std::uintmax_t
fs::remove_all(const path& p, error_code& ec)
{
- ec.clear();
- return fs::do_remove_all(p, ErrorReporter{ec});
+ uintmax_t count = 0;
+ recursive_directory_iterator dir(p, directory_options{64|128}, ec);
+ switch (ec.value())
+ {
+ case 0:
+ {
+ recursive_directory_iterator end;
+ while (dir != end)
+ {
+ dir.__erase(&ec);
+ if (ec)
+ return -1;
+ ++count;
+ }
+ }
+ break;
+ case ENOENT:
+ // Our work here is done.
+ ec.clear();
+ return 0;
+ case ENOTDIR:
+ case ELOOP:
+ // Not a directory, will remove below.
+ break;
+ default:
+ // An error occurred.
+ return -1;
+ }
+ // Remove p itself, which is either a non-directory or is now empty.
+ if (int last = fs::remove(p, ec); !ec)
+ return count + last;
+ return -1;
}
void
diff --git a/libstdc++-v3/src/filesystem/dir-common.h b/libstdc++-v3/src/filesystem/dir-common.h
index 4bfdae4e5a2..ee4f33b6bc1 100644
--- a/libstdc++-v3/src/filesystem/dir-common.h
+++ b/libstdc++-v3/src/filesystem/dir-common.h
@@ -34,11 +34,11 @@
# ifdef _GLIBCXX_HAVE_SYS_TYPES_H
# include <sys/types.h>
# endif
-# include <dirent.h>
-#endif
-#ifdef _GLIBCXX_HAVE_FCNTL_H
-# include <fcntl.h> // O_NOFOLLOW, O_DIRECTORY
-# include <unistd.h> // close
+# include <dirent.h> // opendir, readdir, fdopendir, dirfd
+# ifdef _GLIBCXX_HAVE_FCNTL_H
+# include <fcntl.h> // open, openat, fcntl, AT_FDCWD, O_NOFOLLOW etc.
+# include <unistd.h> // close, unlinkat
+# endif
#endif
namespace std _GLIBCXX_VISIBILITY(default)
@@ -75,42 +75,32 @@ inline int closedir(DIR*) { return -1; }
namespace posix = __gnu_posix;
+inline bool
+is_permission_denied_error(int e)
+{
+ if (e == EACCES)
+ return true;
+#ifdef __APPLE__
+ if (e == EPERM) // See PR 99533
+ return true;
+#endif
+ return false;
+}
+
struct _Dir_base
{
_Dir_base(posix::DIR* dirp = nullptr) : dirp(dirp) { }
// If no error occurs then dirp is non-null,
- // otherwise null (even if an EACCES error is ignored).
- _Dir_base(const posix::char_type* pathname, bool skip_permission_denied,
- [[maybe_unused]] bool nofollow, error_code& ec) noexcept
- : dirp(nullptr)
+ // otherwise null (even if a permission denied error is ignored).
+ _Dir_base(int fd, const posix::char_type* pathname,
+ bool skip_permission_denied, bool nofollow,
+ error_code& ec) noexcept
+ : dirp(_Dir_base::openat(fd, pathname, nofollow))
{
-#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)))
+ if (dirp)
ec.clear();
- else if (errno == EACCES && skip_permission_denied)
+ else if (is_permission_denied_error(errno) && skip_permission_denied)
ec.clear();
else
ec.assign(errno, std::generic_category());
@@ -153,6 +143,16 @@ struct _Dir_base
}
}
+ static constexpr int
+ fdcwd() noexcept
+ {
+#ifdef AT_FDCWD
+ return AT_FDCWD;
+#else
+ return -1; // Use invalid fd if AT_FDCWD isn't supported.
+#endif
+ }
+
static bool is_dot_or_dotdot(const char* s) noexcept
{ return !strcmp(s, ".") || !strcmp(s, ".."); }
@@ -161,21 +161,72 @@ struct _Dir_base
{ return !wcscmp(s, L".") || !wcscmp(s, L".."); }
#endif
+ // Set the close-on-exec flag if not already done via O_CLOEXEC.
+ static bool
+ set_close_on_exec([[maybe_unused]] int fd)
+ {
+#if ! defined O_CLOEXEC && defined FD_CLOEXEC
+ int flags = ::fcntl(fd, F_GETFD);
+ if (flags == -1 || ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC) == -1)
+ return false;
+#endif
+ return true;
+ }
+
+ static ::DIR*
+ openat(int fd, const posix::char_type* pathname, bool nofollow)
+ {
+#if _GLIBCXX_HAVE_FDOPENDIR && defined O_RDONLY && defined O_DIRECTORY \
+ && ! _GLIBCXX_FILESYSTEM_IS_WINDOWS
+
+ // Any file descriptor we open here should be closed on exec.
+#ifdef O_CLOEXEC
+ constexpr int close_on_exec = O_CLOEXEC;
+#else
+ constexpr int close_on_exec = 0;
+#endif
+
+ int flags = O_RDONLY | O_DIRECTORY | close_on_exec;
+
+ // Directory iterators are vulnerable to race conditions unless O_NOFOLLOW
+ // is supported, because a directory could be replaced with a symlink after
+ // checking is_directory(symlink_status(f)). O_NOFOLLOW avoids the race.
+#ifdef O_NOFOLLOW
+ if (nofollow)
+ flags |= O_NOFOLLOW;
+#else
+ nofollow = false;
+#endif
+
+
+#ifdef AT_FDCWD
+ fd = ::openat(fd, pathname, flags);
+#else
+ // If we cannot use openat, there's no benefit to using posix::open unless
+ // we will use O_NOFOLLOW, so just use the simpler posix::opendir.
+ if (!nofollow)
+ return posix::opendir(pathname);
+
+ fd = ::open(pathname, flags);
+#endif
+
+ if (fd == -1)
+ return nullptr;
+ if (set_close_on_exec(fd))
+ if (::DIR* dirp = ::fdopendir(fd))
+ return dirp;
+ int err = errno;
+ ::close(fd);
+ errno = err;
+ return nullptr;
+#else
+ return posix::opendir(pathname);
+#endif
+ }
+
posix::DIR* dirp;
};
-inline bool
-is_permission_denied_error(int e)
-{
- if (e == EACCES)
- return true;
-#ifdef __APPLE__
- if (e == EPERM) // See PR 99533
- return true;
-#endif
- return false;
-}
-
} // namespace filesystem
// BEGIN/END macros must be defined before including this file.
@@ -211,8 +262,6 @@ get_file_type(const std::filesystem::__gnu_posix::dirent& d [[gnu::unused]])
#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 7cf8e62b5e6..e838b4bc6bf 100644
--- a/libstdc++-v3/src/filesystem/dir.cc
+++ b/libstdc++-v3/src/filesystem/dir.cc
@@ -53,7 +53,7 @@ struct fs::_Dir : std::filesystem::_Dir_base
{
_Dir(const fs::path& p, bool skip_permission_denied, bool nofollow,
error_code& ec)
- : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec)
+ : _Dir_base(this->fdcwd(), p.c_str(), skip_permission_denied, nofollow, ec)
{
if (!ec)
path = p;
@@ -113,6 +113,29 @@ struct fs::_Dir : std::filesystem::_Dir_base
return false;
}
+ // Return a file descriptor for the directory and current entry's path.
+ // If dirfd is available, use it and return only the filename.
+ // Otherwise, return AT_FDCWD and return the full path.
+ pair<int, const posix::char_type*>
+ dir_and_pathname() const noexcept
+ {
+ const fs::path& p = entry.path();
+#if _GLIBCXX_HAVE_DIRFD
+ return {::dirfd(this->dirp), std::prev(p.end())->c_str()};
+#endif
+ return {this->fdcwd(), p.c_str()};
+ }
+
+ // Create a new _Dir for the directory this->entry.path().
+ _Dir
+ open_subdir(bool skip_permission_denied, bool nofollow,
+ error_code& ec) noexcept
+ {
+ auto [dirfd, pathname] = dir_and_pathname();
+ _Dir_base d(dirfd, pathname, skip_permission_denied, nofollow, ec);
+ return _Dir(std::exchange(d.dirp, nullptr), entry.path());
+ }
+
fs::path path;
directory_entry entry;
file_type type = file_type::none;
@@ -134,12 +157,9 @@ 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, nofollow, ec);
+ _Dir dir(p, skip_permission_denied, /*nofollow*/false, ec);
if (dir.dirp)
{
@@ -191,6 +211,11 @@ fs::directory_iterator::increment(error_code& ec) noexcept
struct fs::recursive_directory_iterator::_Dir_stack : std::stack<_Dir>
{
+ _Dir_stack(_Dir&& dir)
+ {
+ this->push(std::move(dir));
+ }
+
void clear() { c.clear(); }
};
@@ -199,33 +224,27 @@ recursive_directory_iterator(const path& p, directory_options options,
error_code* ecptr)
: _M_options(options), _M_pending(true)
{
- if (posix::DIR* dirp = posix::opendir(p.c_str()))
+ // Do not report an error for permission denied errors.
+ const bool skip_permission_denied
+ = is_set(options, directory_options::skip_permission_denied);
+
+ error_code ec;
+ _Dir dir(p, skip_permission_denied, /*nofollow*/false, ec);
+
+ if (dir.dirp)
{
- if (ecptr)
- ecptr->clear();
- auto sp = std::make_shared<_Dir_stack>();
- sp->push(_Dir{ dirp, p });
- if (ecptr ? sp->top().advance(*ecptr) : sp->top().advance())
- _M_dirs.swap(sp);
- }
- else
- {
- const int err = errno;
- if (std::filesystem::is_permission_denied_error(err)
- && is_set(options, fs::directory_options::skip_permission_denied))
+ auto sp = std::__make_shared<_Dir_stack>(std::move(dir));
+ if (ecptr ? sp->top().advance(skip_permission_denied, *ecptr)
+ : sp->top().advance(skip_permission_denied))
{
- if (ecptr)
- ecptr->clear();
- return;
+ _M_dirs.swap(sp);
}
-
- if (!ecptr)
- _GLIBCXX_THROW_OR_ABORT(filesystem_error(
- "recursive directory iterator cannot open directory", p,
- std::error_code(err, std::generic_category())));
-
- ecptr->assign(err, std::generic_category());
}
+ else if (ecptr)
+ *ecptr = ec;
+ else if (ec)
+ _GLIBCXX_THROW_OR_ABORT(fs::filesystem_error(
+ "recursive directory iterator cannot open directory", p, ec));
}
fs::recursive_directory_iterator::~recursive_directory_iterator() = default;
@@ -279,7 +298,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, !follow, ec);
+ _Dir dir = top.open_subdir(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 cd98caf73f2..c020f621a88 100644
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -63,6 +63,8 @@
#define _GLIBCXX_END_NAMESPACE_FILESYSTEM } }
#include "ops-common.h"
+#include <filesystem> // std::filesystem::remove_all
+
namespace fs = std::experimental::filesystem;
namespace posix = std::filesystem::__gnu_posix;
@@ -1098,33 +1100,8 @@ fs::remove_all(const path& p)
std::uintmax_t
fs::remove_all(const path& p, error_code& ec) noexcept
{
- const auto s = symlink_status(p, ec);
- if (!status_known(s))
- return -1;
-
- ec.clear();
- if (s.type() == file_type::not_found)
- return 0;
-
- uintmax_t count = 0;
- if (s.type() == file_type::directory)
- {
- directory_iterator d(p, directory_options{99}, ec), end;
- while (!ec && d != end)
- {
- const auto removed = fs::remove_all(d->path(), ec);
- if (removed == numeric_limits<uintmax_t>::max())
- return -1;
- count += removed;
- d.increment(ec);
- }
- if (ec)
- return -1;
- }
-
- if (fs::remove(p, ec))
- ++count;
- return ec ? -1 : count;
+ // Use the C++17 implementation.
+ return std::filesystem::remove_all(p.native(), ec);
}
void
--
2.34.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [committed] libstdc++: Fix filesystem::remove_all races [PR104161]
2022-02-04 23:54 [committed] libstdc++: Fix filesystem::remove_all races [PR104161] Jonathan Wakely
@ 2022-02-05 1:08 ` Jonathan Wakely
2022-02-08 14:01 ` [committed] libstdc++: Fix filesystem::remove_all for Windows [PR104161] Jonathan Wakely
0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Wakely @ 2022-02-05 1:08 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc Patches
On Fri, 4 Feb 2022 at 23:55, Jonathan Wakely wrote:
> +// Used to implement filesystem::remove_all.
> +fs::recursive_directory_iterator&
> +fs::recursive_directory_iterator::__erase(error_code* ecptr)
> +{
> + error_code ec;
> + if (!_M_dirs)
> + {
> + ec = std::make_error_code(errc::invalid_argument);
> + return *this;
> + }
> +
> + // We never want to skip permission denied when removing files.
> + const bool skip_permission_denied = false;
> + // We never want to follow directory symlinks when removing files.
> + const bool nofollow = true;
> +
> + // Loop until we find something we can remove.
> + while (!ec)
> + {
> + auto& top = _M_dirs->top();
> +
> + if (top.entry._M_type == file_type::directory)
> + {
> + _Dir dir = top.open_subdir(skip_permission_denied, nofollow, ec);
> + if (!ec)
> + {
> + __glibcxx_assert(dir.dirp != nullptr);
> + if (dir.advance(skip_permission_denied, ec))
> + {
> + // Non-empty directory, recurse into it.
> + _M_dirs->push(std::move(dir));
> + continue;
> + }
> + if (!ec)
> + {
> + // Directory is empty so we can remove it.
> + if (top.rmdir(ec))
> + break; // Success
> + }
> + }
> + }
> + else if (top.unlink(ec))
> + break; // Success
> + else if (top.entry._M_type == file_type::none)
> + {
> + // We did not have a cached type, so it's possible that top.entry
> + // is actually a directory, and that's why the unlink above failed.
> +#ifdef EPERM
> + // POSIX.1-2017 says unlinking a directory returns EPERM,
> + // but LSB allows EISDIR too. Some targets don't even define EPERM.
> + if (ec.value() == EPERM || ec.value() == EISDIR)
> +#else
> + if (ec.value() == EISDIR)
> +#endif
This doesn't work on Windows because the top.unlink(ec) sets a Windows
error using the system category, so doesn't match the errno values
here.
I have a fix.
> std::uintmax_t
> fs::remove_all(const path& p)
> {
> - return fs::do_remove_all(p, ErrorReporter{"cannot remove all", p});
> + uintmax_t count = 0;
> + auto st = filesystem::status(p);
> + if (!exists(st))
> + return 0;
> + if (is_directory(st))
Gah, this remove_all(const path&) overload was supposed to be using
the same logic as the one below with an error_code parameter.
I'll fix it on Monday.
^ permalink raw reply [flat|nested] 3+ messages in thread
* [committed] libstdc++: Fix filesystem::remove_all for Windows [PR104161]
2022-02-05 1:08 ` Jonathan Wakely
@ 2022-02-08 14:01 ` Jonathan Wakely
0 siblings, 0 replies; 3+ messages in thread
From: Jonathan Wakely @ 2022-02-08 14:01 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc Patches
[-- Attachment #1: Type: text/plain, Size: 2846 bytes --]
On Sat, 5 Feb 2022 at 01:08, Jonathan Wakely wrote:
>
> On Fri, 4 Feb 2022 at 23:55, Jonathan Wakely wrote:
> > +// Used to implement filesystem::remove_all.
> > +fs::recursive_directory_iterator&
> > +fs::recursive_directory_iterator::__erase(error_code* ecptr)
> > +{
> > + error_code ec;
> > + if (!_M_dirs)
> > + {
> > + ec = std::make_error_code(errc::invalid_argument);
> > + return *this;
> > + }
> > +
> > + // We never want to skip permission denied when removing files.
> > + const bool skip_permission_denied = false;
> > + // We never want to follow directory symlinks when removing files.
> > + const bool nofollow = true;
> > +
> > + // Loop until we find something we can remove.
> > + while (!ec)
> > + {
> > + auto& top = _M_dirs->top();
> > +
> > + if (top.entry._M_type == file_type::directory)
> > + {
> > + _Dir dir = top.open_subdir(skip_permission_denied, nofollow, ec);
> > + if (!ec)
> > + {
> > + __glibcxx_assert(dir.dirp != nullptr);
> > + if (dir.advance(skip_permission_denied, ec))
> > + {
> > + // Non-empty directory, recurse into it.
> > + _M_dirs->push(std::move(dir));
> > + continue;
> > + }
> > + if (!ec)
> > + {
> > + // Directory is empty so we can remove it.
> > + if (top.rmdir(ec))
> > + break; // Success
> > + }
> > + }
> > + }
> > + else if (top.unlink(ec))
> > + break; // Success
> > + else if (top.entry._M_type == file_type::none)
> > + {
> > + // We did not have a cached type, so it's possible that top.entry
> > + // is actually a directory, and that's why the unlink above failed.
> > +#ifdef EPERM
> > + // POSIX.1-2017 says unlinking a directory returns EPERM,
> > + // but LSB allows EISDIR too. Some targets don't even define EPERM.
> > + if (ec.value() == EPERM || ec.value() == EISDIR)
> > +#else
> > + if (ec.value() == EISDIR)
> > +#endif
>
> This doesn't work on Windows because the top.unlink(ec) sets a Windows
> error using the system category, so doesn't match the errno values
> here.
>
> I have a fix.
>
> > std::uintmax_t
> > fs::remove_all(const path& p)
> > {
> > - return fs::do_remove_all(p, ErrorReporter{"cannot remove all", p});
> > + uintmax_t count = 0;
> > + auto st = filesystem::status(p);
> > + if (!exists(st))
> > + return 0;
> > + if (is_directory(st))
>
> Gah, this remove_all(const path&) overload was supposed to be using
> the same logic as the one below with an error_code parameter.
>
> I'll fix it on Monday.
Here's that fix.
Tested x86_64-linux, powerpc-aix, x86_64-w64-mingw.
Pushed to trunk.
[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 6377 bytes --]
commit 5750952bec1e632d1f804f4a1bed2f74c0f3b189
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Mon Feb 7 23:36:47 2022
libstdc++: Fix filesystem::remove_all for Windows [PR104161]
The recursive_directory_iterator::__erase member was failing for
Windows, because the entry._M_type value is always file_type::none
(because _Dir_base::advance doesn't populate it for Windows) and
top.unlink uses fs::remove which sets an error using the
system_category. That meant that ec.value() was a Windows error code and
not an errno value, so the comparisons to EPERM and EISDIR failed.
Instead of depending on a specific Windows error code for attempting to
remove a directory, just use directory_entry::refresh() to query the
type first. This doesn't avoid the TOCTTOU races with directory
symlinks, but we can't avoid them on Windows without openat and
unlinkat, and creating symlinks requires admin privs on Windows anyway.
This also fixes the fs::remove_all(const path&) overload, which was
supposed to use the same logic as the other overload, but I forgot to
change it before my previous commit.
libstdc++-v3/ChangeLog:
PR libstdc++/104161
* src/c++17/fs_dir.cc (fs::recursive_directory_iterator::__erase):
[i_GLIBCXX_FILESYSTEM_IS_WINDOWS]: Refresh entry._M_type member,
instead of checking for errno values indicating a directory.
* src/c++17/fs_ops.cc (fs::remove_all(const path&)): Use similar
logic to non-throwing overload.
(fs::remove_all(const path&, error_code&)): Add comments.
* src/filesystem/ops-common.h: Likewise.
diff --git a/libstdc++-v3/src/c++17/fs_dir.cc b/libstdc++-v3/src/c++17/fs_dir.cc
index 01b8c0d5693..54f135d2baf 100644
--- a/libstdc++-v3/src/c++17/fs_dir.cc
+++ b/libstdc++-v3/src/c++17/fs_dir.cc
@@ -476,6 +476,16 @@ fs::recursive_directory_iterator::__erase(error_code* ecptr)
{
auto& top = _M_dirs->top();
+#if _GLIBCXX_FILESYSTEM_IS_WINDOWS
+ // _Dir::unlink uses fs::remove which uses std::system_category() for
+ // Windows errror codes, so we can't just check for EPERM and EISDIR.
+ // Use directory_entry::refresh() here to check if we have a directory.
+ // This can be a TOCTTOU race, but we don't have openat or unlinkat to
+ // solve that on Windows, and generally don't support symlinks anyway.
+ if (top.entry._M_type == file_type::none)
+ top.entry.refresh();
+#endif
+
if (top.entry._M_type == file_type::directory)
{
_Dir dir = top.open_subdir(skip_permission_denied, nofollow, ec);
@@ -498,12 +508,13 @@ fs::recursive_directory_iterator::__erase(error_code* ecptr)
}
else if (top.unlink(ec))
break; // Success
+#if ! _GLIBCXX_FILESYSTEM_IS_WINDOWS
else if (top.entry._M_type == file_type::none)
{
// We did not have a cached type, so it's possible that top.entry
// is actually a directory, and that's why the unlink above failed.
#ifdef EPERM
- // POSIX.1-2017 says unlinking a directory returns EPERM,
+ // POSIX.1-2017 says unlink on a directory returns EPERM,
// but LSB allows EISDIR too. Some targets don't even define EPERM.
if (ec.value() == EPERM || ec.value() == EISDIR)
#else
@@ -516,6 +527,7 @@ fs::recursive_directory_iterator::__erase(error_code* ecptr)
continue;
}
}
+#endif
}
if (!ec)
diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc
index ae35b0535b3..4552a730bf2 100644
--- a/libstdc++-v3/src/c++17/fs_ops.cc
+++ b/libstdc++-v3/src/c++17/fs_ops.cc
@@ -1280,21 +1280,36 @@ fs::remove(const path& p, error_code& ec) noexcept
std::uintmax_t
fs::remove_all(const path& p)
{
+ error_code ec;
uintmax_t count = 0;
- auto st = filesystem::status(p);
- if (!exists(st))
- return 0;
- if (is_directory(st))
+ recursive_directory_iterator dir(p, directory_options{64|128}, ec);
+ switch (ec.value()) // N.B. assumes ec.category() == std::generic_category()
+ {
+ case 0:
+ // Iterate over the directory removing everything.
{
- recursive_directory_iterator dir(p, directory_options{64|128}), end;
- path failed;
+ const recursive_directory_iterator end;
while (dir != end)
{
- failed = dir->path();
- dir.__erase();
+ dir.__erase(); // throws on error
++count;
}
}
+ // Directory is empty now, will remove it below.
+ break;
+ case ENOENT:
+ // Our work here is done.
+ return 0;
+ case ENOTDIR:
+ case ELOOP:
+ // Not a directory, will remove below.
+ break;
+ default:
+ // An error occurred.
+ _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove all", p, ec));
+ }
+
+ // Remove p itself, which is either a non-directory or is now empty.
return count + fs::remove(p);
}
@@ -1303,11 +1318,12 @@ fs::remove_all(const path& p, error_code& ec)
{
uintmax_t count = 0;
recursive_directory_iterator dir(p, directory_options{64|128}, ec);
- switch (ec.value())
+ switch (ec.value()) // N.B. assumes ec.category() == std::generic_category()
{
case 0:
+ // Iterate over the directory removing everything.
{
- recursive_directory_iterator end;
+ const recursive_directory_iterator end;
while (dir != end)
{
dir.__erase(&ec);
@@ -1316,6 +1332,7 @@ fs::remove_all(const path& p, error_code& ec)
++count;
}
}
+ // Directory is empty now, will remove it below.
break;
case ENOENT:
// Our work here is done.
@@ -1329,6 +1346,7 @@ fs::remove_all(const path& p, error_code& ec)
// An error occurred.
return -1;
}
+
// Remove p itself, which is either a non-directory or is now empty.
if (int last = fs::remove(p, ec); !ec)
return count + last;
diff --git a/libstdc++-v3/src/filesystem/ops-common.h b/libstdc++-v3/src/filesystem/ops-common.h
index 2aa9b571230..978e8724154 100644
--- a/libstdc++-v3/src/filesystem/ops-common.h
+++ b/libstdc++-v3/src/filesystem/ops-common.h
@@ -63,6 +63,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__last_system_error() noexcept
{
#ifdef _GLIBCXX_FILESYSTEM_IS_WINDOWS
+ // N.B. use error_code::default_error_condition() to convert to generic.
return {(int)::GetLastError(), std::system_category()};
#else
return {errno, std::generic_category()};
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-08 14:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-04 23:54 [committed] libstdc++: Fix filesystem::remove_all races [PR104161] Jonathan Wakely
2022-02-05 1:08 ` Jonathan Wakely
2022-02-08 14:01 ` [committed] libstdc++: Fix filesystem::remove_all for Windows [PR104161] Jonathan Wakely
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).