From d47d2338b718b7a5bb18ea940dd04488a2c8023d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 9 Mar 2025 21:24:49 +0100 Subject: [PATCH 1/3] image save: implement file-write with atomicwriter Same functionality, but implemented with atomicwriter. There's a slight difference in error-messages produced (but can be adjusted if we want). Before: docker image save -o ./no/such/foo busybox:latest failed to save image: invalid output path: directory "no/such" does not exist docker image save -o /no/permissions busybox:latest failed to save image: stat /no/permissions: permission denied After: docker image save -o ./no/such/foo busybox:latest failed to save image: invalid file path: stat no/such: no such file or directory docker image save -o /no/permissions busybox:latest failed to save image: failed to stat output path: lstat /no/permissions: permission denied Signed-off-by: Sebastiaan van Stijn --- cli/command/image/save.go | 36 +++++++++++++++++++--------------- cli/command/image/save_test.go | 4 ++-- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/cli/command/image/save.go b/cli/command/image/save.go index 813052278c3b..64bd72a74d8d 100644 --- a/cli/command/image/save.go +++ b/cli/command/image/save.go @@ -9,6 +9,7 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/completion" "github.com/docker/docker/client" + "github.com/moby/sys/atomicwriter" "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -48,15 +49,7 @@ func NewSaveCommand(dockerCli command.Cli) *cobra.Command { } // runSave performs a save against the engine based on the specified options -func runSave(ctx context.Context, dockerCli command.Cli, opts saveOptions) error { - if opts.output == "" && dockerCli.Out().IsTerminal() { - return errors.New("cowardly refusing to save to a terminal. Use the -o flag or redirect") - } - - if err := command.ValidateOutputPath(opts.output); err != nil { - return errors.Wrap(err, "failed to save image") - } - +func runSave(ctx context.Context, dockerCLI command.Cli, opts saveOptions) error { var options []client.ImageSaveOption if opts.platform != "" { p, err := platforms.Parse(opts.platform) @@ -67,16 +60,27 @@ func runSave(ctx context.Context, dockerCli command.Cli, opts saveOptions) error options = append(options, client.ImageSaveWithPlatforms(p)) } - responseBody, err := dockerCli.Client().ImageSave(ctx, opts.images, options...) - if err != nil { - return err + var output io.Writer + if opts.output == "" { + if dockerCLI.Out().IsTerminal() { + return errors.New("cowardly refusing to save to a terminal. Use the -o flag or redirect") + } + output = dockerCLI.Out() + } else { + writer, err := atomicwriter.New(opts.output, 0o600) + if err != nil { + return errors.Wrap(err, "failed to save image") + } + defer writer.Close() + output = writer } - defer responseBody.Close() - if opts.output == "" { - _, err := io.Copy(dockerCli.Out(), responseBody) + responseBody, err := dockerCLI.Client().ImageSave(ctx, opts.images, options...) + if err != nil { return err } + defer responseBody.Close() - return command.CopyToFile(opts.output, responseBody) + _, err = io.Copy(output, responseBody) + return err } diff --git a/cli/command/image/save_test.go b/cli/command/image/save_test.go index aa0bb5139dd6..7a3e93eb3547 100644 --- a/cli/command/image/save_test.go +++ b/cli/command/image/save_test.go @@ -44,12 +44,12 @@ func TestNewSaveCommandErrors(t *testing.T) { { name: "output directory does not exist", args: []string{"-o", "fakedir/out.tar", "arg1"}, - expectedError: "failed to save image: invalid output path: directory \"fakedir\" does not exist", + expectedError: `failed to save image: invalid output path: stat fakedir: no such file or directory`, }, { name: "output file is irregular", args: []string{"-o", "/dev/null", "arg1"}, - expectedError: "failed to save image: invalid output path: \"/dev/null\" must be a directory or a regular file", + expectedError: `failed to save image: cannot write to a character device file`, }, { name: "invalid platform", From b8bcf6f5adc799d882025fd41770c86c42c4c804 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 9 Mar 2025 21:25:44 +0100 Subject: [PATCH 2/3] container export: implement file-write with atomicwriter Same functionality, but implemented with atomicwriter. There's a slight difference in error-messages produced (but can be adjusted if we want). Before: docker container export -o ./no/such/foo mycontainer failed to export container: invalid output path: directory "no/such" does not exist docker container export -o /no/permissions mycontainer failed to export container: stat /no/permissions: permission denied After: docker container export -o ./no/such/foo mycontainer failed to export container: invalid file path: stat no/such: no such file or directory docker container export -o /no/permissions mycontainer failed to export container: failed to stat output path: lstat /no/permissions: permission denied Signed-off-by: Sebastiaan van Stijn --- cli/command/container/export.go | 34 +++++++++++++++------------- cli/command/container/export_test.go | 6 ++--- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/cli/command/container/export.go b/cli/command/container/export.go index e9afa7dbe104..990c2e66f8c8 100644 --- a/cli/command/container/export.go +++ b/cli/command/container/export.go @@ -7,6 +7,7 @@ import ( "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/completion" + "github.com/moby/sys/atomicwriter" "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -41,27 +42,28 @@ func NewExportCommand(dockerCli command.Cli) *cobra.Command { return cmd } -func runExport(ctx context.Context, dockerCli command.Cli, opts exportOptions) error { - if opts.output == "" && dockerCli.Out().IsTerminal() { - return errors.New("cowardly refusing to save to a terminal. Use the -o flag or redirect") - } - - if err := command.ValidateOutputPath(opts.output); err != nil { - return errors.Wrap(err, "failed to export container") +func runExport(ctx context.Context, dockerCLI command.Cli, opts exportOptions) error { + var output io.Writer + if opts.output == "" { + if dockerCLI.Out().IsTerminal() { + return errors.New("cowardly refusing to save to a terminal. Use the -o flag or redirect") + } + output = dockerCLI.Out() + } else { + writer, err := atomicwriter.New(opts.output, 0o600) + if err != nil { + return errors.Wrap(err, "failed to export container") + } + defer writer.Close() + output = writer } - clnt := dockerCli.Client() - - responseBody, err := clnt.ContainerExport(ctx, opts.container) + responseBody, err := dockerCLI.Client().ContainerExport(ctx, opts.container) if err != nil { return err } defer responseBody.Close() - if opts.output == "" { - _, err := io.Copy(dockerCli.Out(), responseBody) - return err - } - - return command.CopyToFile(opts.output, responseBody) + _, err = io.Copy(output, responseBody) + return err } diff --git a/cli/command/container/export_test.go b/cli/command/container/export_test.go index ae21a8799533..182713ab993a 100644 --- a/cli/command/container/export_test.go +++ b/cli/command/container/export_test.go @@ -42,8 +42,6 @@ func TestContainerExportOutputToIrregularFile(t *testing.T) { cmd.SetErr(io.Discard) cmd.SetArgs([]string{"-o", "/dev/random", "container"}) - err := cmd.Execute() - assert.Assert(t, err != nil) - expected := `"/dev/random" must be a directory or a regular file` - assert.ErrorContains(t, err, expected) + const expected = `failed to export container: cannot write to a character device file` + assert.Error(t, cmd.Execute(), expected) } From 7cc6b8ebf4aa1754ac9309027f315b270ea47c34 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 9 Mar 2025 21:26:19 +0100 Subject: [PATCH 3/3] cli/command: deprecate CopyToFile and reimplement with atomicwriter Signed-off-by: Sebastiaan van Stijn --- cli/command/utils.go | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/cli/command/utils.go b/cli/command/utils.go index c3d8339f2b87..8b31ada7422c 100644 --- a/cli/command/utils.go +++ b/cli/command/utils.go @@ -17,37 +17,23 @@ import ( "github.com/docker/cli/cli/streams" "github.com/docker/docker/api/types/filters" "github.com/docker/docker/errdefs" - "github.com/moby/sys/sequential" + "github.com/moby/sys/atomicwriter" "github.com/moby/term" "github.com/pkg/errors" "github.com/spf13/pflag" ) // CopyToFile writes the content of the reader to the specified file +// +// Deprecated: use [atomicwriter.New]. func CopyToFile(outfile string, r io.Reader) error { - // We use sequential file access here to avoid depleting the standby list - // on Windows. On Linux, this is a call directly to os.CreateTemp - tmpFile, err := sequential.CreateTemp(filepath.Dir(outfile), ".docker_temp_") - if err != nil { - return err - } - - tmpPath := tmpFile.Name() - - _, err = io.Copy(tmpFile, r) - tmpFile.Close() - + writer, err := atomicwriter.New(outfile, 0o600) if err != nil { - os.Remove(tmpPath) - return err - } - - if err = os.Rename(tmpPath, outfile); err != nil { - os.Remove(tmpPath) return err } - - return nil + defer writer.Close() + _, err = io.Copy(writer, r) + return err } var ErrPromptTerminated = errdefs.Cancelled(errors.New("prompt terminated")) @@ -187,7 +173,7 @@ func AddPlatformFlag(flags *pflag.FlagSet, target *string) { _ = flags.SetAnnotation("platform", "version", []string{"1.32"}) } -// ValidateOutputPath validates the output paths of the `export` and `save` commands. +// ValidateOutputPath validates the output paths of the "docker cp" command. func ValidateOutputPath(path string) error { dir := filepath.Dir(filepath.Clean(path)) if dir != "" && dir != "." { @@ -213,8 +199,8 @@ func ValidateOutputPath(path string) error { return nil } -// ValidateOutputPathFileMode validates the output paths of the `cp` command and serves as a -// helper to `ValidateOutputPath` +// ValidateOutputPathFileMode validates the output paths of the "docker cp" command +// and serves as a helper to [ValidateOutputPath] func ValidateOutputPathFileMode(fileMode os.FileMode) error { switch { case fileMode&os.ModeDevice != 0: