From 6e20be016d41127a3b6450caed8e62d7c746eed9 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Tue, 29 Sep 2026 18:28:54 -0700 Subject: [PATCH] exporter: fix src error cleanup and resolution Release the mount or temp dir in CreateFS on every error path. An invalid src could previously leak a mount for each failed export, and failing attestation steps had the same leak. Pass src to fs.RootPath unmodified so symlinks are resolved before "..", and stop trimming whitespace from the value. Keep the underlying error when reporting src failures so errors.Is still matches. Add tests for mount release, mode=delete and platform-split=false with src, and make the src error assertions work on Windows. Note in the README that older daemons ignore src. Signed-off-by: Tonis Tiigi --- README.md | 3 + client/client_export_local_test.go | 71 ++++++++--- client/client_test.go | 2 + exporter/local/fs.go | 30 +++-- exporter/local/fs_test.go | 192 +++++++++++++++-------------- 5 files changed, 181 insertions(+), 117 deletions(-) diff --git a/README.md b/README.md index b338c3b16..43015f41f 100644 --- a/README.md +++ b/README.md @@ -342,6 +342,9 @@ are resolved within the build result: a link pointing at `/etc` refers to `/etc` inside the result, never on the host running BuildKit. With a multi-platform build, `src` is applied to each platform. +Use a BuildKit daemon that supports `src`. Older daemons ignore unknown exporter +options and may export the entire build result instead. + With a [multi-platform build](docs/multi-platform.md), a subfolder matching each target platform will be created in the destination directory: diff --git a/client/client_export_local_test.go b/client/client_export_local_test.go index c4ab31996..84ed6c8c2 100644 --- a/client/client_export_local_test.go +++ b/client/client_export_local_test.go @@ -1005,6 +1005,32 @@ func testExportLocalSource(t *testing.T, sb integration.Sandbox) { ))) } +func testExportLocalSourceModeDelete(t *testing.T, sb integration.Sandbox) { + c, err := New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + def, err := sourceTestState().Marshal(sb.Context()) + require.NoError(t, err) + + destDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(destDir, "stale.txt"), []byte("stale"), 0600)) + _, err = c.Solve(sb.Context(), def, SolveOpt{ + Exports: []ExportEntry{{ + Type: ExporterLocal, + OutputDir: destDir, + Attrs: map[string]string{"src": "/sub", "mode": "delete"}, + }}, + }, nil) + require.NoError(t, err) + + require.NoError(t, fstest.CheckDirectoryEqualWithApplier(destDir, fstest.Apply( + fstest.CreateFile("nested.txt", []byte("nested"), 0600), + fstest.CreateDir("deeper", 0755), + fstest.CreateFile("deeper/deep.txt", []byte("deep"), 0600), + ))) +} + func testExportLocalSourceNotFound(t *testing.T, sb integration.Sandbox) { c, err := New(sb.Context(), sb.Address()) require.NoError(t, err) @@ -1023,34 +1049,38 @@ func testExportLocalSourceNotFound(t *testing.T, sb integration.Sandbox) { }, }, }, nil) - require.Error(t, err) - require.ErrorContains(t, err, "src=/nope no such file or directory") + require.ErrorContains(t, err, "src=/nope:") // the mountpoint of the ref inside the daemon must never reach the client require.NotContains(t, err.Error(), "buildkit-mount") } func testExportLocalSourceMultiPlatform(t *testing.T, sb integration.Sandbox) { - workers.CheckFeatureCompat(t, sb, workers.FeatureOCIExporter, workers.FeatureMultiPlatform) + testExportLocalSourceMultiPlatformSplit(t, sb, true) +} + +func testExportLocalSourceNoPlatformSplit(t *testing.T, sb integration.Sandbox) { + testExportLocalSourceMultiPlatformSplit(t, sb, false) +} + +func testExportLocalSourceMultiPlatformSplit(t *testing.T, sb integration.Sandbox, split bool) { + workers.CheckFeatureCompat(t, sb, workers.FeatureMultiPlatform) c, err := New(sb.Context(), sb.Address()) require.NoError(t, err) defer c.Close() platformsToTest := []string{"linux/amd64", "linux/arm64"} - // every platform builds the same layout and differs only in file contents, - // so the exported platform directories can be compared against each other frontend := func(ctx context.Context, c gateway.Client) (*gateway.Result, error) { res := gateway.NewResult() expPlatforms := &exptypes.Platforms{ Platforms: make([]exptypes.Platform, len(platformsToTest)), } for i, platform := range platformsToTest { + name := strings.ReplaceAll(platform, "/", "_") + ".txt" st := llb.Scratch(). File(llb.Mkfile("top.txt", 0600, []byte("top"))). File(llb.Mkdir("sub", 0755)). - File(llb.Mkfile("sub/nested.txt", 0600, []byte(platform))). - File(llb.Mkdir("sub/deeper", 0755)). - File(llb.Mkfile("sub/deeper/deep.txt", 0600, []byte(platform))) + File(llb.Mkfile("sub/"+name, 0600, []byte(platform))) def, err := st.Marshal(ctx) if err != nil { @@ -1084,22 +1114,29 @@ func testExportLocalSourceMultiPlatform(t *testing.T, sb integration.Sandbox) { { Type: ExporterLocal, OutputDir: destDir, - Attrs: map[string]string{"src": "/sub"}, + Attrs: map[string]string{"src": "/sub", "platform-split": fmt.Sprint(split)}, }, }, }, "", frontend, nil) require.NoError(t, err) - // src is applied per platform: every platform directory holds the same - // re-rooted layout, with only the file contents telling them apart for _, platform := range platformsToTest { - platDir := filepath.Join(destDir, strings.ReplaceAll(platform, "/", "_")) - require.NoError(t, fstest.CheckDirectoryEqualWithApplier(platDir, fstest.Apply( - fstest.CreateFile("nested.txt", []byte(platform), 0600), - fstest.CreateDir("deeper", 0755), - fstest.CreateFile("deeper/deep.txt", []byte(platform), 0600), - )), "unexpected content for %s", platform) + name := strings.ReplaceAll(platform, "/", "_") + ".txt" + outputDir := destDir + if split { + outputDir = filepath.Join(destDir, strings.ReplaceAll(platform, "/", "_")) + require.NoError(t, fstest.CheckDirectoryEqualWithApplier(outputDir, fstest.Apply( + fstest.CreateFile(name, []byte(platform), 0600), + )), "unexpected content for %s", platform) + continue + } + dt, err := os.ReadFile(filepath.Join(outputDir, name)) + require.NoError(t, err) + require.Equal(t, platform, string(dt)) } + entries, err := os.ReadDir(destDir) + require.NoError(t, err) + require.Len(t, entries, len(platformsToTest)) } func testExportTarSource(t *testing.T, sb integration.Sandbox) { diff --git a/client/client_test.go b/client/client_test.go index cf3d70073..5d91bc70d 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -91,8 +91,10 @@ var allTests = []func(t *testing.T, sb integration.Sandbox){ testExportLocalNoPlatformSplit, testExportLocalNoPlatformSplitOverwrite, testExportLocalSource, + testExportLocalSourceModeDelete, testExportLocalSourceNotFound, testExportLocalSourceMultiPlatform, + testExportLocalSourceNoPlatformSplit, testExportTarSource, testExportTarPlatformIDSanitized, testExporterTargetExists, diff --git a/exporter/local/fs.go b/exporter/local/fs.go index 4ebd7b03c..4aa2815bb 100644 --- a/exporter/local/fs.go +++ b/exporter/local/fs.go @@ -76,11 +76,10 @@ func (c *CreateFSOpts) Load(opt map[string]string) (map[string]string, error) { return nil, err } case keySource: - var src = strings.TrimSpace(v) - if src == "" { - return nil, errors.Errorf("empty value for %s omit it to export the entire filesystem", keySource) + if v == "" { + return nil, errors.Errorf("empty value for %s; omit it to export the entire filesystem", keySource) } - c.Source = path.Join("/", src) + c.Source = v default: rest[k] = v } @@ -89,9 +88,11 @@ func (c *CreateFSOpts) Load(opt map[string]string) (map[string]string, error) { return rest, nil } -// Resolves source inside mountRoot and prevents path traversal +// resolveSafeSource resolves source inside mountRoot and prevents path traversal. // An empty source returns mountRoot itself. func resolveSafeSource(mountRoot, source string) (fsutil.FS, error) { + // RootPath must see the original path so it can resolve symlinks before "..". + // The mounted ref must remain read-only while the returned FS is in use. root, err := fs.RootPath(mountRoot, source) if err != nil { return nil, sourceError(err, source) @@ -105,7 +106,7 @@ func resolveSafeSource(mountRoot, source string) (fsutil.FS, error) { return outputFS, nil } -// Reports err against the source the client asked for, hiding the +// sourceError reports err against the source the client asked for, hiding the // daemon-side mountpoint that RootPath and NewFS name. func sourceError(err error, source string) error { if source == "" { @@ -113,12 +114,14 @@ func sourceError(err error, source string) error { } // the innermost *os.PathError carries the bare syscall error, with no path cause := err - for e := err; e != nil; e = errors.Unwrap(e) { - if pe, ok := e.(*os.PathError); ok { - cause = pe.Err + for { + var pe *os.PathError + if !errors.As(cause, &pe) { + break } + cause = pe.Err } - return errors.Errorf("%s=%s %v", keySource, source, cause) + return errors.Wrapf(cause, "%s=%s", keySource, source) } func CreateFS(ctx context.Context, sessionID string, k string, ref cache.ImmutableRef, attestations []exporter.Attestation, defaultTime time.Time, isMap bool, opt CreateFSOpts) (fsutil.FS, func() error, error) { @@ -149,6 +152,12 @@ func CreateFS(ctx context.Context, sessionID string, k string, ref cache.Immutab cleanup = lm.Unmount } + releaseOnError := true + defer func() { + if releaseOnError && cleanup != nil { + _ = cleanup() + } + }() outputFS, err := resolveSafeSource(src, opt.Source) if err != nil { @@ -260,5 +269,6 @@ func CreateFS(ctx context.Context, sessionID string, k string, ref cache.Immutab outputFS = staticfs.NewMergeFS(outputFS, stmtFS) } + releaseOnError = false return outputFS, cleanup, nil } diff --git a/exporter/local/fs_test.go b/exporter/local/fs_test.go index 22963fc74..38c1ffa67 100644 --- a/exporter/local/fs_test.go +++ b/exporter/local/fs_test.go @@ -1,102 +1,34 @@ package local import ( + "context" "io/fs" + "os" + "path/filepath" "sort" + "syscall" "testing" + "time" + "github.com/containerd/containerd/v2/core/mount" "github.com/containerd/continuity/fs/fstest" + "github.com/moby/buildkit/cache" + "github.com/moby/buildkit/session" + "github.com/moby/buildkit/snapshot" + "github.com/moby/sys/user" "github.com/stretchr/testify/require" "github.com/tonistiigi/fsutil" ) func TestCreateFSOptsLoadSource(t *testing.T) { - tests := []struct { - name string - attrs map[string]string - want string - wantErr string - }{ - { - name: "unset", - attrs: map[string]string{}, - want: "", - }, - { - name: "empty", - attrs: map[string]string{keySource: ""}, - wantErr: "empty value for src", - }, - { - name: "whitespace only", - attrs: map[string]string{keySource: " "}, - wantErr: "empty value for src", - }, - { - name: "absolute", - attrs: map[string]string{keySource: "/app/build"}, - want: "/app/build", - }, - { - name: "trailing slash", - attrs: map[string]string{keySource: "/app/build/"}, - want: "/app/build", - }, - { - name: "relative is anchored to root", - attrs: map[string]string{keySource: "app/build"}, - want: "/app/build", - }, - { - name: "dot slash prefix", - attrs: map[string]string{keySource: "./app/build"}, - want: "/app/build", - }, - { - name: "redundant separators", - attrs: map[string]string{keySource: "/a//b/./c"}, - want: "/a/b/c", - }, - { - name: "dot is root", - attrs: map[string]string{keySource: "."}, - want: "/", - }, - { - name: "slash is root", - attrs: map[string]string{keySource: "/"}, - want: "/", - }, - { - name: "surrounding whitespace is trimmed", - attrs: map[string]string{keySource: " /app "}, - want: "/app", - }, - { - // Clamping at the root matches what BuildKit already does for - // container-side paths: COPY --from, and both source and target of - // RUN --mount, accept ".." and clamp it the same way. - name: "parent traversal is clamped to root", - attrs: map[string]string{keySource: "../../etc"}, - want: "/etc", - }, - } + var opts CreateFSOpts + rest, err := opts.Load(map[string]string{keySource: " "}) + require.NoError(t, err) + require.Equal(t, " ", opts.Source) + require.NotContains(t, rest, keySource) - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - var opts CreateFSOpts - rest, err := opts.Load(tc.attrs) - - if tc.wantErr != "" { - require.ErrorContains(t, err, tc.wantErr) - return - } - - require.NoError(t, err) - require.Equal(t, tc.want, opts.Source) - require.NotContains(t, rest, keySource) - }) - } + _, err = opts.Load(map[string]string{keySource: ""}) + require.ErrorContains(t, err, "empty value for src") } // builds a container filesystem @@ -135,7 +67,7 @@ func walkNames(t *testing.T, f fsutil.FS) []string { if err != nil { return err } - names = append(names, p) + names = append(names, filepath.ToSlash(p)) return nil })) sort.Strings(names) @@ -148,12 +80,18 @@ func TestResolveSafeSource(t *testing.T) { source string want []string wantErr string + wantIs error }{ { name: "empty source exports the whole mount", source: "", want: []string{"app", "etc", "etc/inside.txt", "rel", "sub", "sub/nested.txt", "top.txt"}, }, + { + name: "relative source is anchored to mount", + source: "sub", + want: []string{"nested.txt"}, + }, { name: "subdirectory is re-rooted", source: "/sub", @@ -170,7 +108,7 @@ func TestResolveSafeSource(t *testing.T) { want: []string{"nested.txt"}, }, { - // same clamping as COPY --from and RUN --mount, see the Load test + // Parent traversal cannot escape the mounted result. name: "parent traversal is clamped to the mount", source: "../../etc", want: []string{"inside.txt"}, @@ -178,12 +116,14 @@ func TestResolveSafeSource(t *testing.T) { { name: "missing path fails", source: "/nope", - wantErr: "src=/nope no such file or directory", + wantErr: "src=/nope:", + wantIs: os.ErrNotExist, }, { name: "file is not a directory", source: "/top.txt", - wantErr: "src=/top.txt not a directory", + wantErr: "src=/top.txt:", + wantIs: syscall.ENOTDIR, }, } @@ -191,7 +131,8 @@ func TestResolveSafeSource(t *testing.T) { t.Run(tc.name, func(t *testing.T) { outputFS, err := resolveSafeSource(newRootfs(t), tc.source) if tc.wantErr != "" { - require.EqualError(t, err, tc.wantErr) + require.ErrorContains(t, err, tc.wantErr) + require.ErrorIs(t, err, tc.wantIs) return } require.NoError(t, err) @@ -199,3 +140,74 @@ func TestResolveSafeSource(t *testing.T) { }) } } + +func TestSourceResolvesParentAfterSymlink(t *testing.T) { + root := t.TempDir() + require.NoError(t, fstest.Apply( + fstest.CreateFile("top.txt", []byte("top"), 0600), + fstest.CreateDir("sub", 0755), + fstest.CreateFile("sub/nested.txt", []byte("nested"), 0600), + fstest.CreateDir("sub/inner", 0755), + fstest.Symlink("/sub/inner", "link"), + ).Apply(root)) + + var opts CreateFSOpts + _, err := opts.Load(map[string]string{keySource: "/link/.."}) + require.NoError(t, err) + + outputFS, err := resolveSafeSource(root, opts.Source) + require.NoError(t, err) + require.Equal(t, []string{"inner", "nested.txt"}, walkNames(t, outputFS)) +} + +func TestCreateFSReleasesTempDirOnSourceError(t *testing.T) { + tempDir := t.TempDir() + t.Setenv("TMPDIR", tempDir) + t.Setenv("TMP", tempDir) + t.Setenv("TEMP", tempDir) + require.Equal(t, tempDir, os.TempDir()) + + _, cleanup, err := CreateFS(t.Context(), "", "", nil, nil, time.Time{}, false, CreateFSOpts{Source: "/missing"}) + require.ErrorIs(t, err, os.ErrNotExist) + require.Nil(t, cleanup) + + entries, err := os.ReadDir(tempDir) + require.NoError(t, err) + require.Empty(t, entries) +} + +type sourceTestRef struct { + // CreateFS only calls Mount; the remaining methods are not needed here. + cache.ImmutableRef + mount snapshot.Mountable +} + +func (r sourceTestRef) Mount(context.Context, bool, session.Group) (snapshot.Mountable, error) { + return r.mount, nil +} + +type sourceTestMountable struct { + root string + releases int +} + +func (m *sourceTestMountable) Mount() ([]mount.Mount, func() error, error) { + return []mount.Mount{{Type: "bind", Source: m.root}}, func() error { + m.releases++ + return nil + }, nil +} + +func (m *sourceTestMountable) IdentityMapping() *user.IdentityMapping { + return nil +} + +func TestCreateFSReleasesMountOnSourceError(t *testing.T) { + m := &sourceTestMountable{root: t.TempDir()} + ref := sourceTestRef{mount: m} + + _, cleanup, err := CreateFS(t.Context(), "", "", ref, nil, time.Time{}, false, CreateFSOpts{Source: "/missing"}) + require.ErrorIs(t, err, os.ErrNotExist) + require.Nil(t, cleanup) + require.Equal(t, 1, m.releases) +}