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).

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

From-SVN: r280020

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index 3b5f453b04d6e51701f0ffc728c20c1bb59e2675..0180d7737cb68c8f9e141bcdb4e9c4f64a312b0b 100644 (file)
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -1,3 +1,14 @@
+2020-01-08  Jonathan Wakely  <jwakely@redhat.com>
+
+       PR libstdc++/93201
+       * src/c++17/fs_ops.cc (remove_all(const path&, error_code&)): Check
+       result of recursive call before incrementing iterator.
+       * src/filesystem/ops.cc (remove_all(const path&, error_code&)):
+       Likewise.
+       * testsuite/27_io/filesystem/operations/remove_all.cc: Check errors
+       are reported correctly.
+       * testsuite/experimental/filesystem/operations/remove_all.cc: Likewise.
+
 2019-12-11  Jonathan Wakely  <jwakely@redhat.com>
 
        Backport from mainline

diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc
index d8064819d3611e85cd312b69ec24ec1d859e6ea8..d918c2af53089e78258d7a55f1f096c5b3386b15 100644 (file)
--- a/libstdc++-v3/src/c++17/fs_ops.cc
+++ b/libstdc++-v3/src/c++17/fs_ops.cc
@@ -1299,12 +1299,17 @@ fs::remove_all(const path& p, error_code& ec)
   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/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc
index 36b5d2c24f645c86feca02e92d44c6bbde889d30..a5887f37ce1e0e1cc8efc9266ee2b63351c2382c 100644 (file)
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -1098,12 +1098,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 a19bac9c5f6bc4f45ca0fe42e3023df42b4b2eae..b0b176fc656475acf2b04bff5542992984d4483c 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,43 @@ 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 );
+  }
+
+  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 99fb14a71d723f9ac4d59797cad36c3ec35b63b1..9d51a66c71f9c961c5ec2c3a2dc614cf92068200 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();
 }