From 8b9ecfde650a571e1a8a5032ad497bbff02280d0 Mon Sep 17 00:00:00 2001 From: Lidang-Jiang Date: Sun, 29 Mar 2026 10:26:05 +0800 Subject: [PATCH 1/2] fix: return error on non-ErrNotExist stat failures in Tar.Sync() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, Sync() only checked for fs.ErrNotExist when classifying paths into copy vs delete. Non-NotExist stat errors (e.g. EACCES, EIO) caused the condition to be false, falling through to the else clause which incorrectly treated the path as copyable. This masked real errors and led to cryptic failures downstream. Restructure the condition into a three-way branch: - err == nil → copy - ErrNotExist → delete - other errors → return immediately with context This follows the pattern already used by entriesForPath() in the same file. Fixes #13654 Signed-off-by: Lidang Jiang Signed-off-by: Lidang-Jiang --- internal/sync/tar.go | 6 +- internal/sync/tar_test.go | 140 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 2 deletions(-) create mode 100644 internal/sync/tar_test.go diff --git a/internal/sync/tar.go b/internal/sync/tar.go index 216155e44c0..64ab02d508b 100644 --- a/internal/sync/tar.go +++ b/internal/sync/tar.go @@ -73,10 +73,12 @@ func (t *Tar) Sync(ctx context.Context, service string, paths []*PathMapping) er var pathsToCopy []PathMapping var pathsToDelete []string for _, p := range paths { - if _, err := os.Stat(p.HostPath); err != nil && errors.Is(err, fs.ErrNotExist) { + if _, err := os.Stat(p.HostPath); err == nil { + pathsToCopy = append(pathsToCopy, *p) + } else if errors.Is(err, fs.ErrNotExist) { pathsToDelete = append(pathsToDelete, p.ContainerPath) } else { - pathsToCopy = append(pathsToCopy, *p) + return fmt.Errorf("stat %q: %w", p.HostPath, err) } } diff --git a/internal/sync/tar_test.go b/internal/sync/tar_test.go new file mode 100644 index 00000000000..8f4c391aa4a --- /dev/null +++ b/internal/sync/tar_test.go @@ -0,0 +1,140 @@ +/* + Copyright 2023 Docker Compose CLI authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package sync + +import ( + "context" + "io" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/moby/moby/api/types/container" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeLowLevelClient records calls made to it for test assertions. +type fakeLowLevelClient struct { + containers []container.Summary + execCmds [][]string + untarCount int +} + +func (f *fakeLowLevelClient) ContainersForService(_ context.Context, _ string, _ string) ([]container.Summary, error) { + return f.containers, nil +} + +func (f *fakeLowLevelClient) Exec(_ context.Context, _ string, cmd []string, _ io.Reader) error { + f.execCmds = append(f.execCmds, cmd) + return nil +} + +func (f *fakeLowLevelClient) Untar(_ context.Context, _ string, _ io.ReadCloser) error { + f.untarCount++ + return nil +} + +func TestSync_ExistingPath(t *testing.T) { + tmpDir := t.TempDir() + existingFile := filepath.Join(tmpDir, "exists.txt") + require.NoError(t, os.WriteFile(existingFile, []byte("data"), 0o644)) + + client := &fakeLowLevelClient{ + containers: []container.Summary{{ID: "ctr1"}}, + } + tar := NewTar("proj", client) + + err := tar.Sync(t.Context(), "svc", []*PathMapping{ + {HostPath: existingFile, ContainerPath: "/app/exists.txt"}, + }) + + require.NoError(t, err) + assert.Equal(t, 1, client.untarCount, "existing path should be copied via Untar") + assert.Empty(t, client.execCmds, "no delete command expected for existing path") +} + +func TestSync_NonExistentPath(t *testing.T) { + client := &fakeLowLevelClient{ + containers: []container.Summary{{ID: "ctr1"}}, + } + tar := NewTar("proj", client) + + err := tar.Sync(t.Context(), "svc", []*PathMapping{ + {HostPath: "/no/such/file", ContainerPath: "/app/gone.txt"}, + }) + + require.NoError(t, err) + require.Len(t, client.execCmds, 1, "should issue a delete command") + assert.Equal(t, []string{"rm", "-rf", "/app/gone.txt"}, client.execCmds[0]) +} + +func TestSync_StatPermissionError(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("permission-based test not reliable on Windows") + } + if os.Getuid() == 0 { + t.Skip("test requires non-root to trigger EACCES") + } + + tmpDir := t.TempDir() + restrictedDir := filepath.Join(tmpDir, "noaccess") + require.NoError(t, os.Mkdir(restrictedDir, 0o700)) + targetFile := filepath.Join(restrictedDir, "secret.txt") + require.NoError(t, os.WriteFile(targetFile, []byte("data"), 0o644)) + // Remove all permissions on the parent directory so stat on the child fails with EACCES. + require.NoError(t, os.Chmod(restrictedDir, 0o000)) + t.Cleanup(func() { + // Restore permissions so t.TempDir() cleanup can remove it. + _ = os.Chmod(restrictedDir, 0o700) + }) + + client := &fakeLowLevelClient{ + containers: []container.Summary{{ID: "ctr1"}}, + } + tar := NewTar("proj", client) + + err := tar.Sync(t.Context(), "svc", []*PathMapping{ + {HostPath: targetFile, ContainerPath: "/app/secret.txt"}, + }) + + require.Error(t, err) + assert.Contains(t, err.Error(), "permission denied") + assert.Contains(t, err.Error(), "secret.txt") + assert.Equal(t, 0, client.untarCount, "should not attempt copy on stat error") + assert.Empty(t, client.execCmds, "should not attempt delete on stat error") +} + +func TestSync_MixedPaths(t *testing.T) { + tmpDir := t.TempDir() + existingFile := filepath.Join(tmpDir, "keep.txt") + require.NoError(t, os.WriteFile(existingFile, []byte("data"), 0o644)) + + client := &fakeLowLevelClient{ + containers: []container.Summary{{ID: "ctr1"}}, + } + tar := NewTar("proj", client) + + err := tar.Sync(t.Context(), "svc", []*PathMapping{ + {HostPath: existingFile, ContainerPath: "/app/keep.txt"}, + {HostPath: "/no/such/path", ContainerPath: "/app/removed.txt"}, + }) + + require.NoError(t, err) + assert.Equal(t, 1, client.untarCount, "existing path should be copied") + require.Len(t, client.execCmds, 1) + assert.Contains(t, client.execCmds[0][len(client.execCmds[0])-1], "removed.txt") +} From 0b252518907f1ec3fde83e0e7bc34a3fbed8d4f7 Mon Sep 17 00:00:00 2001 From: Lidang-Jiang Date: Wed, 1 Apr 2026 20:48:25 +0800 Subject: [PATCH 2/2] test: migrate tar_test.go from testify to gotest.tools/v3 Signed-off-by: Lidang-Jiang --- internal/sync/tar_test.go | 43 +++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/internal/sync/tar_test.go b/internal/sync/tar_test.go index 8f4c391aa4a..0c575228337 100644 --- a/internal/sync/tar_test.go +++ b/internal/sync/tar_test.go @@ -23,8 +23,8 @@ import ( "testing" "github.com/moby/moby/api/types/container" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" + "gotest.tools/v3/assert" + "gotest.tools/v3/assert/cmp" ) // fakeLowLevelClient records calls made to it for test assertions. @@ -51,7 +51,7 @@ func (f *fakeLowLevelClient) Untar(_ context.Context, _ string, _ io.ReadCloser) func TestSync_ExistingPath(t *testing.T) { tmpDir := t.TempDir() existingFile := filepath.Join(tmpDir, "exists.txt") - require.NoError(t, os.WriteFile(existingFile, []byte("data"), 0o644)) + assert.NilError(t, os.WriteFile(existingFile, []byte("data"), 0o644)) client := &fakeLowLevelClient{ containers: []container.Summary{{ID: "ctr1"}}, @@ -62,9 +62,9 @@ func TestSync_ExistingPath(t *testing.T) { {HostPath: existingFile, ContainerPath: "/app/exists.txt"}, }) - require.NoError(t, err) - assert.Equal(t, 1, client.untarCount, "existing path should be copied via Untar") - assert.Empty(t, client.execCmds, "no delete command expected for existing path") + assert.NilError(t, err) + assert.Equal(t, client.untarCount, 1, "existing path should be copied via Untar") + assert.Equal(t, len(client.execCmds), 0, "no delete command expected for existing path") } func TestSync_NonExistentPath(t *testing.T) { @@ -77,9 +77,9 @@ func TestSync_NonExistentPath(t *testing.T) { {HostPath: "/no/such/file", ContainerPath: "/app/gone.txt"}, }) - require.NoError(t, err) - require.Len(t, client.execCmds, 1, "should issue a delete command") - assert.Equal(t, []string{"rm", "-rf", "/app/gone.txt"}, client.execCmds[0]) + assert.NilError(t, err) + assert.Equal(t, len(client.execCmds), 1, "should issue a delete command") + assert.DeepEqual(t, client.execCmds[0], []string{"rm", "-rf", "/app/gone.txt"}) } func TestSync_StatPermissionError(t *testing.T) { @@ -92,11 +92,11 @@ func TestSync_StatPermissionError(t *testing.T) { tmpDir := t.TempDir() restrictedDir := filepath.Join(tmpDir, "noaccess") - require.NoError(t, os.Mkdir(restrictedDir, 0o700)) + assert.NilError(t, os.Mkdir(restrictedDir, 0o700)) targetFile := filepath.Join(restrictedDir, "secret.txt") - require.NoError(t, os.WriteFile(targetFile, []byte("data"), 0o644)) + assert.NilError(t, os.WriteFile(targetFile, []byte("data"), 0o644)) // Remove all permissions on the parent directory so stat on the child fails with EACCES. - require.NoError(t, os.Chmod(restrictedDir, 0o000)) + assert.NilError(t, os.Chmod(restrictedDir, 0o000)) t.Cleanup(func() { // Restore permissions so t.TempDir() cleanup can remove it. _ = os.Chmod(restrictedDir, 0o700) @@ -111,17 +111,16 @@ func TestSync_StatPermissionError(t *testing.T) { {HostPath: targetFile, ContainerPath: "/app/secret.txt"}, }) - require.Error(t, err) - assert.Contains(t, err.Error(), "permission denied") - assert.Contains(t, err.Error(), "secret.txt") - assert.Equal(t, 0, client.untarCount, "should not attempt copy on stat error") - assert.Empty(t, client.execCmds, "should not attempt delete on stat error") + assert.ErrorContains(t, err, "permission denied") + assert.ErrorContains(t, err, "secret.txt") + assert.Equal(t, client.untarCount, 0, "should not attempt copy on stat error") + assert.Equal(t, len(client.execCmds), 0, "should not attempt delete on stat error") } func TestSync_MixedPaths(t *testing.T) { tmpDir := t.TempDir() existingFile := filepath.Join(tmpDir, "keep.txt") - require.NoError(t, os.WriteFile(existingFile, []byte("data"), 0o644)) + assert.NilError(t, os.WriteFile(existingFile, []byte("data"), 0o644)) client := &fakeLowLevelClient{ containers: []container.Summary{{ID: "ctr1"}}, @@ -133,8 +132,8 @@ func TestSync_MixedPaths(t *testing.T) { {HostPath: "/no/such/path", ContainerPath: "/app/removed.txt"}, }) - require.NoError(t, err) - assert.Equal(t, 1, client.untarCount, "existing path should be copied") - require.Len(t, client.execCmds, 1) - assert.Contains(t, client.execCmds[0][len(client.execCmds[0])-1], "removed.txt") + assert.NilError(t, err) + assert.Equal(t, client.untarCount, 1, "existing path should be copied") + assert.Equal(t, len(client.execCmds), 1) + assert.Check(t, cmp.Contains(client.execCmds[0][len(client.execCmds[0])-1], "removed.txt")) }