Merge pull request #18123 from aidanhs/aphs-fail-on-broken-tar

Ensure adding a broken tar doesn't silently fail

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.
@@ -454,7 +450,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)
 	}

daemon/daemonbuilder/builder.go

@@ -167,7 +167,7 @@ func (d Docker) Copy(c *container.Container, destPath string, src builder.FileIn
 		}
 		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
@@ -180,11 +180,11 @@ func (d Docker) Copy(c *container.Container, destPath string, src builder.FileIn
 		}
 
 		// 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

integration-cli/docker_cli_build_test.go

@@ -4215,6 +4215,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)

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{
@@ -806,10 +829,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

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()