Make paths absolute before weakly_canonical and add regression tests - #41290
Open
ggarzia-MSFT wants to merge 2 commits into
Open
Make paths absolute before weakly_canonical and add regression tests#41290ggarzia-MSFT wants to merge 2 commits into
ggarzia-MSFT wants to merge 2 commits into
Conversation
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 <relative>` and `--secret src=<relative>` 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>
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes cases where std::filesystem::weakly_canonical() can return a relative path when the leading element(s) don’t exist yet (e.g., output files or missing secret source files), causing the WSL service to reject the path as non-absolute. It updates affected call sites to make paths absolute before canonicalization and adds regression tests to cover the previously-missed scenarios.
Changes:
- Make path inputs absolute before calling
weakly_canonical()for--secret src=, virtiofs share paths, and distro path comparisons. - Add a parser unit test for a relative
--secret src=pointing to a missing file. - Add an E2E test validating
wslc build --iidfileaccepts a relative path from the caller’s working directory.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/windows/wslc/WSLCCLISecretParserUnitTests.cpp | Adds missing-file relative src= regression test; updates expected canonicalization to include absolute(...). |
| test/windows/wslc/e2e/WSLCE2EImageBuildTests.cpp | Adds E2E coverage for relative --iidfile path resolution and records new image name constant. |
| src/windows/wslc/arguments/SpecParsing.cpp | Adjusts secret src= canonicalization to force absolute paths before weakly_canonical(). |
| src/windows/service/exe/WslCoreVm.cpp | Ensures virtiofs share paths are made absolute before weakly_canonical(). |
| src/windows/service/exe/LxssUserSession.cpp | Ensures distro install paths are made absolute before weakly_canonical() comparisons. |
…ation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
401
to
406
| // 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. |
Comment on lines
264
to
270
| // Resolve the --iidfile destination against the client's working directory; the server mounts its | ||
| // parent directory read-write into the VM so buildx writes the image ID straight to it. | ||
| 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(); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 <relative>and--secret src=<relative>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.idfrom the context directory.Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed