From 3243e504d01ea1efa141f3e3cc296903d7d62ca4 Mon Sep 17 00:00:00 2001 From: Aidan Hobson Sayers Date: Fri, 20 Nov 2015 16:49:33 +0000 Subject: [PATCH] Ensure adding a broken tar doesn't silently fail Signed-off-by: Aidan Hobson Sayers --- api/client/build.go | 6 +- daemon/daemonbuilder/builder.go | 10 ++-- integration-cli/docker_cli_build_test.go | 73 ++++++++++++++++++++++++ pkg/archive/archive.go | 30 ++++++++-- pkg/archive/archive_test.go | 39 +++++++++++++ 5 files changed, 143 insertions(+), 15 deletions(-) diff --git a/api/client/build.go b/api/client/build.go index 55efc1ba35..607778879c 100644 --- a/api/client/build.go +++ b/api/client/build.go @@ -38,10 +38,6 @@ import ( "github.com/docker/docker/utils" ) -const ( - tarHeaderSize = 512 -) - // CmdBuild builds a new image from the source code at a given path. // // If '-' is provided instead of a path or URL, Docker will build an image from either a Dockerfile or tar archive read from STDIN. @@ -449,7 +445,7 @@ func writeToFile(r io.Reader, filename string) error { func getContextFromReader(r io.Reader, dockerfileName string) (absContextDir, relDockerfile string, err error) { buf := bufio.NewReader(r) - magic, err := buf.Peek(tarHeaderSize) + magic, err := buf.Peek(archive.HeaderSize) if err != nil && err != io.EOF { return "", "", fmt.Errorf("failed to peek context header from STDIN: %v", err) } diff --git a/daemon/daemonbuilder/builder.go b/daemon/daemonbuilder/builder.go index 0b5528f16f..724ec538be 100644 --- a/daemon/daemonbuilder/builder.go +++ b/daemon/daemonbuilder/builder.go @@ -161,7 +161,7 @@ func (d Docker) Copy(c *daemon.Container, destPath string, src builder.FileInfo, } return fixPermissions(srcPath, destPath, rootUID, rootGID, destExists) } - if decompress { + if decompress && archive.IsArchivePath(srcPath) { // Only try to untar if it is a file and that we've been told to decompress (when ADD-ing a remote file) // First try to unpack the source as an archive @@ -174,11 +174,11 @@ func (d Docker) Copy(c *daemon.Container, destPath string, src builder.FileInfo, } // try to successfully untar the orig - if err := d.Archiver.UntarPath(srcPath, tarDest); err == nil { - return nil - } else if err != io.EOF { - logrus.Debugf("Couldn't untar to %s: %v", tarDest, err) + err := d.Archiver.UntarPath(srcPath, tarDest) + if err != nil { + logrus.Errorf("Couldn't untar to %s: %v", tarDest, err) } + return err } // only needed for fixPermissions, but might as well put it before CopyFileWithTar diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index faeec5465d..5bfdd8db33 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -4217,6 +4217,79 @@ RUN cat /existing-directory-trailing-slash/test/foo | grep Hi` } +func (s *DockerSuite) TestBuildAddBrokenTar(c *check.C) { + testRequires(c, DaemonIsLinux) + name := "testbuildaddbrokentar" + + ctx := func() *FakeContext { + dockerfile := ` +FROM busybox +ADD test.tar /` + tmpDir, err := ioutil.TempDir("", "fake-context") + c.Assert(err, check.IsNil) + testTar, err := os.Create(filepath.Join(tmpDir, "test.tar")) + if err != nil { + c.Fatalf("failed to create test.tar archive: %v", err) + } + defer testTar.Close() + + tw := tar.NewWriter(testTar) + + if err := tw.WriteHeader(&tar.Header{ + Name: "test/foo", + Size: 2, + }); err != nil { + c.Fatalf("failed to write tar file header: %v", err) + } + if _, err := tw.Write([]byte("Hi")); err != nil { + c.Fatalf("failed to write tar file content: %v", err) + } + if err := tw.Close(); err != nil { + c.Fatalf("failed to close tar archive: %v", err) + } + + // Corrupt the tar by removing one byte off the end + stat, err := testTar.Stat() + if err != nil { + c.Fatalf("failed to stat tar archive: %v", err) + } + if err := testTar.Truncate(stat.Size() - 1); err != nil { + c.Fatalf("failed to truncate tar archive: %v", err) + } + + if err := ioutil.WriteFile(filepath.Join(tmpDir, "Dockerfile"), []byte(dockerfile), 0644); err != nil { + c.Fatalf("failed to open destination dockerfile: %v", err) + } + return fakeContextFromDir(tmpDir) + }() + defer ctx.Close() + + if _, err := buildImageFromContext(name, ctx, true); err == nil { + c.Fatalf("build should have failed for TestBuildAddBrokenTar") + } +} + +func (s *DockerSuite) TestBuildAddNonTar(c *check.C) { + testRequires(c, DaemonIsLinux) + name := "testbuildaddnontar" + + // Should not try to extract test.tar + ctx, err := fakeContext(` + FROM busybox + ADD test.tar / + RUN test -f /test.tar`, + map[string]string{"test.tar": "not_a_tar_file"}) + + if err != nil { + c.Fatal(err) + } + defer ctx.Close() + + if _, err := buildImageFromContext(name, ctx, true); err != nil { + c.Fatalf("build failed for TestBuildAddNonTar") + } +} + func (s *DockerSuite) TestBuildAddTarXz(c *check.C) { // /test/foo is not owned by the correct user testRequires(c, NotUserNamespace) diff --git a/pkg/archive/archive.go b/pkg/archive/archive.go index 1c8e1153b3..2394e40faa 100644 --- a/pkg/archive/archive.go +++ b/pkg/archive/archive.go @@ -77,6 +77,11 @@ var ( defaultArchiver = &Archiver{Untar: Untar, UIDMaps: nil, GIDMaps: nil} ) +const ( + // HeaderSize is the size in bytes of a tar header + HeaderSize = 512 +) + const ( // Uncompressed represents the uncompressed. Uncompressed Compression = iota @@ -88,7 +93,8 @@ const ( Xz ) -// IsArchive checks if it is a archive by the header. +// IsArchive checks for the magic bytes of a tar or any supported compression +// algorithm. func IsArchive(header []byte) bool { compression := DetectCompression(header) if compression != Uncompressed { @@ -99,6 +105,23 @@ func IsArchive(header []byte) bool { return err == nil } +// IsArchivePath checks if the (possibly compressed) file at the given path +// starts with a tar file header. +func IsArchivePath(path string) bool { + file, err := os.Open(path) + if err != nil { + return false + } + defer file.Close() + rdr, err := DecompressStream(file) + if err != nil { + return false + } + r := tar.NewReader(rdr) + _, err = r.Next() + return err == nil +} + // DetectCompression detects the compression algorithm of the source. func DetectCompression(source []byte) Compression { for compression, m := range map[Compression][]byte{ @@ -800,10 +823,7 @@ func (archiver *Archiver) UntarPath(src, dst string) error { GIDMaps: archiver.GIDMaps, } } - if err := archiver.Untar(archive, dst, options); err != nil { - return err - } - return nil + return archiver.Untar(archive, dst, options) } // UntarPath is a convenience function which looks for an archive diff --git a/pkg/archive/archive_test.go b/pkg/archive/archive_test.go index 6c54c02d18..882e1cc6eb 100644 --- a/pkg/archive/archive_test.go +++ b/pkg/archive/archive_test.go @@ -49,6 +49,45 @@ func TestIsArchive7zip(t *testing.T) { } } +func TestIsArchivePathDir(t *testing.T) { + cmd := exec.Command("/bin/sh", "-c", "mkdir -p /tmp/archivedir") + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("Fail to create an archive file for test : %s.", output) + } + if IsArchivePath("/tmp/archivedir") { + t.Fatalf("Incorrectly recognised directory as an archive") + } +} + +func TestIsArchivePathInvalidFile(t *testing.T) { + cmd := exec.Command("/bin/sh", "-c", "dd if=/dev/zero bs=1K count=1 of=/tmp/archive && gzip --stdout /tmp/archive > /tmp/archive.gz") + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("Fail to create an archive file for test : %s.", output) + } + if IsArchivePath("/tmp/archive") { + t.Fatalf("Incorrectly recognised invalid tar path as archive") + } + if IsArchivePath("/tmp/archive.gz") { + t.Fatalf("Incorrectly recognised invalid compressed tar path as archive") + } +} + +func TestIsArchivePathTar(t *testing.T) { + cmd := exec.Command("/bin/sh", "-c", "touch /tmp/archivedata && tar -cf /tmp/archive /tmp/archivedata && gzip --stdout /tmp/archive > /tmp/archive.gz") + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("Fail to create an archive file for test : %s.", output) + } + if !IsArchivePath("/tmp/archive") { + t.Fatalf("Did not recognise valid tar path as archive") + } + if !IsArchivePath("/tmp/archive.gz") { + t.Fatalf("Did not recognise valid compressed tar path as archive") + } +} + func TestDecompressStreamGzip(t *testing.T) { cmd := exec.Command("/bin/sh", "-c", "touch /tmp/archive && gzip -f /tmp/archive") output, err := cmd.CombinedOutput()