Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow weakly_canonical to be used with Windows long paths #247

Closed
emmenlau opened this issue Aug 9, 2022 · 15 comments
Closed

Allow weakly_canonical to be used with Windows long paths #247

emmenlau opened this issue Aug 9, 2022 · 15 comments

Comments

@emmenlau
Copy link

emmenlau commented Aug 9, 2022

Currently I'm unable to call weakly_canonical() with a long path on Windows. Long path in this context means the Microsoft Windows extension described at https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation. A path can be prefixed by \\?\ and then can have more than 260 characters (with a number of restrictions).

I could trace the problem of using long paths in weakly_canonical() to the method create_file_handle() in

return ::CreateFileW(p.c_str(), dwDesiredAccess, dwShareMode, lpSecurityAttributes, dwCreationDisposition, dwFlagsAndAttributes, hTemplateFile);

This is called via:

file_status head_status = detail::status_impl(head, &local_ec);

But the call fails for long paths. Since in weakly_canonical(), the handle is only needed for the root item, I could work around this by adding some extra handling to create_file_handle(). My added logic is:

  • If the given path has a long-path prefix but is less than 260 characters long, remove the long path prefix and rely on Windows to do the right thing.

Is that reasonable, and could something like this be added to the library?

@Lastique
Copy link
Member

Lastique commented Aug 9, 2022

If you need to operate on long paths on Windows, it is up to the application to prepend the path with \\?\ prefix before passing to Boost.Filesystem. Boost.Filesystem does not do that for you, and I don't think I want to make it do so. Mostly because that would introduce unnecessary inefficiency and complicate implementation quite substantially.

If you're on a recent enough Windows, I would also recommend to enable long paths by default system-wide.

@emmenlau
Copy link
Author

emmenlau commented Aug 9, 2022

Dear @Lastique , thanks for the quick reply!

My problem is actually the opposite. I do have prepend the path with \\?\ prefix before passing to Boost.Filesystem. This is causing the problem in

return ::CreateFileW(p.c_str(), dwDesiredAccess, dwShareMode, lpSecurityAttributes, dwCreationDisposition, dwFlagsAndAttributes, hTemplateFile);

@Lastique
Copy link
Member

Lastique commented Aug 9, 2022

I don't understand, what problem? Does the call fail with the \\?\ prefix? With what error code? Can you prepare a small reproducer code?

Note that the prefix is only valid for absolute paths, so make sure you're not adding it if the path is relative.

@emmenlau
Copy link
Author

emmenlau commented Aug 9, 2022

I don't understand, what problem? Does the call fail with the \\?\ prefix? With what error code? Can you prepare a small reproducer code?

I assumed that my description was more clear, sorry if it was not!

Here are more details: The call of ::CreateFileW() fails, if the path has a prefix \\?\. If the path has the prefix, then ::CreateFileW() does return a handle, but the handle is all zeros. I'm not knowledgeable about Windows methods, so I do not understand if a handle of zero is correct. I just assume it is wrong. I assume this because subsequently file_status head_status = detail::status_impl(head, &local_ec); showed an error status.

As a test, I added code above ::CreateFileW() that removes the prefix \\?\, and the error was gone. Therefore I conclude that ::CreateFileW() does not support \\?\.

@emmenlau
Copy link
Author

emmenlau commented Aug 9, 2022

So here is my workaround that fixes the problem for me:

diff --git a/libs/filesystem/src/windows_tools.hpp b/libs/filesystem/src/windows_tools.hpp
index 469be041e..e49ecd6a4 100644
--- a/libs/filesystem/src/windows_tools.hpp
+++ b/libs/filesystem/src/windows_tools.hpp
@@ -179,7 +179,11 @@ struct handle_wrapper
 //! Creates a file handle
 inline HANDLE create_file_handle(boost::filesystem::path const& p, DWORD dwDesiredAccess, DWORD dwShareMode, LPSECURITY_ATTRIBUTES lpSecurityAttributes, DWORD dwCreationDisposition, DWORD dwFlagsAndAttributes, HANDLE hTemplateFile = NULL)
 {
-    return ::CreateFileW(p.c_str(), dwDesiredAccess, dwShareMode, lpSecurityAttributes, dwCreationDisposition, dwFlagsAndAttributes, hTemplateFile);
+    std::wstring p_str = p.wstring();
+    if ((p_str.size() < 260) && (p_str.size() > 4) && (p_str[0] == L'\\') && (p_str[1] == L'\\') && (p_str[2] == L'?') && (p_str[3] == L'\\')) {
+        p_str = p_str.substr(4);
+    }
+    return ::CreateFileW(p_str.c_str(), dwDesiredAccess, dwShareMode, lpSecurityAttributes, dwCreationDisposition, dwFlagsAndAttributes, hTemplateFile);
 }
 
 } // namespace detail

I'm not saying that this is beautiful. But it just shows that there would be a way to avoid the problem.

