From 1dfca6084fbe34c774ea8f35847e893e13b2e632 Mon Sep 17 00:00:00 2001 From: Joe Stringer Date: Fri, 26 Jan 2024 12:21:13 +0000 Subject: [PATCH] images: Fix read past end of file for pull --cache Pull has two modes: - No cache: Extract and decompress the target VM image from the docker filesystem to the host filesystem. Result: One file, image.qcow. - Cache: Same as no cache, but also copy the compressed image to the host filesystem. Result: two files, image.qcow{,.zst} Previously, the logic for extracting the target VM image from the docker image with --cache enabled was to: - Extract and decompress the target file directly from inside the docker image filesystem to the host filesystem - Do not seek the source Reader back to the start of the file - Attempt (and fail) to copy the entire compressed image also to the host filesystem, but starting from the end of the source file. - This resulted in an EOF error because the reader wasn't reset to start from the beginning. Unfortunately, tar.Reader doesn't expose a way to seek back to the start of the source file. We could perhaps get an additional reference to a reader starting from the beginning of the current tar header in order to reset back to the beginning of the target file in cache mode, but this was getting a bit complicated. The solution in this patch is to: - Copy the compressed file to a temporary path on the host filesystem - Extract the compressed file from this temporary host-side cache to the intended destination directory. - If in cache mode, move the temporary file also to the output directory. Otherwise, delete the temporary file. There's one minor extra case, if the qcow inside the docker image is not compressed at all then we skip decompression, and in that case we also need to move the temporary path to the destination directory. Fixes: ea09cb7b4e95 ("images: Add docker pull support") Reported-by: Lorenz Bauer Signed-off-by: Joe Stringer --- pkg/images/pull.go | 78 ++++++++++++++++++++++++++++++---------------- 1 file changed, 52 insertions(+), 26 deletions(-) diff --git a/pkg/images/pull.go b/pkg/images/pull.go index 55a6cdac..8b117a32 100644 --- a/pkg/images/pull.go +++ b/pkg/images/pull.go @@ -5,6 +5,7 @@ package images import ( "archive/tar" + "bufio" "context" "errors" "fmt" @@ -121,41 +122,66 @@ func handleTarObject(ctx context.Context, tr *tar.Reader, hdr *tar.Header, conf } case tar.TypeReg: compressed := strings.HasSuffix(dstPath, ".zst") - if compressed { - image = strings.TrimSuffix(dstPath, ".zst") - if _, err := os.Stat(image); err == nil { - return image, os.ErrExist - } - - cmd := exec.CommandContext(ctx, "zstd", "-d", "-", "-o", image) - cmd.Stdin = tr - if _, err := cmd.Output(); err != nil { - var e *exec.ExitError - if errors.As(err, &e) { - fmt.Fprintf(os.Stderr, string(e.Stderr)) - } - return image, fmt.Errorf("failed during zst decompression of %s: %w", hdr.Name, err) - } + // Copy the target file out to the host (compressed or not). + // On failure, clean up the temporary file; otherwise move it + // to the target destination. + tmpFile, err := os.CreateTemp("", filepath.Base(hdr.Name)) + if err != nil { + return image, fmt.Errorf("failed to open temporary file for %s: %w", hdr.Name, err) } - if conf.Cache || !compressed { - dst, err := os.Create(dstPath) - if err != nil { - return image, fmt.Errorf("failed to create file %s: %w", dstPath, err) + defer func() { + tmpPath := tmpFile.Name() + tmpFile.Close() + if err != nil || (!conf.Cache && compressed) { + os.Remove(tmpPath) + return } - defer dst.Close() - - n, err := io.CopyN(dst, tr, hdr.Size) - if err != nil { - return image, fmt.Errorf("failed to copy %s from container %s: %w", dstPath, containerID, err) + if err = os.Rename(tmpFile.Name(), dstPath); err != nil { + fmt.Fprintf(os.Stderr, "Failed to move %s to %s: %s", tmpPath, dstPath, err) } - if n != hdr.Size { - return image, fmt.Errorf("tar header reports file %s size %d, but only %d bytes were pulled", hdr.Name, hdr.Size, n) + }() + + n, err := io.CopyN(tmpFile, tr, hdr.Size) + if err != nil { + return image, fmt.Errorf("failed to copy %s from container %s: %w", dstPath, containerID, err) + } + if n != hdr.Size { + return image, fmt.Errorf("tar header reports file %s size %d, but only %d bytes were pulled", hdr.Name, hdr.Size, n) + } + + if compressed { + if _, err = tmpFile.Seek(0, 0); err != nil { + return image, fmt.Errorf("cannot seek to the start of the compressed target file %s: %w", dstPath, err) } + compressedTarget := bufio.NewReader(tmpFile) + dstImagePath := strings.TrimSuffix(dstPath, ".zst") + + return dstImagePath, extractZst(ctx, compressedTarget, dstImagePath) } + default: return image, fmt.Errorf("unexpected tar header type %d", hdr.Typeflag) } return image, nil } + +func extractZst(ctx context.Context, reader io.Reader, dstPath string) error { + if _, err := os.Stat(dstPath); err == nil { + return os.ErrExist + } + + cmd := exec.CommandContext(ctx, "zstd", "-d", "-", "-o", dstPath) + cmd.Stdin = reader + + if _, err := cmd.Output(); err != nil { + var e *exec.ExitError + if errors.As(err, &e) { + os.Stderr.Write(e.Stderr) + } + return fmt.Errorf("failed during zst decompression to %s: %w", dstPath, err) + } + + return nil +}