Repository navigation
[libc++] Fix filesystem::remove_all bug with read-only nested directory - #197104
Conversation
llvmorg-github-actions
Bot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Jake Egan (jakeegan) ChangesConsider a directory During the recursive removal, According to the standard [fs.op.remove.all]: The word "then" implies an ordering where deleting the contents of p should be successful before deleting file p itself. Fixes #197100. 1 Files Affected:
diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index 745db87ce3736..6afcec07b374d 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -904,6 +904,9 @@ uintmax_t remove_all_impl(int parent_directory, const path& p, error_code& ec) {
break; // we're done iterating through the directory
} else {
count += remove_all_impl(fd, str, ec);
+ // If there's an error removing the child, return immediately to preserve the error code.
+ if (ec)
+ return count;
}
}
|
hubert-reinterpretcast
left a comment
hubert-reinterpretcast
left a comment
There was a problem hiding this comment.
Please add a test.
|
Do you have an idea what that test would look like? I didn't add a test because I'm not sure there's a way to verify this change without checking the specific error code that is returned, which can vary by platform. This change is ensuring the first encountered error is returned, but I don't think the standard mandates specifically what that error should be. I could add a test that checks any error is returned in this scenario, but it wouldn't verify this functional change. |
Perhaps we can create a We can expect |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Remaining CI failures don't appear to be related |
|
Ping (libc++ reviewers) |
|
Ping |
…ry (llvm#197104) Consider a directory `parent/child`, where `parent` has read-only permissions. Calling `remove_all` on `parent` is expected to fail with a permission error, however it fails with a different error depending on the platform (such as "error file exists" on AIX). During the recursive removal, `unlinkat` is first called on the child and fails with a permission error. Rather than returning, the function continues and tries `unlinkat` on the parent and fails with a different error depending on the platform. This error is returned instead of the permission error. This PR makes the first encountered error be returned instead. According to the standard [fs.op.remove.all]: > Effects: Recursively deletes the contents of p if it exists, then > deletes file p itself, as if by POSIX remove. The word "then" implies an ordering where deleting the contents of p should be successful before deleting file p itself. Fixes llvm#197100.
fs.rmSync() is implemented with std::filesystem::remove_all(). When a read-only directory has a child, libc++ before LLVM 23 drops the child's EACCES and reports the ENOTEMPTY it then gets for the parent instead. The test pinned that behavior to macOS, but it depends on the C++ standard library rather than on the OS: a Linux build against libc++ fails the test when run as non-root, and macOS will start reporting EACCES once it ships the fixed libc++. Accept either code for that case on all POSIX platforms. Refs: llvm/llvm-project#197104 Refs: #57103 Signed-off-by: Shelley Vohr <[email protected]> PR-URL: #66065 Reviewed-By: Xuguang Mei <[email protected]>
fs.rmSync() is implemented with std::filesystem::remove_all(). When a read-only directory has a child, libc++ before LLVM 23 drops the child's EACCES and reports the ENOTEMPTY it then gets for the parent instead. The test pinned that behavior to macOS, but it depends on the C++ standard library rather than on the OS: a Linux build against libc++ fails the test when run as non-root, and macOS will start reporting EACCES once it ships the fixed libc++. Accept either code for that case on all POSIX platforms. Refs: llvm/llvm-project#197104 Refs: #57103 Signed-off-by: Shelley Vohr <[email protected]> PR-URL: #66065 Reviewed-By: Xuguang Mei <[email protected]>
fs.rmSync() is implemented with std::filesystem::remove_all(). When a read-only directory has a child, libc++ before LLVM 23 drops the child's EACCES and reports the ENOTEMPTY it then gets for the parent instead. The test pinned that behavior to macOS, but it depends on the C++ standard library rather than on the OS: a Linux build against libc++ fails the test when run as non-root, and macOS will start reporting EACCES once it ships the fixed libc++. Accept either code for that case on all POSIX platforms. Refs: llvm/llvm-project#197104 Refs: #57103 Signed-off-by: Shelley Vohr <[email protected]> PR-URL: #66065 Reviewed-By: Xuguang Mei <[email protected]>
fs.rmSync() is implemented with std::filesystem::remove_all(). When a read-only directory has a child, libc++ before LLVM 23 drops the child's EACCES and reports the ENOTEMPTY it then gets for the parent instead. The test pinned that behavior to macOS, but it depends on the C++ standard library rather than on the OS: a Linux build against libc++ fails the test when run as non-root, and macOS will start reporting EACCES once it ships the fixed libc++. Accept either code for that case on all POSIX platforms. Refs: llvm/llvm-project#197104 Refs: #57103 Signed-off-by: Shelley Vohr <[email protected]> PR-URL: #66065 Reviewed-By: Xuguang Mei <[email protected]>
fs.rmSync() is implemented with std::filesystem::remove_all(). When a read-only directory has a child, libc++ before LLVM 23 drops the child's EACCES and reports the ENOTEMPTY it then gets for the parent instead. The test pinned that behavior to macOS, but it depends on the C++ standard library rather than on the OS: a Linux build against libc++ fails the test when run as non-root, and macOS will start reporting EACCES once it ships the fixed libc++. Accept either code for that case on all POSIX platforms. Refs: llvm/llvm-project#197104 Refs: #57103 Signed-off-by: Shelley Vohr <[email protected]> PR-URL: #66065 Reviewed-By: Xuguang Mei <[email protected]>
Consider a directory
parent/child, whereparenthas read-only permissions. Callingremove_allonparentis expected to fail with a permission error, however it fails with a different error depending on the platform (such as "error file exists" on AIX).During the recursive removal,
unlinkatis first called on the child and fails with a permission error. Rather than returning, the function continues and triesunlinkaton the parent and fails with a different error depending on the platform. This error is returned instead of the permission error. This PR makes the first encountered error be returned instead.According to the standard [fs.op.remove.all]:
The word "then" implies an ordering where deleting the contents of p should be successful before deleting file p itself.
Fixes #197100.