Skip to content

Commit ce8d36f

Browse files
nalindnalind
committed
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 <nalin@redhat.com>
1 parent 6638a3c commit ce8d36f

6 files changed

Lines changed: 157 additions & 10 deletions

File tree

‎storage/drivers/chown.go‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,13 @@ import (
66
"fmt"
77
"io/fs"
88
"os"
9+
"sync"
10+
"syscall"
911

1012
"github.com/opencontainers/selinux/pkg/pwalkdir"
1113
"go.podman.io/storage/pkg/idtools"
1214
"go.podman.io/storage/pkg/reexec"
15+
"go.podman.io/storage/pkg/system"
1316
)
1417

1518
const (
@@ -54,19 +57,43 @@ func chownByMapsMain() {
5457
}
5558

5659
chowner := newLChowner()
60+
var dirModTimes sync.Map
5761

5862
var chown fs.WalkDirFunc = func(path string, d fs.DirEntry, _ error) error {
5963
info, err := d.Info()
6064
if path == "." || err != nil {
6165
return nil
6266
}
67+
if info.IsDir() {
68+
dirModTimes.Store(path, info.ModTime().UnixNano())
69+
}
6370
return chowner.LChown(path, info, toHost, toContainer)
6471
}
6572
if err := pwalkdir.Walk(".", chown); err != nil {
6673
fmt.Fprintf(os.Stderr, "error during chown: %v", err)
6774
os.Exit(1)
6875
}
69-
os.Exit(0)
76+
exitStatus := 0
77+
chowner.modifiedDirectories.Range(func(key, _ any) bool {
78+
dir := key.(string)
79+
if value, ok := dirModTimes.Load(dir); ok {
80+
st, err := os.Lstat(dir)
81+
if err != nil {
82+
fmt.Fprintf(os.Stderr, "error during chown: %v", err)
83+
exitStatus = 1
84+
}
85+
tsCurrent := syscall.NsecToTimespec(st.ModTime().UnixNano())
86+
tsSaved := syscall.NsecToTimespec(value.(int64))
87+
if tsCurrent != tsSaved {
88+
if err := system.LUtimesNano(dir, []syscall.Timespec{tsSaved, tsSaved}); err != nil {
89+
fmt.Fprintf(os.Stderr, "error during chown: %v", err)
90+
exitStatus = 1
91+
}
92+
}
93+
}
94+
return true
95+
})
96+
os.Exit(exitStatus)
7097
}
7198

7299
// ChownPathByMaps walks the filesystem tree, changing the ownership

‎storage/drivers/chown_darwin.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,9 @@ type inode struct {
1919
}
2020

2121
type platformChowner struct {
22-
mutex sync.Mutex
23-
inodes map[inode]bool
22+
mutex sync.Mutex
23+
inodes map[inode]bool
24+
modifiedDirectories sync.Map
2425
}
2526

2627
func newLChowner() *platformChowner {
@@ -102,7 +103,9 @@ func (c *platformChowner) LChown(path string, info os.FileInfo, toHost, toContai
102103
return fmt.Errorf("%s: %w", os.Args[0], err)
103104
}
104105
}
105-
106+
if info.IsDir() {
107+
c.modifiedDirectories.Store(path, struct{}{})
108+
}
106109
}
107110
return nil
108111
}

‎storage/drivers/chown_unix.go‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"errors"
77
"fmt"
88
"os"
9+
"path/filepath"
910
"sync"
1011
"syscall"
1112

@@ -19,8 +20,9 @@ type inode struct {
1920
}
2021

2122
type platformChowner struct {
22-
mutex sync.Mutex
23-
inodes map[inode]string
23+
mutex sync.Mutex
24+
inodes map[inode]string
25+
modifiedDirectories sync.Map
2426
}
2527

2628
func newLChowner() *platformChowner {
@@ -60,11 +62,11 @@ func (c *platformChowner) LChown(path string, info os.FileInfo, toHost, toContai
6062
// of chowning it again. This is necessary when the underlying file system breaks
6163
// inodes on copy-up (as it is with overlay with index=off) to maintain the original
6264
// link and correct file ownership.
63-
6465
// The target already exists so remove it before creating the link to the new target.
6566
if err := os.Remove(path); err != nil {
6667
return err
6768
}
69+
c.modifiedDirectories.Store(filepath.Dir(path), struct{}{})
6870
return os.Link(oldTarget, path)
6971
}
7072

@@ -120,7 +122,9 @@ func (c *platformChowner) LChown(path string, info os.FileInfo, toHost, toContai
120122
return fmt.Errorf("%s: %w", os.Args[0], err)
121123
}
122124
}
123-
125+
if info.IsDir() {
126+
c.modifiedDirectories.Store(path, struct{}{})
127+
}
124128
}
125129
return nil
126130
}

‎storage/drivers/chown_windows.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,13 @@ package graphdriver
44

55
import (
66
"os"
7+
"sync"
78
"syscall"
89

910
"go.podman.io/storage/pkg/idtools"
1011
)
1112

12-
type platformChowner struct{}
13+
type platformChowner struct{ modifiedDirectories sync.Map }
1314