@Lastique
Copy link
Member

Lastique commented Aug 9, 2022

CreateFileW returns INVALID_HANDLE_VALUE on error, which is (HANDLE)-1 (i.e. not zero). A zero returned handle is not considered an error.

Also, I asked about the error code and reproducer because this could give a clue as to what the problem is.

@emmenlau
Copy link
Author

emmenlau commented Aug 9, 2022

Also, I asked about the error code and reproducer because this could give a clue as to what the problem is.

Yes, I understand that. I do not currently have access to a Windows machine, but I'll try to send more information when I have.

@Lastique
Copy link
Member

Lastique commented Aug 9, 2022

CreateFileW is listed among the APIs that do support long paths. I have added a test to verify this.

I suspect the problem is with the path you're using. The \\?\ prefix is not simply allowing for longer paths, it actually disables a great deal of path processing in Windows API. This means that the path must be absolute, must use backslashes as separators, and must not contain dot (".") and dot-dot ("..") elements. If you're using an UNC path then the prefix must be \\?\UNC\ instead of \\. Please verify that the path you're passing to weakly_canonical meets these requirements.

Lastique added a commit that referenced this issue Aug 10, 2022
During its operation, weakly_canonical would call status() on the path
consisting only from the root name of the input path. This would fail
with ERROR_INVALID_FUNCTION if the root name starts with the "\\?\" prefix,
as the root name path is not absolute.

To fix this, we don't check the status of the root name path (which is
not the correct check anyways as it tests the current directory on the
corresponding drive for existence, which is not what we want). Additionally,
avoid calling status() on the paths containing dot and dot-dot elements
during the weakly_canonical execution for the same reason - the "\\?\"
prefix disables most of the path processing in Windows APIs, including
dot and dot-dot elements resolution.

Fixes #247.
@emmenlau
Copy link
Author

Thanks a lot for the follow-up!

What I can say so from my debugger session yesterday is that weakly_canonical was calling CreateFileW with only the head of the path in file_status head_status = detail::status_impl(head, &local_ec);. From what I recall from the debugger, the head of the path was something like \\?\C: and nothing else, maybe with one more backslash.

Please verify that the path you're passing to weakly_canonical meets these requirements.

I have a method that takes care of these requirements before passing the path to weakly_canonical. It's possible that the method is buggy, but it did work for thousands of paths with canonical, and just failed for weakly_canonical so far. But I don't want to guess, so I'll try to reproduce the issue and let you know what I find!

@Lastique
Copy link
Member

Lastique commented Aug 10, 2022

From what I recall from the debugger, the head of the path was something like \\?\C: and nothing else, maybe with one more backslash.

Yes, that was part of how weakly_canonical works, and this turned out to be the problem because the "C:" path is not absolute. I have committed a fix that should resolve this problem.

Thank you for reporting this.

@emmenlau
Copy link
Author

Ok, I'm sitting at the Windows computer and can reproduce the issue now.

The path I pass to weakly_canonical is \\\\?\\C:\\data\\aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa. The double backslashes in the string are copied from source code, but the actual string should have single backslashes.

With this path, I get exception boost::filesystem::filesystem_error with description boost::filesystem::weakly_canonical: Incorrect function [system:1]: "\\?\C:"

When I execute the test in the debugger, it stops with an exception in operations.cpp line 4076. In my version of boost (1.79.0) that line reads:

BOOST_FILESYSTEM_THROW(filesystem_error("boost::filesystem::weakly_canonical", head, local_ec));

The debugger says about the exception:

Exception thrown at 0x00007FFBC778474C in BaseFileTest.exe: Microsoft C++ exception: boost::filesystem::filesystem_error at memory location 0x0000001AA913EDE0.

@emmenlau
Copy link
Author

But I see its fixed now, thanks!

@greenkalx
Copy link

greenkalx commented Jan 11, 2023

I was expecting this fix in 1_81_0.

I see the changes in libs\filesystem\src\operations.cpp from 1_80 to 1_81.

We have three failing unit tests:

BOOST_CHECK(Path(R"(\\?\UNC\x\y)").IsUnc());
BOOST_CHECK(Path(R"(\\?\UNC\server\sharename\)").IsUnc());
BOOST_CHECK(Path(R"(\\?\UNC\x\y\s\z)").VolumeRoot() == LR"(\\?/UNC\x\y\)");

They started failing after 1_73_0, I think first with 1_80_0.

System is Windows 10 with long paths enabled, compiling with Visual Studio 2022 C++17.

I built the .lib's and .dll's.. checked for stale references..

@Lastique
Copy link
Member

Yes, the fix is in Boost 1.81.0.

I'm not sure what unit tests you're running, they don't look like Boost.Filesystem. If you have a problem with Boost.Filesystem, please report it in a new issue, with a short reproducer.

@greenkalx
Copy link

greenkalx commented Jan 13, 2023

Created #272

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants