chrootarchive: use archive untar implementation on Windows - #107
chrootarchive: use archive untar implementation on Windows#107thaJeztah wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #107 +/- ##
==========================================
+ Coverage 65.25% 65.26% +0.01%
==========================================
Files 46 46
Lines 2377 2398 +21
==========================================
+ Hits 1551 1565 +14
- Misses 606 610 +4
- Partials 220 223 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
The Windows extraction path currently does not enforce the UntarWithRoot containment contract (root vs dest), which is a security-relevant behavior gap in the modified code path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Refactors chrootarchive extraction so Windows no longer duplicates decompression/untar logic and instead delegates directly to archive.Untar / archive.UntarUncompressed, while keeping the chroot-based path on non-Windows platforms.
Changes:
- Moved the shared
untarHandlerimplementation into the non-Windows (!windows) implementation. - Replaced the Windows
invokeUnpackpath with direct calls toarchive.Untar/archive.UntarUncompressed. - Adjusted imports and platform-specific destination setup to match the new split.
File summaries
| File | Description |
|---|---|
| chrootarchive/archive.go | Removes the shared untarHandler from the common file and relies on platform-specific implementations. |
| chrootarchive/archive_windows.go | Implements Windows untarHandler by preparing destination and delegating to archive.Untar*. |
| chrootarchive/archive_unix.go | Adds the chroot-based untarHandler implementation for non-Windows builds. |
Review details
Suppressed comments (2)
chrootarchive/archive_windows.go:39
- This comment refers to "entering the chroot", but Windows explicitly does not support chroot; the wording is misleading for this platform-specific implementation.
// Create dest here only if it is the root itself; paths below the root are
// created by the extractor after entering the chroot.
// This case is only currently used by cp.
chrootarchive/archive_windows.go:43
- Passing mode 0 to os.MkdirAll is unusual and makes intent unclear; use a conventional directory mode (it is ignored on Windows but keeps behavior clear and consistent).
if err := os.MkdirAll(dest, 0); err != nil {
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions, decompress bool, root string) error { | ||
| if tarArchive == nil { | ||
| return errors.New("empty archive") | ||
| } | ||
|
|
There was a problem hiding this comment.
Yup, pre-existing issue, and one of the reasons I opened this PR. Peeling away a level of abstraction to make it clear that we're not actually using chrootarchive here.
Possibly on Windows we can do a os.Root based equivalent.
Windows does not support chroot-based extraction, so there is no need to duplicate archive's decompression and untar handling. Move the existing chroot-based handler to the Unix implementation. On Windows, retain the existing destination setup, then delegate directly to archive.Untar or archive.UntarUncompressed. This keeps the Unix behavior unchanged while removing the redundant Windows invokeUnpack path. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
e8c3b2d to
54dddcd
Compare
Windows does not support chroot-based extraction, so there is no need to duplicate archive's decompression and untar handling.
Move the existing chroot-based handler to the Unix implementation. On Windows, retain the existing destination setup, then delegate directly to archive.Untar or archive.UntarUncompressed.
This keeps the Unix behavior unchanged while removing the redundant Windows invokeUnpack path.