1415
func newLChowner() *platformChowner {
1516
return &platformChowner{}

‎storage/tests/chown.bats‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
#!/usr/bin/env bats
2+
3+
load helpers
4+
5+
@test "chown-preserves-modtimes" {
6+
# This test needs "tar".
7+
if test -z "$(which tar 2> /dev/null)" ; then
8+
skip "need tar"
9+
fi
10+
if test -z "$(which dd 2> /dev/null)" ; then
11+
skip "need dd"
12+
fi
13+
if [[ "${STORAGE_OPTION}" =~ "fuse-overlayfs" ]] ; then
14+
# this test would be tripped up by https://github.com/containers/fuse-overlayfs/issues/400
15+
skip "not testing with fuse-overlayfs"
16+
fi
17+
18+
# Create a tree for a layer with at least one hard link and some directories.
19+
pushd $TESTDIR > /dev/null
20+
mkdir layer layer/layer1 layer/layer1/directory layer/layer1/directory/subdirectory
21+
createrandom layer/layer1/directory/subdirectory/linktarget
22+
ln layer/layer1/directory/subdirectory/linktarget layer/layer1/directory/subdirectory/link
23+
ln -s linktarget layer/layer1/directory/subdirectory/symlink
24+
createrandom layer/layer1/directory/subdirectory/otherfile
25+
touch -d 1970-01-01T00:00:00Z layer/layer1 layer/layer1/directory layer/layer1/directory/subdirectory
26+
# Create another tree with just the directories.
27+
mkdir layer/layer2 layer/layer2/directory layer/layer2/directory/subdirectory
28+
touch -d 1970-01-01T00:00:00Z layer/layer2 layer/layer2/directory layer/layer2/directory/subdirectory
29+
popd > /dev/null
30+
31+
# Create a temporary layer.
32+
run storage --debug=false create-layer --name temporary-layer-1
33+
echo "$output"
34+
[ "$status" -eq 0 ]
35+
[ "$output" != "" ]
36+
templayer=temporary-layer-1
37+
38+
# Copy the content into it.
39+
run storage --debug=false copy --chown 0:0 $TESTDIR/layer/layer1 "$templayer":/
40+
echo "$output"
41+
[ "$status" -eq 0 ]
42+
[ "$output" == "" ]
43+
44+
# Generate a diff with the contents.
45+
run storage --debug=false diff -f $TESTDIR/layer1.tar "$templayer"
46+
echo "$output"
47+
[ "$status" -eq 0 ]
48+
[ "$output" == "" ]
49+
50+
# Create another temporary layer.
51+
run storage --debug=false create-layer --name temporary-layer-2
52+
echo "$output"
53+
[ "$status" -eq 0 ]
54+
[ "$output" != "" ]
55+
templayer=temporary-layer-2
56+
57+
# Copy the content into it.
58+
run storage --debug=false copy --chown 0:0 $TESTDIR/layer/layer2 "$templayer":/
59+
echo "$output"
60+
[ "$status" -eq 0 ]
61+
[ "$output" == "" ]
62+
63+
# Generate a diff with the contents.
64+
run storage --debug=false diff -f $TESTDIR/layer2.tar "$templayer"
65+
echo "$output"
66+
[ "$status" -eq 0 ]
67+
[ "$output" == "" ]
68+
69+
# Create a new layer using first diff to populate it.
70+
run storage --debug=false import-layer --name lower-layer --file $TESTDIR/layer1.tar --uidmap 0:2:1024 --gidmap 0:2:1024
71+
echo "$output"
72+
[ "$status" -eq 0 ]
73+
[ "$output" != "" ]
74+
75+
# Create a new layer based on the one we just created, overwriting some
76+
# of its directories that will also contain items that are chown'd when
77+
# being pulled up after the directories are extracted onto disk.
78+
run storage --debug=false import-layer --name middle-layer --file $TESTDIR/layer2.tar --uidmap 0:2:1024 --gidmap 0:2:1024 lower-layer
79+
echo "$output"
80+
[ "$status" -eq 0 ]
81+
[ "$output" != "" ]
82+
83+
# And another one with a different ID map.
84+
run storage --debug=false import-layer --name upper-layer --file $TESTDIR/layer2.tar --uidmap 0:3:1024 --gidmap 0:3:1024 middle-layer
85+
echo "$output"
86+
[ "$status" -eq 0 ]
87+
[ "$output" != "" ]
88+
89+
# Create an image using that layer as its top layer.
90+
run storage --debug=false create-image --name image upper-layer
91+
echo "$output"
92+
[ "$status" -eq 0 ]
93+
[ "$output" != "" ]
94+
95+
# Create a container using that image.
96+
run storage --debug=false create-container --name container --hostuidmap --hostgidmap image
97+
echo "$output"
98+
[ "$status" -eq 0 ]
99+
[ "$output" != "" ]
100+
101+
# In case something goes wrong, for the log.
102+
run storage --debug=false layers -t
103+
echo "$output"
104+
[ "$status" -eq 0 ]
105+
[ "$output" != "" ]
106+
107+
# Check for inconsistencies.
108+
run storage --debug=false check
109+
echo "$output"
110+
[ "$status" -eq 0 ]
111+
[ "$output" == "" ]
112+
}

‎storage/tests/helpers.bash‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ function setup() {
2222
rm -fr ${TESTDIR}
2323
mkdir -p ${TESTDIR}/{root,runroot}
2424
# disable idmapped mounts in the overlay driver, since that
25-
# is the expectation in the idmaps.bats tests.
25+
# is the expectation in the chown.bats and idmaps.bats tests.
2626
export _CONTAINERS_OVERLAY_DISABLE_IDMAP=yes
2727
}
2828

0 commit comments

Comments
 (0)