On 05/01/18 18:02 +0000, Jonathan Wakely wrote:
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.
In Bugzilla the reporter pointed out that the call to symlink_status
in filesystem::remove is now redundant, because handling the ENOENT
case after calling ::remove gives the same result (and is less
vulnerable to TOCTTOU races).
This removes the symlink_status call, and then makes remove_all call
filesystem::remove again instead of ::remove, because doing so doesn't
add an unnecessary symlink_status call now.
Tested powerpc64le-linux, committed to trunk.
commit 89cfa63335230a0ce82152171cdbd2bb5be64440
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Fri Jan 5 23:28:06 2018 +0000
PR libstdc++/83626 simplify filesystem::remove and filesystem::remove_all
PR libstdc++/83626
* src/filesystem/ops.cc (remove(const path&, error_code&)): Remove
unnecessary symlink_status call.
(remove_all(const path&, error_code&)): Use filesystem::remove.
* src/filesystem/std-ops.cc: Likewise.
diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc
index 3690fb94d63..899defea6d2 100644
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -1017,14 +1017,6 @@ fs::remove(const path& p)
bool
fs::remove(const path& p, error_code& ec) noexcept
{
- const auto s = symlink_status(p, ec);
- if (!status_known(s))
- return false;
- if (s.type() == file_type::not_found)
- {
- ec.clear();
- return false; // Nothing to do, not an error.
- }
if (::remove(p.c_str()) == 0)
{
ec.clear();
@@ -1070,14 +1062,9 @@ fs::remove_all(const path& p, error_code& ec) noexcept
return -1;
}
- if (::remove(p.c_str()) == 0)
+ if (fs::remove(p, ec))
++count;
- else if (errno != ENOENT)
- {
- ec.assign(errno, std::generic_category());
- return -1;
- }
- return count;
+ return ec ? -1 : count;
}
void
diff --git a/libstdc++-v3/src/filesystem/std-ops.cc b/libstdc++-v3/src/filesystem/std-ops.cc
index bed1ad1fe27..65b06f5b67b 100644
--- a/libstdc++-v3/src/filesystem/std-ops.cc
+++ b/libstdc++-v3/src/filesystem/std-ops.cc
@@ -1254,7 +1254,7 @@ bool
fs::remove(const path& p)
{
error_code ec;
- bool result = fs::remove(p, ec);
+ const bool result = fs::remove(p, ec);
if (ec)
_GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove", p, ec));
return result;
@@ -1263,14 +1263,6 @@ fs::remove(const path& p)
bool
fs::remove(const path& p, error_code& ec) noexcept
{
- const auto s = symlink_status(p, ec);
- if (!status_known(s))
- return false;
- if (s.type() == file_type::not_found)
- {
- ec.clear();
- return false; // Nothing to do, not an error.
- }
if (::remove(p.c_str()) == 0)
{
ec.clear();
@@ -1316,14 +1308,9 @@ fs::remove_all(const path& p, error_code& ec)
return -1;
}
- if (::remove(p.c_str()) == 0)
+ if (fs::remove(p, ec))
++count;
- else if (errno != ENOENT)
- {
- ec.assign(errno, std::generic_category());
- return -1;
- }
- return count;
+ return ec ? -1 : count;
}
void
