From 8a65b697ab206fc9a755b8be6917fe30c9c0316c Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Wed, 27 May 2026 10:36:46 -0400 Subject: [PATCH 1/2] drivers.ChownPathByMaps(): preserve directory ModTimes When changing the ownership throughout a directory tree, make notes of the initial values of the ModTime for directories that we process. When removing and re-creating hard links to pick up an ownership change, or changing the owner of an directory, note the name of the directory containing the hard link, or of the directory itself. After changing the ownership throughout a directory tree, walk the list of directories whose timestamps we expect to have modified, and if their ModTime values were changed, restore them. Signed-off-by: Nalin Dahyabhai --- storage/drivers/chown.go | 31 ++++++++- storage/drivers/chown_darwin.go | 5 +- storage/drivers/chown_unix.go | 11 +-- storage/drivers/chown_windows.go | 3 +- storage/tests/chown.bats | 112 +++++++++++++++++++++++++++++++ 5 files changed, 154 insertions(+), 8 deletions(-) create mode 100644 storage/tests/chown.bats diff --git a/storage/drivers/chown.go b/storage/drivers/chown.go index 808e0022ff..b566898939 100644 --- a/storage/drivers/chown.go +++ b/storage/drivers/chown.go @@ -6,10 +6,13 @@ import ( "fmt" "io/fs" "os" + "sync" + "syscall" "github.com/opencontainers/selinux/pkg/pwalkdir" "go.podman.io/storage/pkg/idtools" "go.podman.io/storage/pkg/reexec" + "go.podman.io/storage/pkg/system" ) const ( @@ -54,19 +57,45 @@ func chownByMapsMain() { } chowner := newLChowner() + var dirModTimes sync.Map // [path string](mtime int64) var chown fs.WalkDirFunc = func(path string, d fs.DirEntry, _ error) error { info, err := d.Info() if path == "." || err != nil { return nil } + if info.IsDir() { + dirModTimes.Store(path, info.ModTime().UnixNano()) + } return chowner.LChown(path, info, toHost, toContainer) } if err := pwalkdir.Walk(".", chown); err != nil { fmt.Fprintf(os.Stderr, "error during chown: %v", err) os.Exit(1) } - os.Exit(0) + exitStatus := 0 + chowner.modifiedDirectories.Range(func(key, _ any) bool { + dir := key.(string) + if value, ok := dirModTimes.Load(dir); ok { + st, err := os.Lstat(dir) + if err != nil { + fmt.Fprintf(os.Stderr, "error during chown: %v", err) + exitStatus = 1 + return true + } + tsCurrent := st.ModTime().UnixNano() + tsSaved := value.(int64) + if tsCurrent != tsSaved { + ts := syscall.NsecToTimespec(tsSaved) + if err := system.LUtimesNano(dir, []syscall.Timespec{ts, ts}); err != nil { + fmt.Fprintf(os.Stderr, "error during chown: %v", err) + exitStatus = 1 + } + } + } + return true + }) + os.Exit(exitStatus) } // ChownPathByMaps walks the filesystem tree, changing the ownership diff --git a/storage/drivers/chown_darwin.go b/storage/drivers/chown_darwin.go index eea633520c..a065b9b331 100644 --- a/storage/drivers/chown_darwin.go +++ b/storage/drivers/chown_darwin.go @@ -19,8 +19,9 @@ type inode struct { } type platformChowner struct { - mutex sync.Mutex - inodes map[inode]bool + mutex sync.Mutex + inodes map[inode]bool + modifiedDirectories sync.Map } func newLChowner() *platformChowner { diff --git a/storage/drivers/chown_unix.go b/storage/drivers/chown_unix.go index e4567ecec9..154381b435 100644 --- a/storage/drivers/chown_unix.go +++ b/storage/drivers/chown_unix.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "sync" "syscall" @@ -19,8 +20,9 @@ type inode struct { } type platformChowner struct { - mutex sync.Mutex - inodes map[inode]string + mutex sync.Mutex + inodes map[inode]string + modifiedDirectories sync.Map } func newLChowner() *platformChowner { @@ -60,12 +62,14 @@ func (c *platformChowner) LChown(path string, info os.FileInfo, toHost, toContai // of chowning it again. This is necessary when the underlying file system breaks // inodes on copy-up (as it is with overlay with index=off) to maintain the original // link and correct file ownership. - // The target already exists so remove it before creating the link to the new target. if err := os.Remove(path); err != nil { return err } + c.modifiedDirectories.Store(filepath.Dir(path), struct{}{}) return os.Link(oldTarget, path) + } else if info.IsDir() { + c.modifiedDirectories.Store(path, struct{}{}) } // Map an on-disk UID/GID pair from host to container @@ -120,7 +124,6 @@ func (c *platformChowner) LChown(path string, info os.FileInfo, toHost, toContai return fmt.Errorf("%s: %w", os.Args[0], err) } } - } return nil } diff --git a/storage/drivers/chown_windows.go b/storage/drivers/chown_windows.go index 8f45ad82e0..19b4c8c61f 100644 --- a/storage/drivers/chown_windows.go +++ b/storage/drivers/chown_windows.go @@ -4,12 +4,13 @@ package graphdriver import ( "os" + "sync" "syscall" "go.podman.io/storage/pkg/idtools" ) -type platformChowner struct{} +type platformChowner struct{ modifiedDirectories sync.Map } func newLChowner() *platformChowner { return &platformChowner{} diff --git a/storage/tests/chown.bats b/storage/tests/chown.bats new file mode 100644 index 0000000000..ae048a6744 --- /dev/null +++ b/storage/tests/chown.bats @@ -0,0 +1,112 @@ +#!/usr/bin/env bats + +load helpers + +@test "chown-preserves-modtimes" { + # This test needs "tar". + if test -z "$(which tar 2> /dev/null)" ; then + skip "need tar" + fi + if test -z "$(which dd 2> /dev/null)" ; then + skip "need dd" + fi + if [[ "${STORAGE_OPTION}" =~ "fuse-overlayfs" ]] ; then + # this test would be tripped up by https://github.com/containers/fuse-overlayfs/issues/400 + skip "not testing with fuse-overlayfs" + fi + + # Create a tree for a layer with at least one hard link and some directories. + pushd $TESTDIR > /dev/null + mkdir layer layer/layer1 layer/layer1/directory layer/layer1/directory/subdirectory + createrandom layer/layer1/directory/subdirectory/linktarget + ln layer/layer1/directory/subdirectory/linktarget layer/layer1/directory/subdirectory/link + ln -s linktarget layer/layer1/directory/subdirectory/symlink + createrandom layer/layer1/directory/subdirectory/otherfile + touch -d 1970-01-01T00:00:00Z layer/layer1 layer/layer1/directory layer/layer1/directory/subdirectory + # Create another tree with just the directories. + mkdir layer/layer2 layer/layer2/directory layer/layer2/directory/subdirectory + touch -d 1970-01-01T00:00:00Z layer/layer2 layer/layer2/directory layer/layer2/directory/subdirectory + popd > /dev/null + + # Create a temporary layer. + run storage --debug=false create-layer --name temporary-layer-1 + echo "$output" + [ "$status" -eq 0 ] + [ "$output" != "" ] + templayer=temporary-layer-1 + + # Copy the content into it. + run storage --debug=false copy --chown 0:0 $TESTDIR/layer/layer1 "$templayer":/ + echo "$output" + [ "$status" -eq 0 ] + [ "$output" == "" ] + + # Generate a diff with the contents. + run storage --debug=false diff -f $TESTDIR/layer1.tar "$templayer" + echo "$output" + [ "$status" -eq 0 ] + [ "$output" == "" ] + + # Create another temporary layer. + run storage --debug=false create-layer --name temporary-layer-2 + echo "$output" + [ "$status" -eq 0 ] + [ "$output" != "" ] + templayer=temporary-layer-2 + + # Copy the content into it. + run storage --debug=false copy --chown 0:0 $TESTDIR/layer/layer2 "$templayer":/ + echo "$output" + [ "$status" -eq 0 ] + [ "$output" == "" ] + + # Generate a diff with the contents. + run storage --debug=false diff -f $TESTDIR/layer2.tar "$templayer" + echo "$output" + [ "$status" -eq 0 ] + [ "$output" == "" ] + + # Create a new layer using first diff to populate it. + run storage --debug=false import-layer --name lower-layer --file $TESTDIR/layer1.tar --uidmap 0:2:1024 --gidmap 0:2:1024 + echo "$output" + [ "$status" -eq 0 ] + [ "$output" != "" ] + + # Create a new layer based on the one we just created, overwriting some + # of its directories that will also contain items that are chown'd when + # being pulled up after the directories are extracted onto disk. + run storage --debug=false import-layer --name middle-layer --file $TESTDIR/layer2.tar --uidmap 0:2:1024 --gidmap 0:2:1024 lower-layer + echo "$output" + [ "$status" -eq 0 ] + [ "$output" != "" ] + + # And another one with a different ID map. + run storage --debug=false import-layer --name upper-layer --file $TESTDIR/layer2.tar --uidmap 0:3:1024 --gidmap 0:3:1024 middle-layer + echo "$output" + [ "$status" -eq 0 ] + [ "$output" != "" ] + + # Create an image using that layer as its top layer. + run storage --debug=false create-image --name image upper-layer + echo "$output" + [ "$status" -eq 0 ] + [ "$output" != "" ] + + # Create a container using that image. + run storage --debug=false create-container --name container --hostuidmap --hostgidmap image + echo "$output" + [ "$status" -eq 0 ] + [ "$output" != "" ] + + # In case something goes wrong, for the log. + run storage --debug=false layers -t + echo "$output" + [ "$status" -eq 0 ] + [ "$output" != "" ] + + # Check for inconsistencies. + run storage --debug=false check + echo "$output" + [ "$status" -eq 0 ] + [ "$output" == "" ] +} From ba14a36d66795c0ea6e5467ed83c1c24fcb7c612 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Tue, 30 Jun 2026 12:30:29 -0400 Subject: [PATCH 2/2] Fix a typo in a comment Signed-off-by: Nalin Dahyabhai --- storage/pkg/chrootarchive/archive_unix_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/pkg/chrootarchive/archive_unix_test.go b/storage/pkg/chrootarchive/archive_unix_test.go index cdb90ba75f..a040af41e7 100644 --- a/storage/pkg/chrootarchive/archive_unix_test.go +++ b/storage/pkg/chrootarchive/archive_unix_test.go @@ -58,7 +58,7 @@ func TestUntarWithMaliciousSymlinks(t *testing.T) { err = UntarWithRoot(tee, safe, nil, root) assert.ErrorContains(t, err, "open /safe/host-file: no such file or directory") - // Make sure the "host" file is still in tact + // Make sure the "host" file is still intact // Before the fix the host file would be overwritten hostData, err := os.ReadFile(filepath.Join(dir, "host-file")) require.NoError(t, err)