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/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) 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" == "" ] +}