[libc++] Make std::filesystem::canonical throw when given empty path #77223

Yuki-cpp · 2024-01-07T06:33:07Z

As noted in the issue, std::filesystem::canonical returns the current path when given "" while an error is expected. Further investigations shown that the issue lies within std::filesystem::absolute which is then re-used by canonical.
To fix that, this PR adds a check for empty paths in std::filesystem::absolute and raise an error accordingly. This in turn fixes std::filesystem::canonical

Since std::filesystem::weakly_canonical and uses std::filesystem::canonical and should not raise an error when given an empty path, it was updated to return an empty path instead of the results of canonical.

Finally tests for std::filesystem::weakly_canonical and std::filesystem::proximate were updated as they were built on the faulty behavior.

Adds a check for empty paths in std::filesystem::canonical and raise an error accordingly. Since std::filesystem::weakly_canonical uses this function and should not raise an error when given an empty path, it was updated to return an empty path instead of the results of canonical.

github-actions · 2024-01-07T06:33:26Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-01-07T06:33:52Z

@llvm/pr-subscribers-libcxx

Author: Leo (Yuki-cpp)

Changes

Fixes #77170

As noted in the issue, std::filesystem::canonical returns the current path when given "" while an error is expected.
To fix that, this PR adds a check for empty paths in std::filesystem::canonical and raise an error accordingly. Since std::filesystem::weakly_canonical uses this function and should not raise an error when given an empty path, it was updated to return an empty path instead of the results of canonical.

Note: Running clang-format on the canonical.pass.cpp file produced formatting different than the rest of the file. As such, to conform with the Coding Standard I haven't formatted that file automatically but rather kept the style of the file intact. Let me know if I missed something there and I'll gladly fix it.

Note 2: Might as well mention now that I do not have commit access

Full diff: https://github.com/llvm/llvm-project/pull/77223.diff

2 Files Affected:

(modified) libcxx/src/filesystem/operations.cpp (+4-2)
(modified) libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.canonical/canonical.pass.cpp (+26)

diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index 6bee340e0d15c8..8f5b70a071373d 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -82,6 +82,8 @@ path __canonical(path const& orig_p, error_code* ec) {
   path cwd;
   ErrorHandler<path> err("canonical", ec, &orig_p, &cwd);
 
+  if (orig_p.empty())
+    return err.report(capture_errno());
   path p = __do_absolute(orig_p, &cwd, ec);
 #if (defined(_POSIX_VERSION) && _POSIX_VERSION >= 200112) || defined(_LIBCPP_WIN32API)
   std::unique_ptr<path::value_type, decltype(&::free)> hold(detail::realpath(p.c_str(), nullptr), &::free);
@@ -912,7 +914,7 @@ path __weakly_canonical(const path& p, error_code* ec) {
   ErrorHandler<path> err("weakly_canonical", ec, &p);
 
   if (p.empty())
-    return __canonical("", ec);
+    return __current_path(ec);
 
   path result;
   path tmp;
@@ -935,7 +937,7 @@ path __weakly_canonical(const path& p, error_code* ec) {
     --PP;
   }
   if (PP.State == PathParser::PS_BeforeBegin)
-    result = __canonical("", ec);
+    result = __current_path(ec);
   if (ec)
     ec->clear();
   if (DNEParts.empty())
diff --git a/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.canonical/canonical.pass.cpp b/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.canonical/canonical.pass.cpp
index 0098fe8ee698ef..000fb47096ba19 100644
--- a/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.canonical/canonical.pass.cpp
+++ b/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.canonical/canonical.pass.cpp
@@ -92,6 +92,31 @@ static void test_dne_path()
     }
 }
 
+static void test_empty_path()
+{
+    std::error_code ec = GetTestEC();
+    {
+        const path ret = canonical(path{}, ec);
+        assert(ec != GetTestEC());
+        assert(ec);
+        assert(ret == path{});
+    }
+    {
+        TEST_THROWS_TYPE(filesystem_error, canonical(path{}));
+    }
+
+    ec = GetTestEC();
+    {
+        const path ret = canonical("", ec);
+        assert(ec != GetTestEC());
+        assert(ec);
+        assert(ret == path{});
+    }
+    {
+        TEST_THROWS_TYPE(filesystem_error, canonical(""));
+    }
+}
+
 static void test_exception_contains_paths()
 {
 #ifndef TEST_HAS_NO_EXCEPTIONS
@@ -121,6 +146,7 @@ int main(int, char**) {
     signature_test();
     test_canonical();
     test_dne_path();
+    test_empty_path();
     test_exception_contains_paths();
 
     return 0;

github-actions · 2024-01-07T06:35:57Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Zingam · 2024-01-07T07:55:08Z

You should format the code snippet you added. Please check the code formatter checker. It is normal that old code is using old formatting and the new one the new style in the same file.

…y path

Yuki-cpp · 2024-01-07T08:32:11Z

You should format the code snippet you added. Please check the code formatter checker. It is normal that old code is using old formatting and the new one the new style in the same file.

Will remember that for next time. Thanks

mordante · 2024-01-07T12:35:08Z

Thanks for your contribution!

Fixes #77170

As noted in the issue, std::filesystem::canonical returns the current path when given "" while an error is expected. To fix that, this PR adds a check for empty paths in std::filesystem::canonical and raise an error accordingly. Since std::filesystem::weakly_canonical uses this function and should not raise an error when given an empty path, it was updated to return an empty path instead of the results of canonical.

Can you remove the notes below from the commit message, to avoid accidentally merging them in the commit?

Note: Running clang-format on the canonical.pass.cpp file produced formatting different than the rest of the file. As such, to conform with the Coding Standard I haven't formatted that file automatically but rather kept the style of the file intact. Let me know if I missed something there and I'll gladly fix it.

we recently have formatted our code except for the tests. That will done in the future. For now keeping the existing code is fine.

Note 2: Might as well mention now that I do not have commit access

No problem. Can you make sure your e-mail address is not hidden.

mordante · 2024-01-07T12:23:43Z

libcxx/src/filesystem/operations.cpp

+  if (orig_p.empty())
+    return err.report(capture_errno());


I dislike this solution, does not fix the real issue, this part should be fixed in __do_absolute. See https://godbolt.org/z/GrvW1KonT

Can you fix absolute and weakly_canonical too?

Just a heads up.

By moving that test (a slightly modified version) to __do_absolute as you suggest, I can get absolute, canonical and weakly_canonical to work as expected. However it appears that both proximate and relative are dependent on weakly_canonical and are both in their tests depending on the "" becomes current_dir behavior.

Modifying your example, I can highlight some more discrepancies : https://godbolt.org/z/14h8Pv7vc

I believe I can iron those out, but it will take me a bit more time and extend the scope of this PR. Is that acceptable?

Since they are all related to the same root cause I don't mind to have these changes in one PR.

Take the time you need.

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.canonical/canonical.pass.cpp

Yuki-cpp · 2024-01-07T12:47:44Z

Thank you for the review. I'll get to it.

One question though:

Can you remove the notes below from the commit message, to avoid accidentally merging them in the commit?

Are you referring to this part?

Since std::filesystem::weakly_canonical uses this function and should not raise an error when given an empty path, it was updated to return an empty path instead of the results of canonical.

Or to are referring to the note I added to the PR description? (Those 2 shouldn't be in the commit message, but I can also remove them from the PR if you think it's best)

mordante · 2024-01-07T13:35:08Z

Thank you for the review. I'll get to it.

One question though:

Can you remove the notes below from the commit message, to avoid accidentally merging them in the commit?

Are you referring to this part?

I'm referring to the two notes below this comment. The one for weakly_canonical is a good note and would be good to have in the commit message.

…en empty path

…hen given empty path

mordante · 2024-01-10T18:22:06Z

For the buildkite CI issues you can ignore Apple for now, but please have a look at Windows.

Yuki-cpp · 2024-01-11T04:21:08Z

For the buildkite CI issues you can ignore Apple for now, but please have a look at Windows.

Yeah. I am on it.
Ont thing I would like to note though:
When "fixing" the test for proximate, a lot of them are depending on a filesystem tree that doesn't match anything I have found here so far (I am talking about the a and a/b folders, while other tests rely on a static_env)

In the current implementation I have simply "fixed" the test (I'll handle the windows one ASAP) but the constant reliance on those folders make me feel like I missed something somewhere. Is there actually something somewhere that I missed or was previously forgotten that defines those files and folders structure?

…throw when given empty path

Yuki-cpp · 2024-01-14T14:47:56Z

@mordante Windows build should be good now.
Let me know if you think anything need further change.

mordante · 2024-01-14T18:55:44Z

@mordante Windows build should be good now. Let me know if you think anything need further change.

Thanks! I'll try to have a look tomorrow.

mordante · 2024-01-15T16:23:50Z

Thanks I think this looks good, I want to test the patch locally to compare a bit more with GCC. I need to do some unrelated changes so we can properly mark the status for the Apple builds.

Yuki-cpp · 2024-01-18T08:11:25Z

Thanks. I'll be on stand-by if you require any changes here then.

mordante · 2024-01-21T13:21:23Z

I'm quite busy and this patch depends on a work-in-progess patch to fix the apple-build. So it will take a bit longer to get to this patch.

mordante · 2024-02-11T15:26:58Z

I finally landed the patch that makes it easier to disable the Apple tests.

Can you rebase this patch and add the following to the patches that fail on the Apple CI?

// add a description why the test is disabled.
// UNSUPPORTED: using-built-library-before-llvm-19

Yuki-cpp · 2024-02-12T07:04:02Z

Sure.
Might take me a couple of days. Will let you know when I'm done.

mordante · 2024-02-12T18:45:00Z

Sure. Might take me a couple of days. Will let you know when I'm done.

Great. No hurry. I just wanted to inform you I landed the patch that unblocks this patch. (That took longer than I expected.)

Yuki-cpp · 2024-02-27T04:35:22Z

@mordante Just a bit of an update.
I got pulled off on personal matters and won't likely have time to deal with this issue for a while (count around month I think?).
I'll get back to it when I get the time but didn't want to let you hang waiting for it. If you believe that it shouldn't wait, I understand and in that case feel free to assign the issue to somebody else.

Sorry for the annoyance.

mordante · 2024-03-03T17:12:30Z

@mordante Just a bit of an update. I got pulled off on personal matters and won't likely have time to deal with this issue for a while (count around month I think?). I'll get back to it when I get the time but didn't want to let you hang waiting for it. If you believe that it shouldn't wait, I understand and in that case feel free to assign the issue to somebody else.

Sorry for the annoyance.

Thanks for the update! Not problem at all.

philnik777 · 2025-05-31T07:02:30Z

@Yuki-cpp Do you plan to continue working on this?

Yuki-cpp requested a review from a team as a code owner January 7, 2024 06:33

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 7, 2024

fixup! [libc++] Make std::filesystem::canonical throw when given empt…

614ec4b

…y path

mordante self-assigned this Jan 7, 2024

mordante requested changes Jan 7, 2024

View reviewed changes

Yuki-cpp added 2 commits January 10, 2024 21:19

fixup! fixup! [libc++] Make std::filesystem::canonical throw when giv…

fbc6696

…en empty path

fixup! fixup! fixup! [libc++] Make std::filesystem::canonical throw w…

6edd466

…hen given empty path

fixup! fixup! fixup! fixup! [libc++] Make std::filesystem::canonical …

7ac0f47

…throw when given empty path

[libc++] Make std::filesystem::canonical throw when given empty path #77223

Are you sure you want to change the base?

[libc++] Make std::filesystem::canonical throw when given empty path #77223

Uh oh!

Conversation

Yuki-cpp commented Jan 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 7, 2024

Uh oh!

llvmbot commented Jan 7, 2024

Uh oh!

github-actions bot commented Jan 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zingam commented Jan 7, 2024

Uh oh!

Yuki-cpp commented Jan 7, 2024

Uh oh!

mordante commented Jan 7, 2024

Uh oh!

mordante Jan 7, 2024

Choose a reason for hiding this comment

Uh oh!

Yuki-cpp Jan 7, 2024

Choose a reason for hiding this comment

Uh oh!

mordante Jan 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Yuki-cpp commented Jan 7, 2024

Uh oh!

mordante commented Jan 7, 2024

Uh oh!

mordante commented Jan 10, 2024

Uh oh!

Yuki-cpp commented Jan 11, 2024

Uh oh!

Yuki-cpp commented Jan 14, 2024

Uh oh!

mordante commented Jan 14, 2024

Uh oh!

mordante commented Jan 15, 2024

Uh oh!

Yuki-cpp commented Jan 18, 2024

Uh oh!

mordante commented Jan 21, 2024

Uh oh!

mordante commented Feb 11, 2024

Uh oh!

Yuki-cpp commented Feb 12, 2024

Uh oh!

mordante commented Feb 12, 2024

Uh oh!

Yuki-cpp commented Feb 27, 2024

Uh oh!

mordante commented Mar 3, 2024

Uh oh!

philnik777 commented May 31, 2025

Uh oh!

Uh oh!

Yuki-cpp commented Jan 7, 2024 •

edited

Loading

github-actions bot commented Jan 7, 2024 •

edited

Loading