From 90275585ec6e377add29edcc06ce8c27294c150e Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Wed, 27 May 2026 10:36:46 -0400 Subject: [PATCH] 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 | 29 ++++++++- storage/drivers/chown_darwin.go | 9 ++- storage/drivers/chown_unix.go | 12 ++-- storage/drivers/chown_windows.go | 3 +- storage/tests/chown.bats | 106 +++++++++++++++++++++++++++++++ storage/tests/helpers.bash | 2 +- 6 files changed, 151 insertions(+), 10 deletions(-) create mode 100644 storage/tests/chown.bats diff --git a/storage/drivers/chown.go b/storage/drivers/chown.go index 808e0022ff..6f1be6a335 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,43 @@ func chownByMapsMain() { } chowner := newLChowner() + var dirModTimes sync.Map 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 + } + tsCurrent := syscall.NsecToTimespec(st.ModTime().UnixNano()) + tsSaved := syscall.NsecToTimespec(value.(int64)) + if tsCurrent != tsSaved { + if err := system.LUtimesNano(dir, []syscall.Timespec{tsSaved, tsSaved}); 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..02bc76d3f1 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 { @@ -102,7 +103,9 @@ func (c *platformChowner) LChown(path string, info os.FileInfo, toHost, toContai return fmt.Errorf("%s: %w", os.Args[0], err) } } - + if info.IsDir() { + c.modifiedDirectories.Store(path, struct{}{}) + } } return nil } diff --git a/storage/drivers/chown_unix.go b/storage/drivers/chown_unix.go index e4567ecec9..6e7fbf6427 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,11 +62,11 @@ 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) } @@ -120,7 +122,9 @@ func (c *platformChowner) LChown(path string, info os.FileInfo, toHost, toContai return fmt.Errorf("%s: %w", os.Args[0], err) } } - + if info.IsDir() { + c.modifiedDirectories.Store(path, struct{}{}) + } } 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..61fd1544a9 --- /dev/null +++ b/storage/tests/chown.bats @@ -0,0 +1,106 @@ +#!/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 + echo "$output" + [ "$status" -eq 0 ] + [ "$output" != "" ] + templayer="$output" + + # 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 + echo "$output" + [ "$status" -eq 0 ] + [ "$output" != "" ] + templayer="$output" + + # 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 --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 --file $TESTDIR/layer2.tar --uidmap 0:2:1024 --gidmap 0:2:1024 lower + echo "$output" + [ "$status" -eq 0 ] + [ "$output" != "" ] + + # And another one with a different ID map. + run storage --debug=false import-layer --name upper --file $TESTDIR/layer2.tar --uidmap 0:3:1024 --gidmap 0:3:1024 middle + 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 + 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" != "" ] + + # Check for inconsistencies. + run storage --debug=false check + echo "$output" + [ "$status" -eq 0 ] + [ "$output" == "" ] +} diff --git a/storage/tests/helpers.bash b/storage/tests/helpers.bash index 1d0646716f..208a90c405 100755 --- a/storage/tests/helpers.bash +++ b/storage/tests/helpers.bash @@ -22,7 +22,7 @@ function setup() { rm -fr ${TESTDIR} mkdir -p ${TESTDIR}/{root,runroot} # disable idmapped mounts in the overlay driver, since that - # is the expectation in the idmaps.bats tests. + # is the expectation in the chown.bats and idmaps.bats tests. export _CONTAINERS_OVERLAY_DISABLE_IDMAP=yes }