Browse code

Make TarStream return an io.ReadCloser

Currently, the resources associated with the io.Reader returned by
TarStream are only freed when it is read until EOF. This means that
partial uploads or exports (for example, in the case of a full disk or
severed connection) can leak a goroutine and open file. This commit
changes TarStream to return an io.ReadCloser. Resources are freed when
Close is called.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Aaron Lehmann authored on 2015/11/26 09:39:54
Showing 11 changed files
... ...
@@ -155,6 +155,7 @@ func (daemon *Daemon) exportContainerRw(container *Container) (archive.Archive,
155 155
 		return nil, err
156 156
 	}
157 157
 	return ioutils.NewReadCloserWrapper(archive, func() error {
158
+			archive.Close()
158 159
 			return daemon.layerStore.Unmount(container.ID)
159 160
 		}),
160 161
 		nil
... ...
@@ -429,6 +429,7 @@ func (p *v1Pusher) pushImage(v1Image v1Image, ep string) (checksum string, err e
429 429
 	if err != nil {
430 430
 		return "", err
431 431
 	}
432
+	defer arch.Close()
432 433
 
433 434
 	// don't care if this fails; best effort
434 435
 	size, _ := l.Size()
... ...
@@ -365,6 +365,7 @@ func (p *v2Pusher) pushV2Layer(bs distribution.BlobService, l layer.Layer) (dige
365 365
 	if err != nil {
366 366
 		return "", err
367 367
 	}
368
+	defer arch.Close()
368 369
 
369 370
 	// Send the layer
370 371
 	layerUpload, err := bs.Create(context.Background())
... ...
@@ -287,6 +287,8 @@ func (s *saveSession) saveLayer(id layer.ChainID, legacyImg image.V1Image, creat
287 287
 	if err != nil {
288 288
 		return err
289 289
 	}
290
+	defer arch.Close()
291
+
290 292
 	if _, err := io.Copy(tarFile, arch); err != nil {
291 293
 		return err
292 294
 	}
... ...
@@ -4,6 +4,7 @@ import (
4 4
 	"archive/tar"
5 5
 	"bytes"
6 6
 	"io"
7
+	"io/ioutil"
7 8
 )
8 9
 
9 10
 // DigestSHA256EmptyTar is the canonical sha256 digest of empty tar file -
... ...
@@ -15,11 +16,11 @@ type emptyLayer struct{}
15 15
 // EmptyLayer is a layer that corresponds to empty tar.
16 16
 var EmptyLayer = &emptyLayer{}
17 17
 
18
-func (el *emptyLayer) TarStream() (io.Reader, error) {
18
+func (el *emptyLayer) TarStream() (io.ReadCloser, error) {
19 19
 	buf := new(bytes.Buffer)
20 20
 	tarWriter := tar.NewWriter(buf)
21 21
 	tarWriter.Close()
22
-	return buf, nil
22
+	return ioutil.NopCloser(buf), nil
23 23
 }
24 24
 
25 25
 func (el *emptyLayer) ChainID() ChainID {
... ...
@@ -67,7 +67,7 @@ func (diffID DiffID) String() string {
67 67
 type TarStreamer interface {
68 68
 	// TarStream returns a tar archive stream
69 69
 	// for the contents of a layer.
70
-	TarStream() (io.Reader, error)
70
+	TarStream() (io.ReadCloser, error)
71 71
 }
72 72
 
73 73
 // Layer represents a read only layer
... ...
@@ -581,7 +581,7 @@ func (ls *layerStore) Changes(name string) ([]archive.Change, error) {
581 581
 	return ls.driver.Changes(m.mountID, pid)
582 582
 }
583 583
 
584
-func (ls *layerStore) assembleTar(graphID string, metadata io.ReadCloser, size *int64) (io.Reader, error) {
584
+func (ls *layerStore) assembleTar(graphID string, metadata io.ReadCloser, size *int64) (io.ReadCloser, error) {
585 585
 	type diffPathDriver interface {
586 586
 		DiffPath(string) (string, func() error, error)
587 587
 	}
... ...
@@ -100,6 +100,7 @@ func createLayer(ls Store, parent ChainID, layerFunc layerInit) (Layer, error) {
100 100
 	if err != nil {
101 101
 		return nil, err
102 102
 	}
103
+	defer ts.Close()
103 104
 
104 105
 	layer, err := ls.Register(ts, parent)
105 106
 	if err != nil {
... ...
@@ -521,6 +522,7 @@ func assertLayerDiff(t *testing.T, expected []byte, layer Layer) {
521 521
 	if err != nil {
522 522
 		t.Fatal(err)
523 523
 	}
524
+	defer ts.Close()
524 525
 
525 526
 	actual, err := ioutil.ReadAll(ts)
526 527
 	if err != nil {
... ...
@@ -22,12 +22,12 @@ func (ml *mountedLayer) cacheParent() string {
22 22
 	return ""
23 23
 }
24 24
 
25
-func (ml *mountedLayer) TarStream() (io.Reader, error) {
25
+func (ml *mountedLayer) TarStream() (io.ReadCloser, error) {
26 26
 	archiver, err := ml.layerStore.driver.Diff(ml.mountID, ml.cacheParent())
27 27
 	if err != nil {
28 28
 		return nil, err
29 29
 	}
30
-	return autoClosingReader{archiver}, nil
30
+	return archiver, nil
31 31
 }
32 32
 
33 33
 func (ml *mountedLayer) Path() (string, error) {
... ...
@@ -50,15 +50,3 @@ func (ml *mountedLayer) Parent() Layer {
50 50
 func (ml *mountedLayer) Size() (int64, error) {
51 51
 	return ml.layerStore.driver.DiffSize(ml.mountID, ml.cacheParent())
52 52
 }
53
-
54
-type autoClosingReader struct {
55
-	source io.ReadCloser
56
-}
57
-
58
-func (r autoClosingReader) Read(p []byte) (n int, err error) {
59
-	n, err = r.source.Read(p)
60
-	if err != nil {
61
-		r.source.Close()
62
-	}
63
-	return
64
-}
... ...
@@ -14,7 +14,7 @@ type roLayer struct {
14 14
 	references     map[Layer]struct{}
15 15
 }
16 16
 
17
-func (rl *roLayer) TarStream() (io.Reader, error) {
17
+func (rl *roLayer) TarStream() (io.ReadCloser, error) {
18 18
 	r, err := rl.layerStore.store.TarSplitReader(rl.chainID)
19 19
 	if err != nil {
20 20
 		return nil, err
... ...
@@ -357,7 +357,7 @@ type mockLayer struct {
357 357
 	parent  *mockLayer
358 358
 }
359 359
 
360
-func (l *mockLayer) TarStream() (io.Reader, error) {
360
+func (l *mockLayer) TarStream() (io.ReadCloser, error) {
361 361
 	return nil, nil
362 362
 }
363 363