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,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

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
 }

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
 }

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

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
 }