Browse code

Ensure adding a broken tar doesn't silently fail

Signed-off-by: Aidan Hobson Sayers <aidanhs@cantab.net>

Aidan Hobson Sayers authored on 2015/11/21 01:49:33
Showing 5 changed files
... ...
@@ -38,10 +38,6 @@ import (
38 38
 	"github.com/docker/docker/utils"
39 39
 )
40 40
 
41
-const (
42
-	tarHeaderSize = 512
43
-)
44
-
45 41
 // CmdBuild builds a new image from the source code at a given path.
46 42
 //
47 43
 // 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 {
449 449
 func getContextFromReader(r io.Reader, dockerfileName string) (absContextDir, relDockerfile string, err error) {
450 450
 	buf := bufio.NewReader(r)
451 451
 
452
-	magic, err := buf.Peek(tarHeaderSize)
452
+	magic, err := buf.Peek(archive.HeaderSize)
453 453
 	if err != nil && err != io.EOF {
454 454
 		return "", "", fmt.Errorf("failed to peek context header from STDIN: %v", err)
455 455
 	}
... ...
@@ -161,7 +161,7 @@ func (d Docker) Copy(c *daemon.Container, destPath string, src builder.FileInfo,
161 161
 		}
162 162
 		return fixPermissions(srcPath, destPath, rootUID, rootGID, destExists)
163 163
 	}
