libstdc++: Fix error handling in filesystem::remove_all (PR93201)

When recursing into a directory, any errors that occur while removing a
directory entry are ignored, because the subsequent increment of the
directory iterator clears the error_code object.

This fixes that bug by checking the result of each recursive operation
before incrementing. This is a change in observable behaviour, because
previously other directory entries would still be removed even if one
(or more) couldn't be removed due to errors. Now the operation stops on
the first error, which is what the code intended to do all along. The
standard doesn't specify what happens in this case (because the order
that the entries are processed is unspecified anyway).

It also improves the error reporting so that the name of the file that
could not be removed is included in the filesystem_error exception. This
is done by introducing a new helper type for reporting errors with
additional context and a new function that uses that type. Then the
overload of std::filesystem::remove_all that throws an exception can use
the new function to ensure any exception contains the additional
information.

For std::experimental::filesystem::remove_all just fix the bug where
errors are ignored.

	PR libstdc++/93201
	* src/c++17/fs_ops.cc (do_remove_all): New function implementing more
	detailed error reporting for remove_all. Check result of recursive
	call before incrementing iterator.
	(remove_all(const path&), remove_all(const path&, error_code&)): Use
	do_remove_all.
	* src/filesystem/ops.cc (remove_all(const path&, error_code&)): Check
	result of recursive call before incrementing iterator.
	* testsuite/27_io/filesystem/operations/remove_all.cc: Check errors
	are reported correctly.
	* testsuite/experimental/filesystem/operations/remove_all.cc: Likewise.

From-SVN: r280014

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index 98449aadf5e010b9f8ddb63f9e8f494dad41c4a2..459759850d4c068c123f21184842988788f0baf2 100644 (file)
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -1,3 +1,17 @@
+2020-01-08  Jonathan Wakely  <jwakely@redhat.com>
+
+       PR libstdc++/93201
+       * src/c++17/fs_ops.cc (do_remove_all): New function implementing more
+       detailed error reporting for remove_all. Check result of recursive
+       call before incrementing iterator.
+       (remove_all(const path&), remove_all(const path&, error_code&)): Use
+       do_remove_all.
+       * src/filesystem/ops.cc (remove_all(const path&, error_code&)): Check
+       result of recursive call before incrementing iterator.
+       * testsuite/27_io/filesystem/operations/remove_all.cc: Check errors
+       are reported correctly.
+       * testsuite/experimental/filesystem/operations/remove_all.cc: Likewise.
+
 2020-01-07  Thomas Rodgers  <trodgers@redhat.com>
 
        * include/std/condition_variable

diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc
index 8ad2e7fce1faff0bd6f77e5ced22c39d4c10c14d..873f93aacfc4cf5eebd3ec02df3cd0d113a0ad1a 100644 (file)
--- a/libstdc++-v3/src/c++17/fs_ops.cc
+++ b/libstdc++-v3/src/c++17/fs_ops.cc
@@ -1275,42 +1275,105 @@ 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, 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)
 {
-  error_code ec;
-  const auto result = remove_all(p, ec);
-  if (ec)
-    _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove all", p, ec));
-  return result;
+  return fs::do_remove_all(p, ErrorReporter{"cannot remove all", p});
 }
 
 std::uintmax_t
 fs::remove_all(const path& p, error_code& ec)
 {
-  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)
-    {
-      for (directory_iterator d(p, ec), end; !ec && d != end; d.increment(ec))
-       count += fs::remove_all(d->path(), ec);
-      if (ec.value() == ENOENT)
-       ec.clear();
-      else if (ec)
-       return -1;
-    }
-
-  if (fs::remove(p, ec))
-    ++count;
-  return ec ? -1 : count;
+  return fs::do_remove_all(p, ErrorReporter{ec});
 }
 
 void

diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc
index 79dc0fc6511dbc4b222f08fecb6f2b7b5c094695..29ea9c0ce87536655bf66d614f7552d50982cd4a 100644 (file)
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -1099,12 +1099,17 @@ fs::remove_all(const path& p, error_code& ec) noexcept
   uintmax_t count = 0;
   if (s.type() == file_type::directory)
     {
-      for (directory_iterator d(p, ec), end; !ec && d != end; d.increment(ec))
-       count += fs::remove_all(d->path(), ec);
-      if (ec.value() == ENOENT)
-       ec.clear();
-      else if (ec)
-       return -1;
+      directory_iterator d(p, 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))

diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc
index 5b920e490cb264607687fcc4bc30bf9c8eee0ef7..7e018b51af26bbeade592f0be5b5390a999de10c 100644 (file)
--- a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc
@@ -140,10 +140,45 @@ test03()
   VERIFY( !exists(p) );
 }
 
+void
+test04()
+{
+#if defined(__MINGW32__) || defined(__MINGW64__)
+  // no permissions
+#else
+  // PR libstdc++/93201
+  std::error_code ec;
+  std::uintmax_t n;
+
+  auto dir = __gnu_test::nonexistent_path();
+  fs::create_directory(dir);
+  __gnu_test::scoped_file f(dir/"file");
+  // remove write permission on the directory:
+  fs::permissions(dir, fs::perms::owner_read|fs::perms::owner_exec);
+  n = fs::remove_all(dir, ec);
+  VERIFY( n == -1 );
+  VERIFY( ec == std::errc::permission_denied ); // not ENOTEMPTY
+
+  try {
+    fs::remove_all(dir);
+    VERIFY( false );
+  } catch (const fs::filesystem_error& e) {
+    VERIFY( e.code() == std::errc::permission_denied );
+    // First path is the argument to remove_all
+    VERIFY( e.path1() == dir );
+    // Second path is the first file that couldn't be removed
+    VERIFY( e.path2() == dir/"file" );
+  }
+
+  fs::permissions(dir, fs::perms::owner_write, fs::perm_options::add);
+#endif
+}
+
 int
 main()
 {
   test01();
   test02();
   test03();
+  test04();
 }

diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc
index 9e46c762f88f22eaefb7fa59246b61f5e1b5b337..0e2aedae96dbb75fd444a9dfe82bc6fe19c9110d 100644 (file)
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc
@@ -108,9 +108,42 @@ test02()
   VERIFY( !exists(dir) );
 }
 
+void
+test04()
+{
+#if defined(__MINGW32__) || defined(__MINGW64__)
+  // no permissions
+#else
+  // PR libstdc++/93201
+  std::error_code ec;
+  std::uintmax_t n;
+
+  auto dir = __gnu_test::nonexistent_path();
+  fs::create_directory(dir);
+  __gnu_test::scoped_file f(dir/"file");
+  // remove write permission on the directory:
+  fs::permissions(dir, fs::perms::owner_read|fs::perms::owner_exec);
+  n = fs::remove_all(dir, ec);
+  VERIFY( n == -1 );
+  VERIFY( ec == std::errc::permission_denied ); // not ENOTEMPTY
+
+  try {
+    fs::remove_all(dir);
+    VERIFY( false );
+  } catch (const fs::filesystem_error& e) {
+    VERIFY( e.code() == std::errc::permission_denied );
+    // First path is the argument to remove_all
+    VERIFY( e.path1() == dir );
+  }
+
+  fs::permissions(dir, fs::perms::owner_write|fs::perms::add_perms);
+#endif
+}
+
 int
 main()
 {
   test01();
   test02();
+  test04();
 }