You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
refactor container cp task to be simpler and move logic to container service and common filesystem helpers - #41668
This pull request refactors and improves the filesystem utilities used for staging directories, file name validation, and archive extraction. The main enhancements include encapsulating staging directory cleanup in a RAII class, centralizing file/directory move logic, and adding robust validation and tests for file name representability. This results in safer, more maintainable, and more reliable file operations.
This pull request also moves most of the logic out of the ContainerCp method in ContainerTasks.cpp into the Copy method in ContainerService.cpp. Additionally, the parseContainerPath lambda in Copy has been updated to throw if given a bad container path rather than silently deforming the path
Filesystem utility enhancements:
Added IsRepresentableFileName to determine if a given string can be used as a Windows file name, preventing invalid names from being used in file operations.
Introduced the StagingDirectory RAII class, which automatically cleans up temporary directories when they go out of scope, reducing manual cleanup code and risk of leftover files.
Centralized and improved the logic for moving files/directories with the MoveOver function, now handling more edge cases and used consistently.
Archive extraction and staging improvements:
Added ExtractArchiveInto and StageDereferencedTree utilities to streamline extraction of tar streams and dereferencing of symlinks, making archive handling more robust and less error-prone.
Refactored ContainerCp to use the new utilities, resulting in simpler, safer, and more maintainable code for container copy operations.
Testing improvements:
Added comprehensive unit tests for IsRepresentableFileName and StagingDirectory, ensuring correct behavior and preventing regressions.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
It introduces correctness and build/maintainability issues (filename validation gaps, non-static helper symbols, missing header include, and brace-style inconsistency) that should be addressed before merging.
Get a fresh assessment by requesting another Copilot review.
Refactors the wslc cp implementation by moving the bulk of copy logic from the CLI task layer into ContainerService, and introduces/expands shared filesystem helpers (staging, archive extraction, and filename validation) to make copy/extraction safer and easier to maintain.
Changes:
Moved ContainerCp logic into ContainerService::Copy() and simplified the CLI task wrapper.
Added filesystem utilities: StagingDirectory (RAII cleanup), ExtractArchiveInto, StageDereferencedTree, and IsRepresentableFileName.
Added/extended unit tests for staging directory lifetime and filename representability.
File
Description
test/windows/FilesystemUnitTests.cpp
Adds unit coverage for new filename validation and RAII staging directory behavior.
src/windows/wslc/tasks/ContainerTasks.cpp
Simplifies ContainerCp to delegate to ContainerService::Copy.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
IsRepresentableFileName currently does not enforce key Win32 filename constraints (device names, trailing dot/space) and helper functions in filesystem.cpp leak as global symbols.
ExtractTarStream() is a file-local helper but is currently emitted as a global symbol from filesystem.cpp. Give it internal linkage (e.g. make it static or put it in an unnamed namespace).
The reason will be displayed to describe this comment to others. Learn more.
I like this PR a lot, but I think we can do a little more refactoring in the service tasks, possibly break it up into multiple tasks or at least a few helpers. The logic is still hard to read, which means it may have bugs lurking in there.
The reason will be displayed to describe this comment to others. Learn more.
The shape here is a clear improvement - the CLI task delegating into a helper, file destinations staging before they are moved into place, followed links keeping the requested basename, and the reserved-name and trailing dot or space checks handled up front instead of producing broken host entries.
The one thing I would like before I sign off is a regression test for the symlink form of tar traversal, not only dir/../file. This helper now centralizes archive extraction, and container cp consumes an archive built from container-controlled filesystem state, so the classic escape is an entry link -> <path outside the destination> followed by an entry under link/child. An absolute-path entry is worth pinning too. If tar.exe already refuses both, even better - having that stated in FilesystemUnitTests is what makes the helper safe to reuse later without someone re-deriving the argument.
The existing coverage for reserved Windows names, trailing dot and space, .. extraction, trailing slash handling, the stdout and stdin guards, and the directory-to-file rejection all look right to me, and CI is green at the head. This is a small ask on top of work that is already going the right direction.
The reason will be displayed to describe this comment to others. Learn more.
Took a pass over the archive extraction path. The two bot comments on filesystem.cpp are not noise - I reproduced both behaviours against the installed Windows tar.exe on this box, so I have left the evidence inline rather than just agreeing with them.
Nothing else jumped out; the rest reads fine. Holding off on a sign-off until wsl-github-pr comes back green and those two are settled.
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
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.
David Bennett (dkbennett)
left a comment
Blue (OneBlue)
left a comment
ranm-msft
left a comment
This pull request refactors and improves the filesystem utilities used for staging directories, file name validation, and archive extraction. The main enhancements include encapsulating staging directory cleanup in a RAII class, centralizing file/directory move logic, and adding robust validation and tests for file name representability. This results in safer, more maintainable, and more reliable file operations.
This pull request also moves most of the logic out of the
ContainerCpmethod inContainerTasks.cppinto theCopymethod inContainerService.cpp. Additionally, theparseContainerPathlambda inCopyhas been updated to throw if given a bad container path rather than silently deforming the pathFilesystem utility enhancements:
IsRepresentableFileNameto determine if a given string can be used as a Windows file name, preventing invalid names from being used in file operations.StagingDirectoryRAII class, which automatically cleans up temporary directories when they go out of scope, reducing manual cleanup code and risk of leftover files.MoveOverfunction, now handling more edge cases and used consistently.Archive extraction and staging improvements:
ExtractArchiveIntoandStageDereferencedTreeutilities to streamline extraction of tar streams and dereferencing of symlinks, making archive handling more robust and less error-prone.ContainerCpto use the new utilities, resulting in simpler, safer, and more maintainable code for container copy operations.Testing improvements:
IsRepresentableFileNameandStagingDirectory, ensuring correct behavior and preventing regressions.