This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] PR libstdc++/83626 Don't throw for remove("") and remove_all("")


On 05/01/18 10:37 +0000, Jonathan Wakely wrote:
On 04/01/18 21:02 -0500, Tim Song wrote:
What if the file to be removed is externally removed between the
symlink_status and the ::remove call? This is probably QoI because
filesystem race, but it seems reasonable to double check errno if
::remove fails and not fail if the failure is due to the file not
existing.

Yes, the race makes it undefined, but checking for ENOENT seems
reasonable. Thanks for the suggestion.

This makes remove and remove_all handle (some) filesystem races, by
ignoring ENOENT errors when removing entries or while iterating over
sub-directories.

It also avoids redundant stat() calls in remove_all, and fixes a bug
in the return value of the throwing version of remove_all.

Tested powerpc64le-linux, committed to trunk.

I've attached a multithreaded test I used to test the handling of
filesystem races, but I'm not committing that test.



commit 64a3b77dd050d16069061185c3efef020698fca4
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Jan 5 16:39:46 2018 +0000

    PR libstdc++/83626 handle ENOENT due to filesystem race
    
            PR libstdc++/83626
            * src/filesystem/ops.cc (remove(const path&, error_code&)): Do not
            report an error for ENOENT.
            (remove_all(const path&)): Fix type of result variable.
            (remove_all(const path&, error_code&)): Use non-throwing increment
            for directory iterator. Call POSIX remove directly to avoid redundant
            calls to symlink_status. Do not report errors for ENOENT.
            * src/filesystem/std-ops.cc: Likewise.
            * testsuite/27_io/filesystem/operations/remove_all.cc: Test throwing
            overload.
            * testsuite/experimental/filesystem/operations/remove_all.cc:
            Likewise.

diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc
index 5a7cb599bb6..3690fb94d63 100644
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -1025,13 +1025,16 @@ fs::remove(const path& p, error_code& ec) noexcept
       ec.clear();
       return false; // Nothing to do, not an error.
     }
-  if (::remove(p.c_str()) != 0)
+  if (::remove(p.c_str()) == 0)
     {
-      ec.assign(errno, std::generic_category());
-      return false;
+      ec.clear();
+      return true;
     }
-  ec.clear();
-  return true;
+  else if (errno == ENOENT)
+    ec.clear();
+  else
+    ec.assign(errno, std::generic_category());
+  return false;
 }
 
 
@@ -1039,7 +1042,7 @@ std::uintmax_t
 fs::remove_all(const path& p)
 {
   error_code ec;
-  bool result = remove_all(p, ec);
+  const auto result = remove_all(p, ec);
   if (ec)
     _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove all", p, ec));
   return result;
@@ -1051,15 +1054,29 @@ 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)
-    for (directory_iterator d(p, ec), end; !ec && d != end; ++d)
-      count += fs::remove_all(d->path(), ec);
-  if (!ec && fs::remove(p, ec))
+    {
+      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 (::remove(p.c_str()) == 0)
     ++count;
-  if (ec)
-    return -1;
+  else if (errno != ENOENT)
+    {
+      ec.assign(errno, std::generic_category());
+      return -1;
+    }
   return count;
 }
 
diff --git a/libstdc++-v3/src/filesystem/std-ops.cc b/libstdc++-v3/src/filesystem/std-ops.cc
index 6ff280a9c76..2411bbb7977 100644
--- a/libstdc++-v3/src/filesystem/std-ops.cc
+++ b/libstdc++-v3/src/filesystem/std-ops.cc
@@ -1274,13 +1274,16 @@ fs::remove(const path& p, error_code& ec) noexcept
       ec.clear();
       return false; // Nothing to do, not an error.
     }
-  if (::remove(p.c_str()) != 0)
+  if (::remove(p.c_str()) == 0)
     {
-      ec.assign(errno, std::generic_category());
-      return false;
+      ec.clear();
+      return true;
     }
-  ec.clear();
-  return true;
+  else if (errno == ENOENT)
+    ec.clear();
+  else
+    ec.assign(errno, std::generic_category());
+  return false;
 }
 
 
@@ -1288,7 +1291,7 @@ std::uintmax_t
 fs::remove_all(const path& p)
 {
   error_code ec;
-  bool result = remove_all(p, ec);
+  const auto result = remove_all(p, ec);
   if (ec)
     _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove all", p, ec));
   return result;
@@ -1300,15 +1303,29 @@ 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)
-      count += fs::remove_all(d->path(), ec);
-  if (!ec && fs::remove(p, ec))
+    {
+      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 (::remove(p.c_str()) == 0)
     ++count;
-  if (ec)
-    return -1;
+  else if (errno != ENOENT)
+    {
+      ec.assign(errno, std::generic_category());
+      return -1;
+    }
   return count;
 }
 
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 8ee5c4d39c1..633cde57243 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc
@@ -85,8 +85,28 @@ test01()
   b2.path.clear();
 }
 
+void
+test02()
+{
+  const auto dir = __gnu_test::nonexistent_path();
+  create_directories(dir/"a/b/c");
+  std::uintmax_t n = remove_all(dir/"a");
+  VERIFY( n == 3 );
+  VERIFY( exists(dir) );
+  VERIFY( !exists(dir/"a") );
+
+  n = remove_all(dir/"a");
+  VERIFY( n == 0 );
+  VERIFY( exists(dir) );
+
+  n = remove_all(dir);
+  VERIFY( n == 1 );
+  VERIFY( !exists(dir) );
+}
+
 int
 main()
 {
   test01();
+  test02();
 }
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc
index fa146b4317c..67f6e989d27 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc
@@ -85,8 +85,28 @@ test01()
   b2.path.clear();
 }
 
+void
+test02()
+{
+  const auto dir = __gnu_test::nonexistent_path();
+  create_directories(dir/"a/b/c");
+  std::uintmax_t n = remove_all(dir/"a");
+  VERIFY( n == 3 );
+  VERIFY( exists(dir) );
+  VERIFY( !exists(dir/"a") );
+
+  n = remove_all(dir/"a");
+  VERIFY( n == 0 );
+  VERIFY( exists(dir) );
+
+  n = remove_all(dir);
+  VERIFY( n == 1 );
+  VERIFY( !exists(dir) );
+}
+
 int
 main()
 {
   test01();
+  test02();
 }

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]