From 7f01b898e860dfc328ca733de099c7f9e0e262aa Mon Sep 17 00:00:00 2001 From: Gavin Garzia Date: Fri, 7 Aug 2026 13:42:14 -0700 Subject: [PATCH 1/5] Make paths absolute before weakly_canonical and add regression tests weakly_canonical only returns an absolute path when a leading element of the input exists: it builds its result from the root path plus the longest leading sequence it can canonicalize, so a relative path whose first element is missing is returned unchanged. Callers that pass a not-yet-created path therefore produced a relative result, which the service rejects with "Path is not absolute". This affected `wslc build --iidfile ` and `--secret src=` when the target did not exist, plus the distribution path and virtiofs share path helpers in the service. Wrap each call in std::filesystem::absolute so the result is absolute regardless of whether the path exists. Existing coverage could not catch this: every --iidfile e2e test passed an absolute path, and the relative --secret test used a file that already existed, which makes weakly_canonical succeed on its own. Add two tests that fail without the fix - a parser unit test using a relative src that names a missing file, and an e2e test running `build --iidfile image.id` from the context directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/windows/service/exe/LxssUserSession.cpp | 4 +- src/windows/service/exe/WslCoreVm.cpp | 2 +- src/windows/wslc/arguments/SpecParsing.cpp | 2 +- .../wslc/WSLCCLISecretParserUnitTests.cpp | 37 +++++++++++++-- .../wslc/e2e/WSLCE2EImageBuildTests.cpp | 46 +++++++++++++++++++ 5 files changed, 84 insertions(+), 7 deletions(-) diff --git a/src/windows/service/exe/LxssUserSession.cpp b/src/windows/service/exe/LxssUserSession.cpp index 050f6a945..c06eeaf94 100644 --- a/src/windows/service/exe/LxssUserSession.cpp +++ b/src/windows/service/exe/LxssUserSession.cpp @@ -3847,7 +3847,7 @@ void LxssUserSessionImpl::_ValidateDistributionNameAndPathNotInUse( if (Path != nullptr) { - canonicalPath = std::filesystem::weakly_canonical(Path, error); + canonicalPath = std::filesystem::weakly_canonical(std::filesystem::absolute(Path, error), error); if (error) { LOG_WIN32(error.value()); @@ -3891,7 +3891,7 @@ void LxssUserSessionImpl::_ValidateDistributionNameAndPathNotInUse( if (Path != nullptr) { - auto canonicalDistroPath = std::filesystem::weakly_canonical(configuration.BasePath, error); + auto canonicalDistroPath = std::filesystem::weakly_canonical(std::filesystem::absolute(configuration.BasePath, error), error); if (error) { LOG_WIN32(error.value()); diff --git a/src/windows/service/exe/WslCoreVm.cpp b/src/windows/service/exe/WslCoreVm.cpp index 38ecd47fc..b56bdba94 100644 --- a/src/windows/service/exe/WslCoreVm.cpp +++ b/src/windows/service/exe/WslCoreVm.cpp @@ -2182,7 +2182,7 @@ std::tuple WslCoreVm::AddVirtioFsShare sharePath.push_back(L'\\'); } - sharePath = std::filesystem::weakly_canonical(sharePath).wstring(); + sharePath = std::filesystem::weakly_canonical(std::filesystem::absolute(sharePath)).wstring(); std::wstring effectiveOptions(Options); diff --git a/src/windows/wslc/arguments/SpecParsing.cpp b/src/windows/wslc/arguments/SpecParsing.cpp index b869c3a44..fb3bfb857 100644 --- a/src/windows/wslc/arguments/SpecParsing.cpp +++ b/src/windows/wslc/arguments/SpecParsing.cpp @@ -170,7 +170,7 @@ services::BuildSecret ParseSecretSpec(const std::wstring& spec) // reject an unmountable or unreadable file instead. weakly_canonical resolves a relative path // against the current directory, collapses '..', and resolves symlinks for the portion of the // path that exists; it succeeds for a missing file but still reports genuine errors. - auto absPath = std::filesystem::weakly_canonical(srcPath, ec); + auto absPath = std::filesystem::weakly_canonical(std::filesystem::absolute(srcPath), ec); if (ec.value() != 0) { throw ArgumentException( diff --git a/test/windows/wslc/WSLCCLISecretParserUnitTests.cpp b/test/windows/wslc/WSLCCLISecretParserUnitTests.cpp index 88a8b1950..fc533c2e1 100644 --- a/test/windows/wslc/WSLCCLISecretParserUnitTests.cpp +++ b/test/windows/wslc/WSLCCLISecretParserUnitTests.cpp @@ -107,7 +107,7 @@ class WSLCCLISecretParserUnitTests VERIFY_ARE_EQUAL(expectedId, secret.Id); VERIFY_IS_TRUE(secret.Value.empty()); std::error_code ec; - const auto expectedCanonical = std::filesystem::weakly_canonical(std::filesystem::absolute(expectedPath), ec); + const auto expectedCanonical = std::filesystem::weakly_canonical(std::filesystem::absolute(expectedPath, ec), ec); VERIFY_ARE_EQUAL(expectedCanonical.wstring(), secret.SourcePath); } @@ -209,7 +209,7 @@ class WSLCCLISecretParserUnitTests VERIFY_IS_TRUE(secret.Value.empty()); std::error_code ec; - const auto expectedCanonical = std::filesystem::weakly_canonical(path, ec); + const auto expectedCanonical = std::filesystem::weakly_canonical(std::filesystem::absolute(path, ec), ec); VERIFY_ARE_EQUAL(expectedCanonical.wstring(), secret.SourcePath); } @@ -261,10 +261,41 @@ class WSLCCLISecretParserUnitTests VERIFY_IS_TRUE(std::filesystem::path(secret.SourcePath).is_absolute()); std::error_code ec; - const auto expectedCanonical = std::filesystem::weakly_canonical(absPath, ec); + const auto expectedCanonical = std::filesystem::weakly_canonical(std::filesystem::absolute(absPath, ec), ec); VERIFY_ARE_EQUAL(expectedCanonical.wstring(), secret.SourcePath); } + // A relative src= naming a file that does not exist must still resolve to an absolute SourcePath. + // Parsing deliberately does not require the file to exist, so this case is reachable and the server + // still rejects a non-absolute path. std::filesystem::weakly_canonical cannot handle it on its own: + // it only produces an absolute path by canonicalizing the longest leading sequence of elements that + // exist, so a bare missing filename has nothing to canonicalize and is returned unchanged. The + // relative-src test above cannot catch this because its file exists. + TEST_METHOD(Secret_File_RelativeSrcMissingFileResolvedToAbsolutePath) + { + const auto directory = std::filesystem::temp_directory_path(); + const auto relativeSrc = L"wslc_ut_secret_missing_" + std::to_wstring(GetCurrentProcessId()) + L"_" + + std::to_wstring(GetTickCount64()) + L".bin"; + VERIFY_IS_FALSE(std::filesystem::exists(directory / relativeSrc)); + + auto originalDir = std::filesystem::current_path(); + auto restoreDir = wil::scope_exit([&]() { + std::error_code ec; + std::filesystem::current_path(originalDir, ec); + }); + std::filesystem::current_path(directory); + + VERIFY_IS_FALSE(std::filesystem::path(relativeSrc).is_absolute()); + + auto secret = validation::ParseSecretSpec(L"id=s,src=" + relativeSrc); + VERIFY_ARE_EQUAL(std::wstring(L"s"), secret.Id); + VERIFY_IS_TRUE(std::filesystem::path(secret.SourcePath).is_absolute()); + + // The leading directory exists, so it canonicalizes; only the missing filename is appended. + const auto expected = std::filesystem::canonical(directory) / relativeSrc; + VERIFY_ARE_EQUAL(expected.wstring(), secret.SourcePath); + } + // --- Invalid: spec structure --- TEST_METHOD(Secret_Invalid_EmptyId) diff --git a/test/windows/wslc/e2e/WSLCE2EImageBuildTests.cpp b/test/windows/wslc/e2e/WSLCE2EImageBuildTests.cpp index 8ece83bb5..f1d52c207 100644 --- a/test/windows/wslc/e2e/WSLCE2EImageBuildTests.cpp +++ b/test/windows/wslc/e2e/WSLCE2EImageBuildTests.cpp @@ -1301,6 +1301,51 @@ class WSLCE2EImageBuildTests VERIFY_ARE_EQUAL(inspectedId, wsl::windows::common::string::WideToMultiByte(iid)); } + // --iidfile must accept a path relative to the caller's current directory. The client is responsible + // for making the path absolute before it reaches the service, which rejects non-absolute paths. + // std::filesystem::weakly_canonical alone is not sufficient here: --iidfile names a file that does + // not exist yet, so there is no leading element to canonicalize and the path is returned unchanged. + WSLC_TEST_METHOD(WSLCE2E_Image_Build_IidFile_RelativePath) + { + auto imageCleanup = DeleteImageOnExit(BuiltImageIidFileRelative); + auto testRoot = std::filesystem::current_path() / L"wslc-e2e-build-iidfile-relative"; + auto cleanup = SetupTestDirectory(testRoot); + + auto contextDir = SharedOutputBuildContext(); + + auto dockerfilePath = testRoot / L"Dockerfile"; + WriteTestFileContent(dockerfilePath, "FROM debian:latest\nRUN echo wslc-iidfile-relative-marker > /marker.txt\n"); + + // Run wslc from testRoot so the --iidfile argument below resolves against it. Declared after + // the directory cleanup so the working directory is restored before the directory is removed. + auto originalDirectory = std::filesystem::current_path(); + auto restoreDirectory = wil::scope_exit([&]() { + std::error_code ec; + std::filesystem::current_path(originalDirectory, ec); + }); + std::filesystem::current_path(testRoot); + + const std::wstring relativeIidFile = L"image.id"; + VERIFY_IS_FALSE(std::filesystem::path(relativeIidFile).is_absolute()); + VERIFY_IS_FALSE(std::filesystem::exists(testRoot / relativeIidFile)); + + auto buildResult = RunWslc(std::format( + L"build \"{}\" -f \"{}\" -t {} --iidfile \"{}\"", + contextDir.wstring(), + dockerfilePath.wstring(), + BuiltImageIidFileRelative.NameAndTag(), + relativeIidFile)); + buildResult.Verify({.ExitCode = 0}); + + VERIFY_IS_TRUE(std::filesystem::exists(testRoot / relativeIidFile), L"--iidfile must accept a relative path"); + const auto iid = ReadFileContent((testRoot / relativeIidFile).wstring()); + VERIFY_IS_TRUE(iid.starts_with(L"sha256:"), L"iidfile must contain a sha256 digest"); + + // The digest written to the iidfile must match the ID the image is stored under. + const auto inspectedId = InspectImage(BuiltImageIidFileRelative.NameAndTag()).Id; + VERIFY_ARE_EQUAL(inspectedId, wsl::windows::common::string::WideToMultiByte(iid)); + } + // A failing build must not write the iidfile (matching docker: the file only appears on success). WSLC_TEST_METHOD(WSLCE2E_Image_Build_IidFile_BuildFailure_NoFileWritten) { @@ -1421,6 +1466,7 @@ class WSLCE2EImageBuildTests const TestImage BuiltImageOutputCacheOnly{L"wslc-e2e-build-output-cacheonly", L"latest", L""}; const TestImage BuiltImageIidFile{L"wslc-e2e-build-iidfile", L"latest", L""}; const TestImage BuiltImageIidFileNotWritable{L"wslc-e2e-build-iidfile-readonly", L"latest", L""}; + const TestImage BuiltImageIidFileRelative{L"wslc-e2e-build-iidfile-relative", L"latest", L""}; // Runs `tar.exe -tf ` and returns the member listing so tests can assert an exporter produced a // valid, non-empty archive that contains an expected entry. From 5c965aa86cf9bb183c5bbbfef32e5145fa173141 Mon Sep 17 00:00:00 2001 From: Gavin Garzia Date: Mon, 10 Aug 2026 14:15:33 -0700 Subject: [PATCH 2/5] Add shared GetCanonicalPath helpers and use them for path canonicalization Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/windows/common/filesystem.cpp | 29 +++ src/windows/common/filesystem.hpp | 15 ++ src/windows/service/exe/LxssUserSession.cpp | 4 +- src/windows/service/exe/WslCoreVm.cpp | 2 +- src/windows/wslc/arguments/SpecParsing.cpp | 4 +- src/windows/wslc/services/ImageService.cpp | 2 +- src/windows/wslc/tasks/ContainerTasks.cpp | 2 +- src/windows/wslinstaller/exe/WslInstaller.cpp | 4 +- test/windows/CMakeLists.txt | 1 + test/windows/FilesystemUnitTests.cpp | 193 ++++++++++++++++++ test/windows/InstallerTests.cpp | 8 +- test/windows/UnitTests.cpp | 4 +- test/windows/WSLCTests.cpp | 5 +- 13 files changed, 256 insertions(+), 17 deletions(-) create mode 100644 test/windows/FilesystemUnitTests.cpp diff --git a/src/windows/common/filesystem.cpp b/src/windows/common/filesystem.cpp index 0441fb2c2..88011ae70 100644 --- a/src/windows/common/filesystem.cpp +++ b/src/windows/common/filesystem.cpp @@ -774,6 +774,35 @@ bool wsl::windows::common::filesystem::FileExists(_In_ LPCWSTR Path) return (Attributes != INVALID_FILE_ATTRIBUTES); } +std::filesystem::path wsl::windows::common::filesystem::GetCanonicalPath(const std::filesystem::path& Path) +{ + std::error_code error; + auto canonicalPath = GetCanonicalPath(Path, error); + THROW_HR_IF_MSG(HRESULT_FROM_WIN32(error.value()), !!error, "GetCanonicalPath(%ls)", Path.c_str()); + + return canonicalPath; +} + +std::filesystem::path wsl::windows::common::filesystem::GetCanonicalPath(const std::filesystem::path& Path, std::error_code& Error) +{ + // N.B. absolute() is applied first because weakly_canonical() does not resolve a relative path + // against the current directory on its own. Its result is checked before canonicalizing because + // weakly_canonical() clears Error on success, which would otherwise mask an absolute() failure. + const auto absolutePath = std::filesystem::absolute(Path, Error); + if (Error) + { + return {}; + } + + auto canonicalPath = std::filesystem::weakly_canonical(absolutePath, Error); + if (Error) + { + return {}; + } + + return canonicalPath; +} + std::filesystem::path wsl::windows::common::filesystem::GetFullPath(_In_ LPCWSTR Path) { DWORD Attributes = GetFileAttributesW(Path); diff --git a/src/windows/common/filesystem.hpp b/src/windows/common/filesystem.hpp index 1a89f8b8f..d587b1a7a 100644 --- a/src/windows/common/filesystem.hpp +++ b/src/windows/common/filesystem.hpp @@ -137,6 +137,21 @@ void EnsureDirectoryWithAttributes(_In_ PCWSTR Path, _In_ ULONG Mode, _In_ ULONG bool FileExists(_In_ LPCWSTR Path); +/// +/// Resolves Path to an absolute, canonical form. The path is made absolute against the current +/// directory first because std::filesystem::weakly_canonical does not reliably resolve a relative +/// path on its own. '..' components are collapsed and symlinks are resolved for the portion of the +/// path that exists, so a path naming a file that does not exist yet still succeeds. +/// Throws on failure. +/// +std::filesystem::path GetCanonicalPath(const std::filesystem::path& Path); + +/// +/// Non-throwing overload of GetCanonicalPath. On failure Error is set and an empty path is +/// returned; on success Error is cleared. +/// +std::filesystem::path GetCanonicalPath(const std::filesystem::path& Path, std::error_code& Error); + std::filesystem::path GetFullPath(_In_ LPCWSTR Path); std::pair GetHostAndDomainNames(); diff --git a/src/windows/service/exe/LxssUserSession.cpp b/src/windows/service/exe/LxssUserSession.cpp index c06eeaf94..60723945d 100644 --- a/src/windows/service/exe/LxssUserSession.cpp +++ b/src/windows/service/exe/LxssUserSession.cpp @@ -3847,7 +3847,7 @@ void LxssUserSessionImpl::_ValidateDistributionNameAndPathNotInUse( if (Path != nullptr) { - canonicalPath = std::filesystem::weakly_canonical(std::filesystem::absolute(Path, error), error); + canonicalPath = wsl::windows::common::filesystem::GetCanonicalPath(Path, error); if (error) { LOG_WIN32(error.value()); @@ -3891,7 +3891,7 @@ void LxssUserSessionImpl::_ValidateDistributionNameAndPathNotInUse( if (Path != nullptr) { - auto canonicalDistroPath = std::filesystem::weakly_canonical(std::filesystem::absolute(configuration.BasePath, error), error); + auto canonicalDistroPath = wsl::windows::common::filesystem::GetCanonicalPath(configuration.BasePath, error); if (error) { LOG_WIN32(error.value()); diff --git a/src/windows/service/exe/WslCoreVm.cpp b/src/windows/service/exe/WslCoreVm.cpp index b56bdba94..2eb87ae08 100644 --- a/src/windows/service/exe/WslCoreVm.cpp +++ b/src/windows/service/exe/WslCoreVm.cpp @@ -2182,7 +2182,7 @@ std::tuple WslCoreVm::AddVirtioFsShare sharePath.push_back(L'\\'); } - sharePath = std::filesystem::weakly_canonical(std::filesystem::absolute(sharePath)).wstring(); + sharePath = wsl::windows::common::filesystem::GetCanonicalPath(sharePath).wstring(); std::wstring effectiveOptions(Options); diff --git a/src/windows/wslc/arguments/SpecParsing.cpp b/src/windows/wslc/arguments/SpecParsing.cpp index fb3bfb857..8a5db3c36 100644 --- a/src/windows/wslc/arguments/SpecParsing.cpp +++ b/src/windows/wslc/arguments/SpecParsing.cpp @@ -167,10 +167,10 @@ services::BuildSecret ParseSecretSpec(const std::wstring& spec) // Normalize to an absolute path (the service requires one to mount the file's directory) but do // not verify the file exists or is a regular file here: that would be a TOCTOU race with the // build, and the file may only be reachable from the service's context. Let the service/BuildKit - // reject an unmountable or unreadable file instead. weakly_canonical resolves a relative path + // reject an unmountable or unreadable file instead. GetCanonicalPath resolves a relative path // against the current directory, collapses '..', and resolves symlinks for the portion of the // path that exists; it succeeds for a missing file but still reports genuine errors. - auto absPath = std::filesystem::weakly_canonical(std::filesystem::absolute(srcPath), ec); + auto absPath = wsl::windows::common::filesystem::GetCanonicalPath(srcPath, ec); if (ec.value() != 0) { throw ArgumentException( diff --git a/src/windows/wslc/services/ImageService.cpp b/src/windows/wslc/services/ImageService.cpp index 163c712c9..1f94db86c 100644 --- a/src/windows/wslc/services/ImageService.cpp +++ b/src/windows/wslc/services/ImageService.cpp @@ -266,7 +266,7 @@ void ImageService::Build( std::wstring iidPathStr; if (iidFilePath.has_value()) { - iidPathStr = std::filesystem::weakly_canonical(std::filesystem::absolute(*iidFilePath)).wstring(); + iidPathStr = wsl::windows::common::filesystem::GetCanonicalPath(*iidFilePath).wstring(); } WSLCBuildImageOptions options{ diff --git a/src/windows/wslc/tasks/ContainerTasks.cpp b/src/windows/wslc/tasks/ContainerTasks.cpp index 4bb61374e..7de5e65d4 100644 --- a/src/windows/wslc/tasks/ContainerTasks.cpp +++ b/src/windows/wslc/tasks/ContainerTasks.cpp @@ -400,7 +400,7 @@ void ContainerCp(CLIExecutionContext& context) // Resolve any symlinks in the target path since tar.exe refuses to extract through a symlink. std::error_code canonicalError; - auto absTarget = std::filesystem::weakly_canonical(std::filesystem::absolute(target), canonicalError); + auto absTarget = wsl::windows::common::filesystem::GetCanonicalPath(target, canonicalError); if (canonicalError) { absTarget = std::filesystem::absolute(target); // Fall back to absolute if canonicalization fails. diff --git a/src/windows/wslinstaller/exe/WslInstaller.cpp b/src/windows/wslinstaller/exe/WslInstaller.cpp index 7ac796d6c..1e9b1f06c 100644 --- a/src/windows/wslinstaller/exe/WslInstaller.cpp +++ b/src/windows/wslinstaller/exe/WslInstaller.cpp @@ -24,7 +24,7 @@ std::wstring GetMsiPackagePath() static_assert(!wsl::shared::OfficialBuild); - return std::filesystem::weakly_canonical(WSL_DEV_THIN_MSI_PACKAGE).wstring(); + return wsl::windows::common::filesystem::GetCanonicalPath(WSL_DEV_THIN_MSI_PACKAGE).wstring(); #endif @@ -50,7 +50,7 @@ try } // A canonical path is required because msiexec doesn't like symlinks. - return UpgradeLogInfo{std::filesystem::weakly_canonical(path), true}; + return UpgradeLogInfo{wsl::windows::common::filesystem::GetCanonicalPath(path), true}; } catch (...) { diff --git a/test/windows/CMakeLists.txt b/test/windows/CMakeLists.txt index 3b5c9253b..53aed61e1 100644 --- a/test/windows/CMakeLists.txt +++ b/test/windows/CMakeLists.txt @@ -5,6 +5,7 @@ set(SOURCES NetworkTests.cpp Plan9Tests.cpp DrvFsTests.cpp + FilesystemUnitTests.cpp Common.cpp PluginTests.cpp PolicyTests.cpp diff --git a/test/windows/FilesystemUnitTests.cpp b/test/windows/FilesystemUnitTests.cpp new file mode 100644 index 000000000..ae552e072 --- /dev/null +++ b/test/windows/FilesystemUnitTests.cpp @@ -0,0 +1,193 @@ +/*++ + +Copyright (c) Microsoft. All rights reserved. + +Module Name: + + FilesystemUnitTests.cpp + +Abstract: + + This file contains unit tests for the helpers in src/windows/common/filesystem.cpp. + These tests only touch the local filesystem so they do not require an installed distribution. + +--*/ + +#include "precomp.h" +#include "Common.h" + +using wsl::windows::common::filesystem::GetCanonicalPath; + +namespace { + +// Creates a uniquely named directory under the temp directory and removes it on destruction. +class ScopedTempDirectory +{ +public: + ScopedTempDirectory() + { + m_path = std::filesystem::temp_directory_path() / + (L"wsl_ut_canonical_" + std::to_wstring(GetCurrentProcessId()) + L"_" + std::to_wstring(++s_counter)); + std::filesystem::create_directories(m_path); + } + + ~ScopedTempDirectory() + { + std::error_code error; + std::filesystem::remove_all(m_path, error); + } + + ScopedTempDirectory(const ScopedTempDirectory&) = delete; + ScopedTempDirectory& operator=(const ScopedTempDirectory&) = delete; + + // The canonical form of the directory, which is what GetCanonicalPath is expected to resolve to. + // N.B. std::filesystem::canonical is used rather than weakly_canonical so the expected value is + // computed independently of the API under test. + std::filesystem::path Canonical() const + { + return std::filesystem::canonical(m_path); + } + + const std::filesystem::path& Path() const + { + return m_path; + } + +private: + std::filesystem::path m_path; + static inline int s_counter = 0; +}; + +// Sets the current directory for the lifetime of the object and restores the previous one on destruction. +class ScopedCurrentDirectory +{ +public: + explicit ScopedCurrentDirectory(const std::filesystem::path& Path) : m_previous(std::filesystem::current_path()) + { + std::filesystem::current_path(Path); + } + + ~ScopedCurrentDirectory() + { + std::error_code error; + std::filesystem::current_path(m_previous, error); + } + + ScopedCurrentDirectory(const ScopedCurrentDirectory&) = delete; + ScopedCurrentDirectory& operator=(const ScopedCurrentDirectory&) = delete; + +private: + std::filesystem::path m_previous; +}; + +// Returns a file name that is guaranteed not to exist in the given directory. +std::wstring UniqueMissingName(const std::filesystem::path& Directory) +{ + const auto name = L"missing_" + std::to_wstring(GetCurrentProcessId()) + L"_" + std::to_wstring(GetTickCount64()) + L".txt"; + VERIFY_IS_FALSE(std::filesystem::exists(Directory / name)); + return name; +} + +} // namespace + +namespace FilesystemUnitTests { +class FilesystemUnitTests +{ + WSL_TEST_CLASS(FilesystemUnitTests) + + // A relative path naming a file that does not exist must still resolve to an absolute path. + // This is the case std::filesystem::weakly_canonical cannot handle on its own: it builds its result + // from the longest leading sequence of elements that exist, so a bare missing file name has nothing + // to canonicalize and is returned unchanged. + TEST_METHOD(GetCanonicalPath_RelativeMissingPathIsMadeAbsolute) + { + ScopedTempDirectory directory; + const auto name = UniqueMissingName(directory.Path()); + + ScopedCurrentDirectory scopedDirectory(directory.Path()); + + // Establish that the input is relative and that weakly_canonical alone leaves it that way. + VERIFY_IS_FALSE(std::filesystem::path(name).is_absolute()); + VERIFY_IS_FALSE(std::filesystem::weakly_canonical(name).is_absolute()); + + const auto result = GetCanonicalPath(name); + VERIFY_IS_TRUE(result.is_absolute()); + VERIFY_ARE_EQUAL((directory.Canonical() / name).wstring(), result.wstring()); + } + + // The same resolution must happen for a relative path whose target already exists. + TEST_METHOD(GetCanonicalPath_RelativeExistingPathIsMadeAbsolute) + { + ScopedTempDirectory directory; + const std::wstring name = L"existing.txt"; + std::ofstream(directory.Path() / name).put('x'); + VERIFY_IS_TRUE(std::filesystem::exists(directory.Path() / name)); + + ScopedCurrentDirectory scopedDirectory(directory.Path()); + + const auto result = GetCanonicalPath(name); + VERIFY_IS_TRUE(result.is_absolute()); + VERIFY_ARE_EQUAL((directory.Canonical() / name).wstring(), result.wstring()); + } + + // '.' and '..' components must be collapsed even when the intermediate directory does not exist. + TEST_METHOD(GetCanonicalPath_CollapsesDotSegments) + { + ScopedTempDirectory directory; + const auto name = UniqueMissingName(directory.Path()); + + ScopedCurrentDirectory scopedDirectory(directory.Path()); + + const auto result = GetCanonicalPath(L".\\nonexistent\\..\\" + name); + VERIFY_ARE_EQUAL((directory.Canonical() / name).wstring(), result.wstring()); + } + + // An already absolute path must be returned unchanged. + TEST_METHOD(GetCanonicalPath_AbsolutePathIsUnchanged) + { + ScopedTempDirectory directory; + const auto expected = directory.Canonical() / UniqueMissingName(directory.Path()); + + VERIFY_ARE_EQUAL(expected.wstring(), GetCanonicalPath(expected).wstring()); + } + + // A failure must be reported through Error rather than thrown. + // N.B. An empty path is rejected by std::filesystem::absolute. This is the case the previous + // weakly_canonical(absolute(Path, error), error) idiom silently dropped, because weakly_canonical + // clears the error_code on success and therefore erased the failure absolute had just reported. + TEST_METHOD(GetCanonicalPath_ErrorOverloadReportsFailure) + { + std::error_code error; + const auto result = GetCanonicalPath(std::filesystem::path{}, error); + + VERIFY_IS_TRUE(!!error); + VERIFY_IS_TRUE(result.empty()); + } + + // Error must be cleared when the call succeeds so callers can reuse the same variable. + TEST_METHOD(GetCanonicalPath_ErrorOverloadClearsErrorOnSuccess) + { + ScopedTempDirectory directory; + const auto expected = directory.Canonical() / UniqueMissingName(directory.Path()); + + auto error = std::make_error_code(std::errc::permission_denied); + const auto result = GetCanonicalPath(expected, error); + + VERIFY_IS_FALSE(!!error); + VERIFY_ARE_EQUAL(expected.wstring(), result.wstring()); + } + + // The throwing overload must surface the same failure the non-throwing overload reports. + TEST_METHOD(GetCanonicalPath_ThrowingOverloadSurfacesFailure) + { + std::error_code error; + (void)GetCanonicalPath(std::filesystem::path{}, error); + VERIFY_IS_TRUE(!!error); + + const auto expectedResult = HRESULT_FROM_WIN32(error.value()); + VERIFY_THROWS_SPECIFIC(GetCanonicalPath(std::filesystem::path{}), wil::ResultException, [&](const wil::ResultException& e) { + return e.GetErrorCode() == expectedResult; + }); + } +}; +} // namespace FilesystemUnitTests \ No newline at end of file diff --git a/test/windows/InstallerTests.cpp b/test/windows/InstallerTests.cpp index a988fa566..0648f5283 100644 --- a/test/windows/InstallerTests.cpp +++ b/test/windows/InstallerTests.cpp @@ -50,7 +50,7 @@ class InstallerTests WEX::Common::String MsixPackagePath; WEX::TestExecution::RuntimeParameters::TryGetValue(L"Package", MsixPackagePath); - m_msixPackagePath = std::filesystem::weakly_canonical(static_cast(MsixPackagePath)).wstring(); + m_msixPackagePath = wsl::windows::common::filesystem::GetCanonicalPath(static_cast(MsixPackagePath)).wstring(); VERIFY_IS_FALSE(m_msixPackagePath.empty()); for (const auto& e : m_packageManager.FindPackages(wsl::windows::common::wslutil::c_msixPackageFamilyName)) @@ -61,7 +61,7 @@ class InstallerTests #ifdef WSL_DEV_THIN_MSI_PACKAGE - m_msiPath = std::filesystem::weakly_canonical(WSL_DEV_THIN_MSI_PACKAGE).wstring(); + m_msiPath = wsl::windows::common::filesystem::GetCanonicalPath(WSL_DEV_THIN_MSI_PACKAGE).wstring(); #else @@ -383,12 +383,12 @@ class InstallerTests if (auto found = L"wsl." + version + arch + L".msi"; PathFileExists(found.c_str())) { - installerFile = std::filesystem::weakly_canonical(found); + installerFile = wsl::windows::common::filesystem::GetCanonicalPath(found); cleanup.release(); } else if (auto found = L"Microsoft.WSL_" + version + L".0_x64_ARM64.msixbundle"; PathFileExists(found.c_str())) { - installerFile = std::filesystem::weakly_canonical(found); + installerFile = wsl::windows::common::filesystem::GetCanonicalPath(found); cleanup.release(); } else diff --git a/test/windows/UnitTests.cpp b/test/windows/UnitTests.cpp index bade022ee..8b7d60206 100644 --- a/test/windows/UnitTests.cpp +++ b/test/windows/UnitTests.cpp @@ -2622,7 +2622,7 @@ Error code: Wsl/InstallDistro/WSL_E_DISTRO_NOT_FOUND WSL2_TEST_METHOD(CorruptedVhd) { // Create a 100MB vhd without a filesystem. - auto distroPath = std::filesystem::weakly_canonical(wil::GetCurrentDirectoryW()); + auto distroPath = wsl::windows::common::filesystem::GetCanonicalPath(wil::GetCurrentDirectoryW()); auto vhdPath = distroPath / L"CorruptedTest.vhdx"; VIRTUAL_STORAGE_TYPE storageType{}; @@ -3083,7 +3083,7 @@ Error code: Wsl/InstallDistro/WSL_E_DISTRO_NOT_FOUND VERIFY_IS_TRUE(std::filesystem::exists(std::format(L"{}\\ext4.vhdx", testFolder))); } - auto absolutePath = std::filesystem::weakly_canonical(".").wstring(); + auto absolutePath = wsl::windows::common::filesystem::GetCanonicalPath(".").wstring(); // Move the distro to a different folder (absolute path) { diff --git a/test/windows/WSLCTests.cpp b/test/windows/WSLCTests.cpp index cd3085375..4f075d94b 100644 --- a/test/windows/WSLCTests.cpp +++ b/test/windows/WSLCTests.cpp @@ -9369,8 +9369,9 @@ class WSLCTests WSLC_TEST_METHOD(ContainerVolumesAdvanced) { - auto hostFolder = std::filesystem::weakly_canonical(std::filesystem::current_path() / "test-volume"); - auto symlinkFolder = std::filesystem::weakly_canonical(std::filesystem::current_path() / "test-volume-symlink"); + auto hostFolder = wsl::windows::common::filesystem::GetCanonicalPath(std::filesystem::current_path() / "test-volume"); + auto symlinkFolder = + wsl::windows::common::filesystem::GetCanonicalPath(std::filesystem::current_path() / "test-volume-symlink"); std::filesystem::create_directories(hostFolder); auto cleanup = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [&]() { From d1a9e26f03adeec64e2ea84e3ca12d54711f8f32 Mon Sep 17 00:00:00 2001 From: Gavin Garzia Date: Tue, 11 Aug 2026 10:54:43 -0700 Subject: [PATCH 3/5] Include filesystem.hpp directly where GetCanonicalPath is used Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/windows/wslc/services/ImageService.cpp | 1 + src/windows/wslc/tasks/ContainerTasks.cpp | 1 + 2 files changed, 2 insertions(+) diff --git a/src/windows/wslc/services/ImageService.cpp b/src/windows/wslc/services/ImageService.cpp index d057cc8c4..0e5447432 100644 --- a/src/windows/wslc/services/ImageService.cpp +++ b/src/windows/wslc/services/ImageService.cpp @@ -16,6 +16,7 @@ Module Name: #include "SessionService.h" #include "SpecParsing.h" #include "WarningCallback.h" +#include #include #include #include diff --git a/src/windows/wslc/tasks/ContainerTasks.cpp b/src/windows/wslc/tasks/ContainerTasks.cpp index bc2aed562..746f68214 100644 --- a/src/windows/wslc/tasks/ContainerTasks.cpp +++ b/src/windows/wslc/tasks/ContainerTasks.cpp @@ -23,6 +23,7 @@ Module Name: #include "SessionService.h" #include "TableOutput.h" #include +#include #include #include From 83fa7ab5b2991f9f1d46cefe2f20d01ec8c50743 Mon Sep 17 00:00:00 2001 From: Gavin Garzia Date: Tue, 11 Aug 2026 14:00:14 -0700 Subject: [PATCH 4/5] Simplify the filesystem unit tests so they no longer create files or directories Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/windows/common/filesystem.cpp | 2 +- test/windows/FilesystemUnitTests.cpp | 128 ++++++++------------------- 2 files changed, 38 insertions(+), 92 deletions(-) diff --git a/src/windows/common/filesystem.cpp b/src/windows/common/filesystem.cpp index 88011ae70..fdadc2ad9 100644 --- a/src/windows/common/filesystem.cpp +++ b/src/windows/common/filesystem.cpp @@ -785,7 +785,7 @@ std::filesystem::path wsl::windows::common::filesystem::GetCanonicalPath(const s std::filesystem::path wsl::windows::common::filesystem::GetCanonicalPath(const std::filesystem::path& Path, std::error_code& Error) { - // N.B. absolute() is applied first because weakly_canonical() does not resolve a relative path + // absolute() is applied first because weakly_canonical() does not resolve a relative path // against the current directory on its own. Its result is checked before canonicalizing because // weakly_canonical() clears Error on success, which would otherwise mask an absolute() failure. const auto absolutePath = std::filesystem::absolute(Path, Error); diff --git a/test/windows/FilesystemUnitTests.cpp b/test/windows/FilesystemUnitTests.cpp index ae552e072..bd7e03fec 100644 --- a/test/windows/FilesystemUnitTests.cpp +++ b/test/windows/FilesystemUnitTests.cpp @@ -9,7 +9,7 @@ Module Name: Abstract: This file contains unit tests for the helpers in src/windows/common/filesystem.cpp. - These tests only touch the local filesystem so they do not require an installed distribution. + These tests only read from the local filesystem so they do not require an installed distribution. --*/ @@ -20,72 +20,29 @@ using wsl::windows::common::filesystem::GetCanonicalPath; namespace { -// Creates a uniquely named directory under the temp directory and removes it on destruction. -class ScopedTempDirectory +// Returns a file name that does not exist in the given directory. +std::wstring UniqueMissingName(const std::filesystem::path& Directory) { -public: - ScopedTempDirectory() - { - m_path = std::filesystem::temp_directory_path() / - (L"wsl_ut_canonical_" + std::to_wstring(GetCurrentProcessId()) + L"_" + std::to_wstring(++s_counter)); - std::filesystem::create_directories(m_path); - } - - ~ScopedTempDirectory() - { - std::error_code error; - std::filesystem::remove_all(m_path, error); - } - - ScopedTempDirectory(const ScopedTempDirectory&) = delete; - ScopedTempDirectory& operator=(const ScopedTempDirectory&) = delete; - - // The canonical form of the directory, which is what GetCanonicalPath is expected to resolve to. - // N.B. std::filesystem::canonical is used rather than weakly_canonical so the expected value is - // computed independently of the API under test. - std::filesystem::path Canonical() const - { - return std::filesystem::canonical(m_path); - } - - const std::filesystem::path& Path() const - { - return m_path; - } + static int counter = 0; + const auto name = std::format(L"wsl_ut_canonical_{}_{}.txt", GetCurrentProcessId(), ++counter); + VERIFY_IS_FALSE(std::filesystem::exists(Directory / name)); -private: - std::filesystem::path m_path; - static inline int s_counter = 0; -}; + return name; +} -// Sets the current directory for the lifetime of the object and restores the previous one on destruction. -class ScopedCurrentDirectory +// The canonical form of the current directory, which is what a relative path is expected to resolve +// against. std::filesystem::canonical is used rather than weakly_canonical so the expected value is +// computed independently of the API under test. +std::filesystem::path CanonicalCurrentDirectory() { -public: - explicit ScopedCurrentDirectory(const std::filesystem::path& Path) : m_previous(std::filesystem::current_path()) - { - std::filesystem::current_path(Path); - } - - ~ScopedCurrentDirectory() - { - std::error_code error; - std::filesystem::current_path(m_previous, error); - } - - ScopedCurrentDirectory(const ScopedCurrentDirectory&) = delete; - ScopedCurrentDirectory& operator=(const ScopedCurrentDirectory&) = delete; - -private: - std::filesystem::path m_previous; -}; + return std::filesystem::canonical(std::filesystem::current_path()); +} -// Returns a file name that is guaranteed not to exist in the given directory. -std::wstring UniqueMissingName(const std::filesystem::path& Directory) +// A file that is known to exist, used to cover paths that resolve to a real filesystem entry. The +// test module itself is used so that no file has to be created. +std::filesystem::path ExistingFile() { - const auto name = L"missing_" + std::to_wstring(GetCurrentProcessId()) + L"_" + std::to_wstring(GetTickCount64()) + L".txt"; - VERIFY_IS_FALSE(std::filesystem::exists(Directory / name)); - return name; + return {wil::GetModuleFileNameW(wil::GetModuleInstanceHandle())}; } } // namespace @@ -96,63 +53,53 @@ class FilesystemUnitTests WSL_TEST_CLASS(FilesystemUnitTests) // A relative path naming a file that does not exist must still resolve to an absolute path. - // This is the case std::filesystem::weakly_canonical cannot handle on its own: it builds its result - // from the longest leading sequence of elements that exist, so a bare missing file name has nothing - // to canonicalize and is returned unchanged. + // std::filesystem::weakly_canonical cannot do this on its own: it builds its result from the + // longest leading sequence of elements that exist, so a bare missing file name has nothing to + // canonicalize and is returned unchanged. TEST_METHOD(GetCanonicalPath_RelativeMissingPathIsMadeAbsolute) { - ScopedTempDirectory directory; - const auto name = UniqueMissingName(directory.Path()); - - ScopedCurrentDirectory scopedDirectory(directory.Path()); - - // Establish that the input is relative and that weakly_canonical alone leaves it that way. - VERIFY_IS_FALSE(std::filesystem::path(name).is_absolute()); + const auto name = UniqueMissingName(std::filesystem::current_path()); VERIFY_IS_FALSE(std::filesystem::weakly_canonical(name).is_absolute()); const auto result = GetCanonicalPath(name); + VERIFY_IS_TRUE(result.is_absolute()); - VERIFY_ARE_EQUAL((directory.Canonical() / name).wstring(), result.wstring()); + VERIFY_ARE_EQUAL((CanonicalCurrentDirectory() / name).wstring(), result.wstring()); } // The same resolution must happen for a relative path whose target already exists. TEST_METHOD(GetCanonicalPath_RelativeExistingPathIsMadeAbsolute) { - ScopedTempDirectory directory; - const std::wstring name = L"existing.txt"; - std::ofstream(directory.Path() / name).put('x'); - VERIFY_IS_TRUE(std::filesystem::exists(directory.Path() / name)); + const auto existing = ExistingFile(); + const auto relativePath = std::filesystem::relative(existing, std::filesystem::current_path()); + VERIFY_IS_FALSE(relativePath.empty()); + VERIFY_IS_FALSE(relativePath.is_absolute()); - ScopedCurrentDirectory scopedDirectory(directory.Path()); + const auto result = GetCanonicalPath(relativePath); - const auto result = GetCanonicalPath(name); VERIFY_IS_TRUE(result.is_absolute()); - VERIFY_ARE_EQUAL((directory.Canonical() / name).wstring(), result.wstring()); + VERIFY_ARE_EQUAL(std::filesystem::canonical(existing).wstring(), result.wstring()); } // '.' and '..' components must be collapsed even when the intermediate directory does not exist. TEST_METHOD(GetCanonicalPath_CollapsesDotSegments) { - ScopedTempDirectory directory; - const auto name = UniqueMissingName(directory.Path()); - - ScopedCurrentDirectory scopedDirectory(directory.Path()); + const auto name = UniqueMissingName(std::filesystem::current_path()); const auto result = GetCanonicalPath(L".\\nonexistent\\..\\" + name); - VERIFY_ARE_EQUAL((directory.Canonical() / name).wstring(), result.wstring()); + + VERIFY_ARE_EQUAL((CanonicalCurrentDirectory() / name).wstring(), result.wstring()); } // An already absolute path must be returned unchanged. TEST_METHOD(GetCanonicalPath_AbsolutePathIsUnchanged) { - ScopedTempDirectory directory; - const auto expected = directory.Canonical() / UniqueMissingName(directory.Path()); + const auto expected = CanonicalCurrentDirectory() / UniqueMissingName(std::filesystem::current_path()); VERIFY_ARE_EQUAL(expected.wstring(), GetCanonicalPath(expected).wstring()); } - // A failure must be reported through Error rather than thrown. - // N.B. An empty path is rejected by std::filesystem::absolute. This is the case the previous + // An empty path is rejected by std::filesystem::absolute. This is the case the previous // weakly_canonical(absolute(Path, error), error) idiom silently dropped, because weakly_canonical // clears the error_code on success and therefore erased the failure absolute had just reported. TEST_METHOD(GetCanonicalPath_ErrorOverloadReportsFailure) @@ -167,8 +114,7 @@ class FilesystemUnitTests // Error must be cleared when the call succeeds so callers can reuse the same variable. TEST_METHOD(GetCanonicalPath_ErrorOverloadClearsErrorOnSuccess) { - ScopedTempDirectory directory; - const auto expected = directory.Canonical() / UniqueMissingName(directory.Path()); + const auto expected = CanonicalCurrentDirectory() / UniqueMissingName(std::filesystem::current_path()); auto error = std::make_error_code(std::errc::permission_denied); const auto result = GetCanonicalPath(expected, error); @@ -190,4 +136,4 @@ class FilesystemUnitTests }); } }; -} // namespace FilesystemUnitTests \ No newline at end of file +} // namespace FilesystemUnitTests From 76219537e21bd429adffd8136510046f73a7a82a Mon Sep 17 00:00:00 2001 From: Gavin Garzia Date: Tue, 11 Aug 2026 15:43:58 -0700 Subject: [PATCH 5/5] Use a path Win32 always rejects instead of an empty one in the filesystem unit tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- test/windows/FilesystemUnitTests.cpp | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/test/windows/FilesystemUnitTests.cpp b/test/windows/FilesystemUnitTests.cpp index bd7e03fec..e9fb3b5f9 100644 --- a/test/windows/FilesystemUnitTests.cpp +++ b/test/windows/FilesystemUnitTests.cpp @@ -38,6 +38,14 @@ std::filesystem::path CanonicalCurrentDirectory() return std::filesystem::canonical(std::filesystem::current_path()); } +// A path that std::filesystem::absolute is guaranteed to reject, because it exceeds the longest path +// Win32 can express. An empty path is not used: whether absolute rejects one is implementation +// defined, and some standard library versions accept it. +std::filesystem::path UnresolvablePath() +{ + return {L"C:\\" + std::wstring(40000, L'a')}; +} + // A file that is known to exist, used to cover paths that resolve to a real filesystem entry. The // test module itself is used so that no file has to be created. std::filesystem::path ExistingFile() @@ -99,15 +107,15 @@ class FilesystemUnitTests VERIFY_ARE_EQUAL(expected.wstring(), GetCanonicalPath(expected).wstring()); } - // An empty path is rejected by std::filesystem::absolute. This is the case the previous - // weakly_canonical(absolute(Path, error), error) idiom silently dropped, because weakly_canonical - // clears the error_code on success and therefore erased the failure absolute had just reported. + // A failure from std::filesystem::absolute must be reported. absolute returns an empty path when + // it fails, and weakly_canonical succeeds on an empty path and clears the error_code, so calling + // the two in sequence without checking in between silently turns the failure into success. TEST_METHOD(GetCanonicalPath_ErrorOverloadReportsFailure) { std::error_code error; - const auto result = GetCanonicalPath(std::filesystem::path{}, error); + const auto result = GetCanonicalPath(UnresolvablePath(), error); - VERIFY_IS_TRUE(!!error); + VERIFY_ARE_NOT_EQUAL(std::error_code{}, error); VERIFY_IS_TRUE(result.empty()); } @@ -119,7 +127,7 @@ class FilesystemUnitTests auto error = std::make_error_code(std::errc::permission_denied); const auto result = GetCanonicalPath(expected, error); - VERIFY_IS_FALSE(!!error); + VERIFY_ARE_EQUAL(std::error_code{}, error); VERIFY_ARE_EQUAL(expected.wstring(), result.wstring()); } @@ -127,11 +135,11 @@ class FilesystemUnitTests TEST_METHOD(GetCanonicalPath_ThrowingOverloadSurfacesFailure) { std::error_code error; - (void)GetCanonicalPath(std::filesystem::path{}, error); - VERIFY_IS_TRUE(!!error); + (void)GetCanonicalPath(UnresolvablePath(), error); + VERIFY_ARE_NOT_EQUAL(std::error_code{}, error); const auto expectedResult = HRESULT_FROM_WIN32(error.value()); - VERIFY_THROWS_SPECIFIC(GetCanonicalPath(std::filesystem::path{}), wil::ResultException, [&](const wil::ResultException& e) { + VERIFY_THROWS_SPECIFIC(GetCanonicalPath(UnresolvablePath()), wil::ResultException, [&](const wil::ResultException& e) { return e.GetErrorCode() == expectedResult; }); }