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

Message ID 20200108164434.GA852711@redhat.com
State New
Headers show
Series
  • libstdc++: Fix error handling in filesystem::remove_all (PR93201)
Related show

Commit Message

Jonathan Wakely Jan. 8, 2020, 4:44 p.m.
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.

Tested powerpc64le-linux, committed to trunk.
commit b89525a0036c9b9f6d3d6952b54624fca27d9774
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Jan 8 16:23:07 2020 +0000

    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.

Comments

Jonathan Wakely Jan. 8, 2020, 5 p.m. | #1
On 08/01/20 16:44 +0000, Jonathan Wakely wrote:
>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.


This is what I plan to commit to the branches. It doesn't improve the
error reporting, just fixes the bug. But that *does* still change the
observable behaviour when an error occurs (because now we return
immediately on the first error, instead of continuing to remove as
much as possible).

I've sent an email to the Library Evolution list to discuss the
desired behaviour. Does anybody here have an opinion? Should we
preserve the existing "keep going then report errors at the end"
behaviour for the branches, or just make this change?
commit 43899ee8cc95620b0d93560d40899997ce3cc76d
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Jan 8 16:23:07 2020 +0000

    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.

diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc
index d8064819d36..d918c2af530 100644
--- 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 36b5d2c24f6..a5887f37ce1 100644
--- 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 a19bac9c5f6..b0b176fc656 100644
--- 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 99fb14a71d7..9d51a66c71f 100644
--- 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();
 }

Patch

diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc
index 8ad2e7fce1f..873f93aacfc 100644
--- 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 79dc0fc6511..29ea9c0ce87 100644
--- 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 5b920e490cb..7e018b51af2 100644
--- 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 9e46c762f88..0e2aedae96d 100644
--- 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();
 }