Skip to content

Improve ArchGetFileName for windows to return full path #3361

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.

Sign up for GitHub

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

Merged
pixar-oss merged 3 commits into from
Dec 12, 2024
Merged

Improve ArchGetFileName for windows to return full path #3361

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b41ea1
Improve ArchGetFileName to return full path for Windows platform
roggiezhang-nv Nov 20, 2024
b2baa52
Handle long paths that are over MAX_PATH on Windows
roggiezhang-nv Dec 4, 2024
19201b2
Use std::filesystem::canonical to normalize path
roggiezhang-nv Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions pxr/base/arch/fileSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,24 +532,34 @@ ArchGetFileName(FILE *file)
}
return result;
#elif defined (ARCH_OS_WINDOWS)
static constexpr DWORD bufSize =
sizeof(FILE_NAME_INFO) + sizeof(WCHAR) * 4096;
HANDLE hfile = _FileToWinHANDLE(file);
auto fileNameInfo = reinterpret_cast<PFILE_NAME_INFO>(malloc(bufSize));
string result;
if (GetFileInformationByHandleEx(
hfile, FileNameInfo, static_cast<void *>(fileNameInfo), bufSize)) {
WCHAR filePath[MAX_PATH];
Copy link
Contributor

@anwang2009 anwang2009 Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs aren't super clear to me, but I think using MAX_PATH for the buffer size seems like it could be insufficient for Windows configurations where long paths are enabled.

GetFilePathNameByHandleW is supposed to return the required buffer size in the case where the filename would overflow the given buffer – it seems like we should be handling that somehow.

from @sunyab

Copy link
Contributor Author

@roggiezhang-nv roggiezhang-nv Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved.

HANDLE hfile = _FileToWinHANDLE(file);
if (GetFinalPathNameByHandleW(hfile, filePath, MAX_PATH, VOLUME_NAME_DOS)) {
size_t outSize = WideCharToMultiByte(
CP_UTF8, 0, fileNameInfo->FileName,
fileNameInfo->FileNameLength/sizeof(WCHAR),
CP_UTF8, 0, filePath,
wcslen(filePath),
Copy link
Contributor

@anwang2009 anwang2009 Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid calling wcslen by using the return value of GetFilePathNameByHandleW?

from @sunyab

Copy link
Contributor Author

@roggiezhang-nv roggiezhang-nv Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

NULL, 0, NULL, NULL);
result.resize(outSize);
WideCharToMultiByte(
CP_UTF8, 0, fileNameInfo->FileName,
fileNameInfo->FileNameLength/sizeof(WCHAR),
CP_UTF8, 0, filePath,
-1,
&result.front(), outSize, NULL, NULL);

if (result.length() > 4)
{
// It needs to strip the path prefix as
// the path returned is DOS device path, and the
// syntax is one of:
// \\.\C:\Test\Foo.txt
// \\?\C:\Test\Foo.txt
Copy link
Contributor

@anwang2009 anwang2009 Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment might be easier to understand as:

// Strip prefix from paths returned by GetFinalPathNameByHandleW,
// which is one of:
//   \\.\C:\Test\Foo.txt
//   \\?\C:\Test\Foo.txt

from @sunyab

Copy link
Contributor Author

@roggiezhang-nv roggiezhang-nv Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

if (result.compare(0, 4, "\\\\?\\") == 0 ||
result.compare(0, 4, "\\\\.\\") == 0)
{
result.erase(0, 4);
}
Copy link
Contributor

@anwang2009 anwang2009 Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While doing some research, I saw that network paths on Windows will look like "\?\UNC...". I think we've run into issues with network file paths in the past, so we need to figure out whether we need to deal with this specially.

from @sunyab

Copy link
Contributor Author

@roggiezhang-nv roggiezhang-nv Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I treated it specially without stripping the prefix if it's UNC path.

}
}
free(fileNameInfo);
return result;
#else
#error Unknown system architecture
Expand Down
7 changes: 7 additions & 0 deletions pxr/base/arch/testenv/testFileSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <cstdio>
#include <cstdlib>
#include <cstring>
#include <filesystem>

PXR_NAMESPACE_USING_DIRECTIVE

Expand Down Expand Up @@ -92,6 +93,12 @@ int main()
fclose(firstFile);
ARCH_AXIOM(ArchGetFileLength(firstName.c_str()) == strlen(testContent));

// Open a file, check that the file path from FILE* handle is matched.
ARCH_AXIOM((firstFile = ArchOpenFile(firstName.c_str(), "rb")) != NULL);
std::string filePath = ArchGetFileName(firstFile);
ARCH_AXIOM(std::filesystem::equivalent(filePath, firstName));
fclose(firstFile);

// Map the file and assert the bytes are what we expect they are.
ARCH_AXIOM((firstFile = ArchOpenFile(firstName.c_str(), "rb")) != NULL);
ArchConstFileMapping cfm = ArchMapFileReadOnly(firstFile);
Expand Down