drivers.ChownPathByMaps(): preserve directory ModTimes

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

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 {

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
 }

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{}

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)

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