8000 contenthash: do not follow nested symlinks when computing checksum · moby/buildkit@b69f184 · GitHub
[go: up one dir, main page]
Skip to content

Commit b69f184

Browse files
jsternbergjsternberg
committed
contenthash: do not follow nested symlinks when computing checksum
Nested symlinks were not intended to be followed after the first level of wildcard expansion. When include/exclude patterns were used or a wildcard was present higher up in the tree, we would erroneously follow ALL symlinks to compute the hash. When a symlink was broken, this would result in an error. We weren't supposed to be following the symlinks at all. This fixes things by changing `includedPaths` to also return whether a path should follow links. If a parent directory exists for the path, then the symlink is not followed. Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
1 parent 0ca6aba commit b69f184

File tree

3 files changed

+60
-10
lines changed

3 files changed

+60
-10
lines changed

‎cache/contenthash/checksum.go‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ type includedPath struct {
9292
included bool
9393
includeMatchInfo patternmatcher.MatchInfo
9494
excludeMatchInfo patternmatcher.MatchInfo
95+
followLinks bool
9596
}
9697

9798
type cacheManager struct {
@@ -431,17 +432,16 @@ func (cc *cacheContext) Checksum(ctx context.Context, mountable cache.Mountable,
431432
return "", err
432433
}
433434

434-
if opts.FollowLinks {
435-
for i, w := range includedPaths {
436-
if w.record.Type == CacheRecordTypeSymlink {
437-
dgst, err := cc.lazyChecksum(ctx, m, w.path, opts.FollowLinks)
438-
if err != nil {
439-
return "", err
440-
}
441-
includedPaths[i].record = &CacheRecord{Digest: string(dgst)}
435+
for i, w := range includedPaths {
436+
if w.followLinks && w.record.Type == CacheRecordTypeSymlink {
437+
dgst, err := cc.lazyChecksum(ctx, m, w.path, opts.FollowLinks)
438+
if err != nil {
439+
return "", err
442440
}
441+
includedPaths[i].record = &CacheRecord{Digest: string(dgst)}
443442
}
444443
}
444+
445445
if len(includedPaths) == 0 {
446446
return digest.FromBytes([]byte{}), nil
447447
}
@@ -597,6 +597,10 @@ func (cc *cacheContext) includedPaths(ctx context.Context, m *mount, p string, o
597597
}
598598

599599
maybeIncludedPath := &includedPath{path: fn}
600+
if parentDir == nil && opts.FollowLinks {
601+
maybeIncludedPath.followLinks = true
602+
}
603+
600604
var shouldInclude bool
601605
if opts.Wildcard {
602606
if p != "" && (lastMatchedDir == "" || !strings.HasPrefix(fn, lastMatchedDir+"/")) {

‎cache/contenthash/checksum_test.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -527,6 +527,25 @@ func TestSymlinksNoFollow(t *testing.T) {
527527
require.NoError(t, err)
528528
require.Equal(t, expectedSym, dgst)
529529

530+
expectedSym = digest.Digest("sha256:9b761577efcb1239cf4be971914c5d7404914dd32ff436401af1764dc5446b83")
531+
532+
// Broken symlink is not followed in subdirectory.
533+
dgst, err = cc.Checksum(context.TODO(), ref, "foo", ChecksumOpts{FollowLinks: true}, nil)
534+
require.NoError(t, err)
535+
require.Equal(t, expectedSym, dgst)
536+
537+
expectedSym = digest.Digest("sha256:e14a98332f46b81993ff4a3b7898bb00fc3d245d84e4c42e57c9ee6b129c7c41")
538+
539+
// Same with wildcard used.
540+
dgst, err = cc.Checksum(context.TODO(), ref, "fo?", ChecksumOpts{FollowLinks: true, Wildcard: true}, nil)
541+
require.NoError(t, err)
542+
require.Equal(t, expectedSym, dgst)
543+
544+
// Still works with exclude pattern.
545+
dgst, err = cc.Checksum(context.TODO(), ref, "foo", ChecksumOpts{FollowLinks: true, ExcludePatterns: []string{"*.git"}}, nil)
546+
require.NoError(t, err)
547+
require.Equal(t, expectedSym, dgst)
548+
530549
err = ref.Release(context.TODO())
531550
require.NoError(t, err)
532551
}

‎frontend/dockerfile/exclude_patterns_test.go‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ func testExcludedFilesOnCopy(t *testing.T, sb integration.Sandbox) {
4141
fstest.CreateDir("dir1", fs.ModePerm),
4242
fstest.CreateDir("dir2", fs.ModePerm),
4343
fstest.CreateDir("dir3", fs.ModePerm),
44+
fstest.CreateDir("dir4", fs.ModePerm),
4445
fstest.CreateFile("dir1/file-101.png", []byte(`2`), 0600),
4546
fstest.CreateFile("dir1/file-102.txt", []byte(`3`), 0600),
4647
fstest.CreateFile("dir1/file-103.jpeg", []byte(`4`), 0600),
@@ -49,6 +50,9 @@ func testExcludedFilesOnCopy(t *testing.T, sb integration.Sandbox) {
4950
fstest.CreateFile("dir2/file-203.png", []byte(`8`), 0600),
5051
fstest.CreateFile("dir3/file-301.mp3", []byte(`9`), 0600),
5152
fstest.CreateFile("dir3/file-302.mpeg", []byte(`10`), 0600),
53+
fstest.CreateFile("dir4/file-401.txt", []byte(`11`), 0600),
54+
fstest.Symlink("dir4/file-402.txt", "dir4/file-401.txt"),
55+
fstest.Symlink("dir4/file-403.txt", "dir4/file-404.txt"),
5256
)
5357

5458
runTest := func(dockerfile []byte, localMounts map[string]fsutil.FS) {
@@ -93,6 +97,9 @@ func testExcludedFilesOnCopy(t *testing.T, sb integration.Sandbox) {
9397
{filename: "builder1/1/dir2/file-203.png", excluded: true},
9498
{filename: "builder1/1/dir3/file-301.mp3", excluded: false, expectedContent: `9`},
9599
{filename: "builder1/1/dir3/file-302.mpeg", excluded: false, expectedContent: `10`},
100+
{filename: "builder1/1/dir4/file-401.txt", excluded: false, expectedContent: `11`},
101+
{filename: "builder1/1/dir4/file-402.txt", excluded: false, expectedContent: `file-401.txt`},
102+
{filename: "builder1/1/dir4/file-403.txt", excluded: false, expectedContent: `file-404.txt`},
96103

97104
// files copied with COPY command
98105
{filename: "builder2/2/dir1/file-101.png", excluded: false, expectedContent: `2`},
@@ -103,6 +110,9 @@ func testExcludedFilesOnCopy(t *testing.T, sb integration.Sandbox) {
103110
{filename: "builder2/2/dir2/file-203.png", excluded: false, expectedContent: `8`},
104111
{filename: "builder2/2/dir3/file-301.mp3", excluded: false, expectedContent: `9`},
105112
{filename: "builder2/2/dir3/file-302.mpeg", excluded: false, expectedContent: `10`},
113+
{filename: "builder2/2/dir4/file-401.txt", excluded: true},
114+
{filename: "builder2/2/dir4/file-402.txt", excluded: true},
115+
{filename: "builder2/2/dir4/file-403.txt", excluded: true},
106116

107117
// Files copied with ADD command
108118
{filename: "builder3/3/file-301.mp3", excluded: true},
@@ -118,6 +128,9 @@ func testExcludedFilesOnCopy(t *testing.T, sb integration.Sandbox) {
118128
{filename: "builder4/4/file-301.mp3", excluded: true},
119129
{filename: "builder4/4/file-301.mp3", excluded: true},
120130
{filename: "builder4/4/file-302.mpeg", excluded: false, expectedContent: `10`},
131+
{filename: "builder4/4/dir4/file-401.txt", excluded: true},
132+
{filename: "builder4/4/dir4/file-402.txt", excluded: true},
133+
{filename: "builder4/4/dir4/file-403.txt", excluded: true},
121134

122135
// Files copied with ADD wildcard
123136
{filename: "builder5/5/file-101.png", excluded: true},
@@ -129,16 +142,30 @@ func testExcludedFilesOnCopy(t *testing.T, sb integration.Sandbox) {
129142
{filename: "builder5/5/file-301.mp3", excluded: true},
130143
{filename: "builder5/5/file-301.mp3", excluded: true},
131144
{filename: "builder5/5/file-302.mpeg", excluded: true},
145+
{filename: "builder5/5/dir4/file-401.txt", excluded: false, expectedContent: `11`},
146+
{filename: "builder5/5/dir4/file-402.txt", excluded: false, expectedContent: `file-401.txt`},
147+
{filename: "builder5/5/dir4/file-403.txt", excluded: false, expectedContent: `file-404.txt`},
132148
}
133149

134150
for _, tc := range testCases {
135-
dt, err := os.ReadFile(path.Join(destDir.Name, tc.filename))
151+
fpath := path.Join(destDir.Name, tc.filename)
152+
153+
var dt []byte
154+
st, err := os.Stat(fpath)
136155
if tc.excluded {
137156
require.Errorf(t, err, "File %s should not exist: %v", tc.filename, err)
138157
continue
139158
}
140-
141159
require.NoErrorf(t, err, 7F09 "File %s should exist", tc.filename)
160+
161+
if st.Mode()&os.ModeSymlink != 0 {
162+
target, err := os.Readlink(fpath)
163+
require.NoErrorf(t, err, "Readlink %s should not error", tc.filename)
164+
dt = []byte(target)
165+
} else {
166+
dt, err = os.ReadFile(fpath)
167+
require.NoErrorf(t, err, "File %s should exist", tc.filename)
168+
}
142169
require.Equalf(t, tc.expectedContent, string(dt), "File %s does not have matched content", tc.filename)
143170
}
144171

0 commit comments

Comments
 (0)
0