164
-	if decompress {
164
+	if decompress && archive.IsArchivePath(srcPath) {
165 165
 		// Only try to untar if it is a file and that we've been told to decompress (when ADD-ing a remote file)
166 166
 
167 167
 		// 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,
174 174
 		}
175 175
 
176 176
 		// try to successfully untar the orig
177
-		if err := d.Archiver.UntarPath(srcPath, tarDest); err == nil {
178
-			return nil
179
-		} else if err != io.EOF {
180
-			logrus.Debugf("Couldn't untar to %s: %v", tarDest, err)
177
+		err := d.Archiver.UntarPath(srcPath, tarDest)
178
+		if err != nil {
179
+			logrus.Errorf("Couldn't untar to %s: %v", tarDest, err)
181 180
 		}
181
+		return err
182 182
 	}
183 183
 
184 184
 	// only needed for fixPermissions, but might as well put it before CopyFileWithTar
... ...
@@ -4217,6 +4217,79 @@ RUN cat /existing-directory-trailing-slash/test/foo | grep Hi`
4217 4217
 
4218 4218
 }
4219 4219
 
4220
+func (s *DockerSuite) TestBuildAddBrokenTar(c *check.C) {
4221
+	testRequires(c, DaemonIsLinux)
4222
+	name := "testbuildaddbrokentar"
4223
+
4224
+	ctx := func() *FakeContext {
4225
+		dockerfile := `
4226
+FROM busybox
4227
+ADD test.tar /`
4228
+		tmpDir, err := ioutil.TempDir("", "fake-context")
4229
+		c.Assert(err, check.IsNil)
4230
+		testTar, err := os.Create(filepath.Join(tmpDir, "test.tar"))
4231
+		if err != nil {
4232
+			c.Fatalf("failed to create test.tar archive: %v", err)
4233
+		}
4234
+		defer testTar.Close()
4235
+
4236
+		tw := tar.NewWriter(testTar)
4237
+
4238
+		if err := tw.WriteHeader(&tar.Header{
4239
+			Name: "test/foo",
4240
+			Size: 2,
4241
+		}); err != nil {
4242
+			c.Fatalf("failed to write tar file header: %v", err)
4243
+		}
4244
+		if _, err := tw.Write([]byte("Hi")); err != nil {
4245
+			c.Fatalf("failed to write tar file content: %v", err)
4246
+		}
4247
+		if err := tw.Close(); err != nil {
4248
+			c.Fatalf("failed to close tar archive: %v", err)
4249
+		}
4250
+
4251
+		// Corrupt the tar by removing one byte off the end
4252
+		stat, err := testTar.Stat()
4253
+		if err != nil {
4254
+			c.Fatalf("failed to stat tar archive: %v", err)
4255
+		}
4256
+		if err := testTar.Truncate(stat.Size() - 1); err != nil {
4257
+			c.Fatalf("failed to truncate tar archive: %v", err)
4258
+		}
4259
+
4260
+		if err := ioutil.WriteFile(filepath.Join(tmpDir, "Dockerfile"), []byte(dockerfile), 0644); err != nil {
4261
+			c.Fatalf("failed to open destination dockerfile: %v", err)
4262
+		}
4263
+		return fakeContextFromDir(tmpDir)
4264
+	}()
4265
+	defer ctx.Close()
4266
+
4267
+	if _, err := buildImageFromContext(name, ctx, true); err == nil {
4268
+		c.Fatalf("build should have failed for TestBuildAddBrokenTar")
4269
+	}
4270
+}
4271
+
4272
+func (s *DockerSuite) TestBuildAddNonTar(c *check.C) {
4273
+	testRequires(c, DaemonIsLinux)
4274
+	name := "testbuildaddnontar"
4275
+
4276
+	// Should not try to extract test.tar
4277
+	ctx, err := fakeContext(`
4278
+		FROM busybox
4279
+		ADD test.tar /
4280
+		RUN test -f /test.tar`,
4281
+		map[string]string{"test.tar": "not_a_tar_file"})
4282
+
4283
+	if err != nil {
4284
+		c.Fatal(err)
4285
+	}
4286
+	defer ctx.Close()
4287
+
4288
+	if _, err := buildImageFromContext(name, ctx, true); err != nil {
4289
+		c.Fatalf("build failed for TestBuildAddNonTar")
4290
+	}
4291
+}
4292
+
4220 4293
 func (s *DockerSuite) TestBuildAddTarXz(c *check.C) {
4221 4294
 	// /test/foo is not owned by the correct user
4222 4295
 	testRequires(c, NotUserNamespace)
... ...
@@ -78,6 +78,11 @@ var (
78 78
 )
79 79
 
80 80
 const (
81
+	// HeaderSize is the size in bytes of a tar header
82
+	HeaderSize = 512
83
+)
84
+
85
+const (
81 86
 	// Uncompressed represents the uncompressed.
82 87
 	Uncompressed Compression = iota
83 88
 	// Bzip2 is bzip2 compression algorithm.
... ...
@@ -88,7 +93,8 @@ const (
88 88
 	Xz
89 89
 )
90 90
 
91
-// IsArchive checks if it is a archive by the header.
91
+// IsArchive checks for the magic bytes of a tar or any supported compression
92
+// algorithm.
92 93
 func IsArchive(header []byte) bool {
93 94
 	compression := DetectCompression(header)
94 95
 	if compression != Uncompressed {
... ...
@@ -99,6 +105,23 @@ func IsArchive(header []byte) bool {
99 99
 	return err == nil
100 100
 }
101 101
 
102
+// IsArchivePath checks if the (possibly compressed) file at the given path
103
+// starts with a tar file header.
104
+func IsArchivePath(path string) bool {
105
+	file, err := os.Open(path)
106
+	if err != nil {
107
+		return false
108
+	}
109
+	defer file.Close()
110
+	rdr, err := DecompressStream(file)
111
+	if err != nil {
112
+		return false
113
+	}
114
+	r := tar.NewReader(rdr)
115
+	_, err = r.Next()
116
+	return err == nil
117
+}
118
+
102 119
 // DetectCompression detects the compression algorithm of the source.
103 120
 func DetectCompression(source []byte) Compression {
104 121
 	for compression, m := range map[Compression][]byte{
... ...
@@ -800,10 +823,7 @@ func (archiver *Archiver) UntarPath(src, dst string) error {
800 800
 			GIDMaps: archiver.GIDMaps,
801 801
 		}
802 802
 	}
803
-	if err := archiver.Untar(archive, dst, options); err != nil {
804
-		return err
805
-	}
806
-	return nil
803
+	return archiver.Untar(archive, dst, options)
807 804
 }
808 805
 
809 806
 // UntarPath is a convenience function which looks for an archive
... ...
@@ -49,6 +49,45 @@ func TestIsArchive7zip(t *testing.T) {
49 49
 	}
50 50
 }
51 51
 
52
+func TestIsArchivePathDir(t *testing.T) {
53
+	cmd := exec.Command("/bin/sh", "-c", "mkdir -p /tmp/archivedir")
54
+	output, err := cmd.CombinedOutput()
55
+	if err != nil {
56
+		t.Fatalf("Fail to create an archive file for test : %s.", output)
57
+	}
58
+	if IsArchivePath("/tmp/archivedir") {
59
+		t.Fatalf("Incorrectly recognised directory as an archive")
60
+	}
61
+}
62
+
63
+func TestIsArchivePathInvalidFile(t *testing.T) {
64
+	cmd := exec.Command("/bin/sh", "-c", "dd if=/dev/zero bs=1K count=1 of=/tmp/archive && gzip --stdout /tmp/archive > /tmp/archive.gz")
65
+	output, err := cmd.CombinedOutput()
66
+	if err != nil {
67
+		t.Fatalf("Fail to create an archive file for test : %s.", output)
68
+	}
69
+	if IsArchivePath("/tmp/archive") {
70
+		t.Fatalf("Incorrectly recognised invalid tar path as archive")
71
+	}
72
+	if IsArchivePath("/tmp/archive.gz") {
73
+		t.Fatalf("Incorrectly recognised invalid compressed tar path as archive")
74
+	}
75
+}
76
+
77
+func TestIsArchivePathTar(t *testing.T) {
78
+	cmd := exec.Command("/bin/sh", "-c", "touch /tmp/archivedata && tar -cf /tmp/archive /tmp/archivedata && gzip --stdout /tmp/archive > /tmp/archive.gz")
79
+	output, err := cmd.CombinedOutput()
80
+	if err != nil {
81
+		t.Fatalf("Fail to create an archive file for test : %s.", output)
82
+	}
83
+	if !IsArchivePath("/tmp/archive") {
84
+		t.Fatalf("Did not recognise valid tar path as archive")
85
+	}
86
+	if !IsArchivePath("/tmp/archive.gz") {
87
+		t.Fatalf("Did not recognise valid compressed tar path as archive")
88
+	}
89
+}
90
+
52 91
 func TestDecompressStreamGzip(t *testing.T) {
53 92
 	cmd := exec.Command("/bin/sh", "-c", "touch /tmp/archive && gzip -f /tmp/archive")
54 93
 	output, err := cmd.CombinedOutput()