Skip to content

cli/command/container: copyToContainer: improve error-handling #6254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thaJeztah merged 2 commits into from
Aug 13, 2025
Merged

cli/command/container: copyToContainer: improve error-handling #6254

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5ea907
cli/command/container: copyToContainer rename error-return
thaJeztah Aug 12, 2025
eb5b03a
cli/command/container: copyToContainer: improve error-handling
thaJeztah Aug 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 30 additions & 30 deletions cli/command/container/cp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,50 +298,50 @@ func copyFromContainer(ctx context.Context, dockerCLI command.Cli, copyConfig cp
// about both the source and destination. The API is a simple tar
// archive/extract API but we can use the stat info header about the
// destination to be more informed about exactly what the destination is.
func copyToContainer(ctx context.Context, dockerCLI command.Cli, copyConfig cpConfig) (err error) {
func copyToContainer(ctx context.Context, dockerCLI command.Cli, copyConfig cpConfig) error {
srcPath := copyConfig.sourcePath
dstPath := copyConfig.destPath

if srcPath != "-" {
// Get an absolute source path.
srcPath, err = resolveLocalPath(srcPath)
p, err := resolveLocalPath(srcPath)
if err != nil {
return err
}
srcPath = p
}

apiClient := dockerCLI.Client()
// Prepare destination copy info by stat-ing the container path.
dstInfo := archive.CopyInfo{Path: dstPath}
dstStat, err := apiClient.ContainerStatPath(ctx, copyConfig.container, dstPath)

// If the destination is a symbolic link, we should evaluate it.
if err == nil && dstStat.Mode&os.ModeSymlink != 0 {
linkTarget := dstStat.LinkTarget
if !isAbs(linkTarget) {
// Join with the parent directory.
dstParent, _ := archive.SplitPathDirEntry(dstPath)
linkTarget = filepath.Join(dstParent, linkTarget)
}

dstInfo.Path = linkTarget
dstStat, err = apiClient.ContainerStatPath(ctx, copyConfig.container, linkTarget)
// FIXME(thaJeztah): unhandled error (should this return?)
}
if dstStat, err := apiClient.ContainerStatPath(ctx, copyConfig.container, dstPath); err == nil {
// If the destination is a symbolic link, we should evaluate it.
if dstStat.Mode&os.ModeSymlink != 0 {
linkTarget := dstStat.LinkTarget
if !isAbs(linkTarget) {
// Join with the parent directory.
dstParent, _ := archive.SplitPathDirEntry(dstPath)
linkTarget = filepath.Join(dstParent, linkTarget)
}

// Validate the destination path
if err := command.ValidateOutputPathFileMode(dstStat.Mode); err != nil {
return fmt.Errorf(`destination "%s:%s" must be a directory or a regular file: %w`, copyConfig.container, dstPath, err)
}
dstInfo.Path = linkTarget
dstStat, err = apiClient.ContainerStatPath(ctx, copyConfig.container, linkTarget)
Copy link

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error from the second ContainerStatPath call is not handled. If this call fails, the function continues with potentially stale dstStat data, which could lead to incorrect validation in the subsequent if block.

Suggested change
dstStat, err = apiClient.ContainerStatPath(ctx, copyConfig.container, linkTarget)
dstStat, err = apiClient.ContainerStatPath(ctx, copyConfig.container, linkTarget)
if err != nil {
return fmt.Errorf("failed to stat symlink target '%s' in container '%s': %w", linkTarget, copyConfig.container, err)
}

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error from the second ContainerStatPath call is not handled. If this call fails, the subsequent code at line 332 will still execute with a potentially invalid dstStat, which could cause issues in ValidateOutputPathFileMode.

Suggested change
dstStat, err = apiClient.ContainerStatPath(ctx, copyConfig.container, linkTarget)
dstStat, err = apiClient.ContainerStatPath(ctx, copyConfig.container, linkTarget)
if err != nil {
// Ignore any error and assume that the parent directory of the destination
// path exists, in which case the copy may still succeed. If there is any
// type of conflict (e.g., non-directory overwriting an existing directory
// or vice versa) the extraction will fail. If the destination simply did
// not exist, but the parent directory does, the extraction will still
// succeed.
_ = err // Intentionally ignore stat errors (see above)
return nil
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

READ THE BLOODY COMMENT @copilot - it's not hard, really, you should try it!

}
// Validate the destination path
if err == nil {
if err := command.ValidateOutputPathFileMode(dstStat.Mode); err != nil {
return fmt.Errorf(`destination "%s:%s" must be a directory or a regular file: %w`, copyConfig.container, dstPath, err)
}
dstInfo.Exists, dstInfo.IsDir = true, dstStat.Mode.IsDir()
}

// Ignore any error and assume that the parent directory of the destination
// path exists, in which case the copy may still succeed. If there is any
// type of conflict (e.g., non-directory overwriting an existing directory
// or vice versa) the extraction will fail. If the destination simply did
// not exist, but the parent directory does, the extraction will still
// succeed.
if err == nil {
dstInfo.Exists, dstInfo.IsDir = true, dstStat.Mode.IsDir()
// Ignore any error and assume that the parent directory of the destination
// path exists, in which case the copy may still succeed. If there is any
// type of conflict (e.g., non-directory overwriting an existing directory
// or vice versa) the extraction will fail. If the destination simply did
// not exist, but the parent directory does, the extraction will still
// succeed.
_ = err // Intentionally ignore stat errors (see above)
}

var (
Expand Down Expand Up @@ -411,7 +411,7 @@ func copyToContainer(ctx context.Context, dockerCLI command.Cli, copyConfig cpCo
cancel()
<-done
restore()
fmt.Fprintln(dockerCLI.Err(), "Successfully copied", progressHumanSize(copiedSize), "to", copyConfig.container+":"+dstInfo.Path)
_, _ = fmt.Fprintln(dockerCLI.Err(), "Successfully copied", progressHumanSize(copiedSize), "to", copyConfig.container+":"+dstInfo.Path)

return res
}
Expand Down
Loading