archive: fix creation time updates on Windows - #79
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #79 +/- ##
==========================================
- Coverage 65.81% 64.52% -1.30%
==========================================
Files 42 44 +2
Lines 2039 2258 +219
==========================================
+ Hits 1342 1457 +115
- Misses 519 597 +78
- Partials 178 204 +26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Failing with os.Root.Chtimes; |
05d98bd to
0677695
Compare
| func openForWriteAttributesAt(parent windows.Handle, name string, noFollow bool) (windows.Handle, error) { | ||
| name16, err := windows.UTF16FromString(name) | ||
| if err != nil { | ||
| return windows.InvalidHandle, err | ||
| } | ||
|
|
||
| pathp, err := windows.UTF16PtrFromString(name) | ||
| if err != nil { | ||
| return err | ||
| attrs := uint32(windows.OBJ_CASE_INSENSITIVE) | ||
| if noFollow { | ||
| attrs |= windows.OBJ_DONT_REPARSE | ||
| } | ||
| h, err := windows.CreateFile(pathp, | ||
| windows.FILE_WRITE_ATTRIBUTES, windows.FILE_SHARE_WRITE, nil, | ||
| windows.OPEN_EXISTING, windows.FILE_FLAG_BACKUP_SEMANTICS, 0) | ||
|
|
||
| var handle windows.Handle | ||
| err = windows.NtCreateFile( | ||
| &handle, | ||
| windows.SYNCHRONIZE|windows.FILE_WRITE_ATTRIBUTES, | ||
| &windows.OBJECT_ATTRIBUTES{ | ||
| Length: uint32(unsafe.Sizeof(windows.OBJECT_ATTRIBUTES{})), | ||
| RootDirectory: parent, | ||
| ObjectName: &windows.NTUnicodeString{ | ||
| Length: uint16((len(name16) - 1) * 2), // #nosec G115 -- Length is USHORT by definition. A Windows path component cannot exceed uint16 bytes. |
There was a problem hiding this comment.
This was more convoluted than I hoped; the TL;DR;
os.Root.Chtimesdoes NOT set creation time.- Our old code did, but did not have the protection from
os.Root(first calledos.Chmod) - neither
os.Root, norgolang.org/x/sys/windowshave awindows.OpenAt(or similar) utility - Golang stdlib itself DOES, but they MADE IT INTERNAL! 😠 https://github.com/golang/go/blob/go1.26.0/src/internal/syscall/windows/at_windows.go#L28-L41
const (
O_NOFOLLOW_ANY = 0x200000000 // disallow symlinks anywhere in the path
O_WRITE_ATTRS = 0x800000000 // FILE_WRITE_ATTRIBUTES, used by Chmod
)
func Openat(dirfd syscall.Handle, name string, flag uint64, perm uint32) (_ syscall.Handle, e1 error) {
if len(name) == 0 {
return syscall.InvalidHandle, syscall.ERROR_FILE_NOT_FOUND
}
var access, options uint32
// Map Win32 file flags to NT create options.
fileFlags := uint32(flag) & FileFlagsMask
if fileFlags&^ValidFileFlagsMask != 0 {There was a problem hiding this comment.
Golang stdlib itself DOES, but they MADE IT INTERNAL! 😠
https://github.com/golang/go/blob/go1.26.0/src/internal/syscall/windows/at_windows.go#L28-L41
- This PR forks their internal package, so that we can use
windows.Openatarchive: rewrite chtimesAt using windows.Openat #88
There was a problem hiding this comment.
Pull request overview
This PR fixes a Windows regression where applying timestamps during archive extraction stopped updating file creation time after refactoring to os.Root-relative operations. It reintroduces creation-time updates on Windows and adds a Windows-only regression test to prevent future regressions.
Changes:
- Restore Windows creation-time updates by opening files relative to an
os.Rootparent handle and callingwindows.SetFileTimefor creation/access/modify. - Switch internal timestamp setting to use a
chtimes(root, name, atime, mtime)helper consistently (instead of callingroot.Chtimesdirectly in a few places). - Add a Windows regression test asserting that
chtimesupdates creation time.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| time_windows.go | Reintroduces Windows creation-time updates using a root-relative open + SetFileTime. |
| time_nonwindows.go | Updates chtimes to be os.Root-relative and reorders timeToTimespec helper. |
| archive.go | Routes timestamp application through chtimes(...) to preserve Windows semantics. |
| diff.go | Uses chtimes(...) for post-extraction directory timestamp normalization. |
| changes_test.go | Refactors test helpers to open an os.Root and use root-relative chtimes. |
| archive_windows_test.go | Adds regression coverage for Windows creation-time behavior. |
Comments suppressed due to low confidence (2)
changes_test.go:320
- More path.Join usages in mutateSampleDir build filesystem paths with forward slashes. Switch these to filepath.Join for correct OS-specific separators.
err = os.RemoveAll(path.Join(rootPath, "file5"))
assert.NilError(t, err)
err = os.MkdirAll(path.Join(rootPath, "file5"), 0o666)
assert.NilError(t, err)
// Create new file
err = os.WriteFile(path.Join(rootPath, "filenew"), []byte("filenew\n"), 0o777)
assert.NilError(t, err)
// Create new dir
err = os.MkdirAll(path.Join(rootPath, "dirnew"), 0o766)
assert.NilError(t, err)
changes_test.go:337
- path.Join is also used for symlink and file paths later in mutateSampleDir; this can produce invalid Windows paths. Use filepath.Join consistently.
err = os.Symlink("targetnew", path.Join(rootPath, "symlinknew"))
assert.NilError(t, err)
// Change a symlink
err = os.RemoveAll(path.Join(rootPath, "symlink2"))
assert.NilError(t, err)
err = os.Symlink("target2change", path.Join(rootPath, "symlink2"))
assert.NilError(t, err)
// Replace dir with file
err = os.RemoveAll(path.Join(rootPath, "dir2"))
assert.NilError(t, err)
err = os.WriteFile(path.Join(rootPath, "dir2"), []byte("dir2\n"), 0o777)
assert.NilError(t, err)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
time_windows.go:13
- The doc comment for openForWriteAttributesAt is incomplete ("with permission to"), which makes the exported behavior unclear when reading the file.
// openForWriteAttributesAt opens name relative to parent with permission to
// If noFollow is true, it does not follow reparse points.
3429bb7 to
67a119a
Compare
| } | ||
|
|
||
| var handle windows.Handle | ||
| err = windows.NtCreateFile( |
There was a problem hiding this comment.
This reintroduces the TOCTOU though? Except this time we can only modify the timestamp.. which probably isn't too bad.
There was a problem hiding this comment.
Good call; we need to look at some of the logic (and perhaps there's more to define w.r.t. "correct handling"), basically, in createTarFile;
createTarFilecreates the file and writes its contents (if the tar header described a non-symlink.chtimesre-opens the path and applies timestamps.
We (currently) drop the file handle between 1 and 2, but perform chtimes following symlinks (also for the last component). I think that's incorrect
NOTE: 👉 We do NOT currently call
createTarFileconcurrently; we always process the Tar sequentially, so we do not expect TOCTOU cases in our own code.
- The TOCTOU is between (1) and (2)
- We created a non-symlink (1), so (2) should expect it to still be the same type
- If the final component is now a symlink, that's unexpected, and we should rejected it regardless if the symlink is within
rootor outside ofroot
So; chtimes should;
- Open the parent path with
os.Root(following symlinks, but withinroot- this can be improved if we preserve the parent file-handle);os.Rootmakes sure it's withinroot. - Open the file handle itself (WITHOUT following symlinks)
- Error out if last component is a symlink/reparse point, because we didn't expect it to be.
I created a follow-up (but we can include in this PR if we think that makes sense);
ctalledo
left a comment
There was a problem hiding this comment.
Nice catch on the creation-time regression from the os.Root migration in #45. The fix looks correct and well-scoped, the root-relative NtCreateFile + SetFileTime helper is clearly documented with upstream references, and CI is green across the full matrix. LGTM. One optional test-strengthening suggestion inline.
Add a regression test verifying that updating file timestamps also updates the file creation time on Windows. The current implementation preserves this behavior, which is relied on by archive extraction and should remain unchanged during future refactoring. Commit df55fdf replaced the use of our local chtimes implementation for os.Chtimes, which may be less advanced and not set the creation time. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Restore updating the creation time when applying timestamps on Windows. The previous refactoring to os.Root.Chtimes() only updated the access and modification times, causing a regression. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
…p ci] Bumps the go-modules group in /e2e-go with 2 updates: [github.com/moby/go-archive](https://github.com/moby/go-archive) and [github.com/shirou/gopsutil/v4](https://github.com/shirou/gopsutil). Updates `github.com/moby/go-archive` from 0.2.1 to 0.3.2 Release notes *Sourced from [github.com/moby/go-archive's releases](https://github.com/moby/go-archive/releases).* > v0.3.2 > ------ > > What's Changed > -------------- > > Fix a regression introduced in v0.3.0 that caused archive extraction to fail when paths traversed absolute symlinks inside the destination root, such as `var/run -> /run`. Absolute symlink targets are now resolved relative to the extraction root while relative symlink escapes remain rejected. [moby/go-archive#93](https://redirect.github.com/moby/go-archive/pull/93) > > **Full Changelog**: <moby/go-archive@v0.3.1...v0.3.2> > > v0.3.1 > ------ > > Fixes > ----- > > This patch release fixes a regression introduced in v0.2.1 where archive extraction could fail when an archive omitted explicit entries for parent directories. For example, extracting `etc/dnf/` without a preceding `etc/` entry could return `mkdirat etc/dnf: no such file or directory`. > > This prevented affected images from being extracted. Archive extraction now creates implied parent directories for both file and directory entries. > > What's Changed > -------------- > > * archive: create implied parents for directory entries [moby/go-archive#92](https://redirect.github.com/moby/go-archive/pull/92) > * archive: Tarballer.Go: suppress io.ErrClosedPipe logs on close [moby/go-archive#94](https://redirect.github.com/moby/go-archive/pull/94) > > **Full Changelog**: <moby/go-archive@v0.3.0...v0.3.1> > > v0.3.0 > ------ > > Security > -------- > > This release fixes **CVE-2026-17106** / **[GHSA-hfg8-hc9c-6c3h](https://github.com/moby/go-archive/security/advisories/GHSA-hfg8-hc9c-6c3h)**, where a crafted tar archive could use links to cause extraction operations to create or overwrite files outside the intended destination directory. > > The issue affected `Unpack`, `UnpackLayer`, `Untar`, `UntarUncompressed`, and the `ApplyLayer` helpers. Users should upgrade and avoid extracting untrusted archives with earlier versions. > > What's Changed > -------------- > > * archive: harden tar extraction against path traversal [moby/go-archive#45](https://redirect.github.com/moby/go-archive/pull/45) > * archive: do not follow reparse points in chtimes [moby/go-archive#90](https://redirect.github.com/moby/go-archive/pull/90) > * archive: fix creation time updates on Windows [moby/go-archive#79](https://redirect.github.com/moby/go-archive/pull/79) > * archive: minor cleanups and godoc touch-up [moby/go-archive#87](https://redirect.github.com/moby/go-archive/pull/87) > * archive: RebaseArchiveEntries: fix archive path rebasing [moby/go-archive#43](https://redirect.github.com/moby/go-archive/pull/43) > > Test and CI changes > ------------------- > > * ci: enable dependabot for actions [moby/go-archive#81](https://redirect.github.com/moby/go-archive/pull/81) > * archive: make breakoutErr unwrap its cause [moby/go-archive#91](https://redirect.github.com/moby/go-archive/pull/91) > * archive: use filepath for filesystem paths in tests [moby/go-archive#80](https://redirect.github.com/moby/go-archive/pull/80) > * archive: use filepath for filesystem paths in tests [moby/go-archive#80](https://redirect.github.com/moby/go-archive/pull/80) > > **Full Changelog**: <moby/go-archive@v0.2.1...v0.3.0> Commits * [`9e6d2c7`](moby/go-archive@9e6d2c7) Merge pull request [#93](https://redirect.github.com/moby/go-archive/issues/93) from thaJeztah/fix\_absolute\_symlinks * [`4f6cd58`](moby/go-archive@4f6cd58) archive: resolve hardlinks through absolute symlinks * [`e564ecc`](moby/go-archive@e564ecc) archive: resolve absolute symlinks within extraction root * [`5bb8a45`](moby/go-archive@5bb8a45) Merge pull request [#94](https://redirect.github.com/moby/go-archive/issues/94) from thaJeztah/denoise * [`1bec7ec`](moby/go-archive@1bec7ec) archive: Tarballer.Go: suppress io.ErrClosedPipe logs on close * [`279fa6d`](moby/go-archive@279fa6d) Merge pull request [#92](https://redirect.github.com/moby/go-archive/issues/92) from thaJeztah/fix\_implied\_directories * [`517985a`](moby/go-archive@517985a) archive: create implied parents for directory entries * [`1c23372`](moby/go-archive@1c23372) Merge pull request [#43](https://redirect.github.com/moby/go-archive/issues/43) from thaJeztah/fix\_rebase\_from\_root * [`8829a25`](moby/go-archive@8829a25) RebaseArchiveEntries: fix archive path rebasing * [`c583b20`](moby/go-archive@c583b20) Merge pull request [#90](https://redirect.github.com/moby/go-archive/issues/90) from thaJeztah/chtimes\_nofollow * Additional commits viewable in [compare view](moby/go-archive@v0.2.1...v0.3.2) Updates `github.com/shirou/gopsutil/v4` from 4.26.6 to 4.26.7 Release notes *Sourced from [github.com/shirou/gopsutil/v4's releases](https://github.com/shirou/gopsutil/releases).* > v4.26.7 > ------- > > What's Changed > -------------- > > ### cpu > > * fix: harden parsers against malformed/truncated input by [`@shirou`](https://github.com/shirou) in [shirou/gopsutil#2109](https://redirect.github.com/shirou/gopsutil/pull/2109) > * [cpu][windows]: compute cpu-total times from integer ticks by [`@skartikey`](https://github.com/skartikey) in [shirou/gopsutil#2111](https://redirect.github.com/shirou/gopsutil/pull/2111) > * [darwin][process]: fix errno handling and library lifetime on darwin by [`@shirou`](https://github.com/shirou) in [shirou/gopsutil#2119](https://redirect.github.com/shirou/gopsutil/pull/2119) > * [cpu][windows]: compute total counters from individual stats to handle processor groups correctly by [`@srebhan`](https://github.com/srebhan) in [shirou/gopsutil#2125](https://redirect.github.com/shirou/gopsutil/pull/2125) > * [cpu][windows]: harden the cpu-total computation added in [#2125](https://redirect.github.com/shirou/gopsutil/issues/2125) by [`@shirou`](https://github.com/shirou) in [shirou/gopsutil#2128](https://redirect.github.com/shirou/gopsutil/pull/2128) > > ### net > > * fix(net): pad GetExtendedTcpTable buffer to prevent GC thrashing on Windows by [`@HarshalPatel1972`](https://github.com/HarshalPatel1972) in [shirou/gopsutil#2108](https://redirect.github.com/shirou/gopsutil/pull/2108) > > ### process > > * process: implement Darwin IOCounters via proc\_pid\_rusage by [`@DavRack`](https://github.com/DavRack) in [shirou/gopsutil#2117](https://redirect.github.com/shirou/gopsutil/pull/2117) > > ### other > > * feat: add psutil comparison tests for cpu, mem and load by [`@shirou`](https://github.com/shirou) in [shirou/gopsutil#2114](https://redirect.github.com/shirou/gopsutil/pull/2114) > > New Contributors > ---------------- > > * [`@DavRack`](https://github.com/DavRack) made their first contribution in [shirou/gopsutil#2117](https://redirect.github.com/shirou/gopsutil/pull/2117) > * [`@srebhan`](https://github.com/srebhan) made their first contribution in [shirou/gopsutil#2125](https://redirect.github.com/shirou/gopsutil/pull/2125) > > **Full Changelog**: <shirou/gopsutil@v4.26.6...v4.26.7> Commits * [`52a24c8`](shirou/gopsutil@52a24c8) Merge pull request [#2128](https://redirect.github.com/shirou/gopsutil/issues/2128) from shirou/feat/follow-up-2125 * [`268a953`](shirou/gopsutil@268a953) [cpu][windows]: harden the cpu-total computation added in [#2125](https://redirect.github.com/shirou/gopsutil/issues/2125) * [`1e34da6`](shirou/gopsutil@1e34da6) Merge pull request [#2125](https://redirect.github.com/shirou/gopsutil/issues/2125) from srebhan/fix\_cpu\_windows\_total * [`61f8802`](shirou/gopsutil@61f8802) Merge pull request [#2122](https://redirect.github.com/shirou/gopsutil/issues/2122) from shirou/dependabot/github\_actions/actions/checko... * [`7fb4dcf`](shirou/gopsutil@7fb4dcf) Merge pull request [#2123](https://redirect.github.com/shirou/gopsutil/issues/2123) from shirou/dependabot/github\_actions/actions/setup-... * [`ae7d91a`](shirou/gopsutil@ae7d91a) Merge pull request [#2119](https://redirect.github.com/shirou/gopsutil/issues/2119) from shirou/fix/darwin-errno-and-libcache * [`49052a1`](shirou/gopsutil@49052a1) [darwin][process]: use a PID above PID\_MAX in the not-running tests * [`991b238`](shirou/gopsutil@991b238) [darwin]: pass the remaining Go pointers as unsafe.Pointer on darwin * [`b9930e2`](shirou/gopsutil@b9930e2) Merge pull request [#2124](https://redirect.github.com/shirou/gopsutil/issues/2124) from shirou/dependabot/github\_actions/actions/labele... * [`38a01b4`](shirou/gopsutil@38a01b4) [cpu][windows]: compute total counters from individual stats to handle proces... * Additional commits viewable in [compare view](shirou/gopsutil@v4.26.6...v4.26.7) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore major version` will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself) - `@dependabot ignore minor version` will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself) - `@dependabot ignore ` will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself) - `@dependabot unignore ` will remove all of the ignore conditions of the specified dependency - `@dependabot unignore ` will remove the ignore condition of the specified dependency and ignore conditions
archive: add Windows creation time test
Add a regression test verifying that updating file timestamps also
updates the file creation time on Windows.
The current implementation preserves this behavior, which is relied on
by archive extraction and should remain unchanged during future
refactoring.
Commit df55fdf replaced the use of
our local chtimes implementation for os.Chtimes, which may be less
advanced and not set the creation time.
archive: fix creation time updates on Windows
Restore updating the creation time when applying timestamps on
Windows. The previous refactoring to os.Root.Chtimes() only updated
the access and modification times, causing a regression.