From 21278efaee563b356851a530b08b0537fee095d7 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Wed, 25 Nov 2015 16:39:54 -0800 Subject: [PATCH] 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 --- daemon/commit.go | 1 + distribution/push_v1.go | 1 + distribution/push_v2.go | 1 + image/tarexport/save.go | 2 ++ layer/empty.go | 5 +++-- layer/layer.go | 2 +- layer/layer_store.go | 2 +- layer/layer_test.go | 2 ++ layer/mounted_layer.go | 16 ++-------------- layer/ro_layer.go | 2 +- migrate/v1/migratev1_test.go | 2 +- 11 files changed, 16 insertions(+), 20 deletions(-) diff --git a/daemon/commit.go b/daemon/commit.go index 01bfa3c7ae..2ff5176d39 100644 --- a/daemon/commit.go +++ b/daemon/commit.go @@ -155,6 +155,7 @@ func (daemon *Daemon) exportContainerRw(container *Container) (archive.Archive, return nil, err } return ioutils.NewReadCloserWrapper(archive, func() error { + archive.Close() return daemon.layerStore.Unmount(container.ID) }), nil diff --git a/distribution/push_v1.go b/distribution/push_v1.go index f6ffbb4445..9e1a09cfbf 100644 --- a/distribution/push_v1.go +++ b/distribution/push_v1.go @@ -429,6 +429,7 @@ func (p *v1Pusher) pushImage(v1Image v1Image, ep string) (checksum string, err e if err != nil { return "", err } + defer arch.Close() // don't care if this fails; best effort size, _ := l.Size() diff --git a/distribution/push_v2.go b/distribution/push_v2.go index 15edf2e2b3..f2c23ea0ea 100644 --- a/distribution/push_v2.go +++ b/distribution/push_v2.go @@ -365,6 +365,7 @@ func (p *v2Pusher) pushV2Layer(bs distribution.BlobService, l layer.Layer) (dige if err != nil { return "", err } + defer arch.Close() // Send the layer layerUpload, err := bs.Create(context.Background()) diff --git a/image/tarexport/save.go b/image/tarexport/save.go index 1019e29244..f16a149c06 100644 --- a/image/tarexport/save.go +++ b/image/tarexport/save.go @@ -287,6 +287,8 @@ func (s *saveSession) saveLayer(id layer.ChainID, legacyImg image.V1Image, creat if err != nil { return err } + defer arch.Close() + if _, err := io.Copy(tarFile, arch); err != nil { return err } diff --git a/layer/empty.go b/layer/empty.go index 04d2a20b88..5e1cb184b6 100644 --- a/layer/empty.go +++ b/layer/empty.go @@ -4,6 +4,7 @@ import ( "archive/tar" "bytes" "io" + "io/ioutil" ) // DigestSHA256EmptyTar is the canonical sha256 digest of empty tar file - @@ -15,11 +16,11 @@ type emptyLayer struct{} // EmptyLayer is a layer that corresponds to empty tar. var EmptyLayer = &emptyLayer{} -func (el *emptyLayer) TarStream() (io.Reader, error) { +func (el *emptyLayer) TarStream() (io.ReadCloser, error) { buf := new(bytes.Buffer) tarWriter := tar.NewWriter(buf) tarWriter.Close() - return buf, nil + return ioutil.NopCloser(buf), nil } func (el *emptyLayer) ChainID() ChainID { diff --git a/layer/layer.go b/layer/layer.go index 96bf5e28c1..d4533e4f56 100644 --- a/layer/layer.go +++ b/layer/layer.go @@ -67,7 +67,7 @@ func (diffID DiffID) String() string { type TarStreamer interface { // TarStream returns a tar archive stream // for the contents of a layer. - TarStream() (io.Reader, error) + TarStream() (io.ReadCloser, error) } // Layer represents a read only layer diff --git a/layer/layer_store.go b/layer/layer_store.go index 50e5787994..701dd76244 100644 --- a/layer/layer_store.go +++ b/layer/layer_store.go @@ -581,7 +581,7 @@ func (ls *layerStore) Changes(name string) ([]archive.Change, error) { return ls.driver.Changes(m.mountID, pid) } -func (ls *layerStore) assembleTar(graphID string, metadata io.ReadCloser, size *int64) (io.Reader, error) { +func (ls *layerStore) assembleTar(graphID string, metadata io.ReadCloser, size *int64) (io.ReadCloser, error) { type diffPathDriver interface { DiffPath(string) (string, func() error, error) } diff --git a/layer/layer_test.go b/layer/layer_test.go index e08f594e01..c3e4cf1d52 100644 --- a/layer/layer_test.go +++ b/layer/layer_test.go @@ -100,6 +100,7 @@ func createLayer(ls Store, parent ChainID, layerFunc layerInit) (Layer, error) { if err != nil { return nil, err } + defer ts.Close() layer, err := ls.Register(ts, parent) if err != nil { @@ -521,6 +522,7 @@ func assertLayerDiff(t *testing.T, expected []byte, layer Layer) { if err != nil { t.Fatal(err) } + defer ts.Close() actual, err := ioutil.ReadAll(ts) if err != nil { diff --git a/layer/mounted_layer.go b/layer/mounted_layer.go index a1eaa25c4c..35f43609c6 100644 --- a/layer/mounted_layer.go +++ b/layer/mounted_layer.go @@ -22,12 +22,12 @@ func (ml *mountedLayer) cacheParent() string { return "" } -func (ml *mountedLayer) TarStream() (io.Reader, error) { +func (ml *mountedLayer) TarStream() (io.ReadCloser, error) { archiver, err := ml.layerStore.driver.Diff(ml.mountID, ml.cacheParent()) if err != nil { return nil, err } - return autoClosingReader{archiver}, nil + return archiver, nil } func (ml *mountedLayer) Path() (string, error) { @@ -50,15 +50,3 @@ func (ml *mountedLayer) Parent() Layer { func (ml *mountedLayer) Size() (int64, error) { return ml.layerStore.driver.DiffSize(ml.mountID, ml.cacheParent()) } - -type autoClosingReader struct { - source io.ReadCloser -} - -func (r autoClosingReader) Read(p []byte) (n int, err error) { - n, err = r.source.Read(p) - if err != nil { - r.source.Close() - } - return -} diff --git a/layer/ro_layer.go b/layer/ro_layer.go index 3a547ca013..93525c1bbb 100644 --- a/layer/ro_layer.go +++ b/layer/ro_layer.go @@ -14,7 +14,7 @@ type roLayer struct { references map[Layer]struct{} } -func (rl *roLayer) TarStream() (io.Reader, error) { +func (rl *roLayer) TarStream() (io.ReadCloser, error) { r, err := rl.layerStore.store.TarSplitReader(rl.chainID) if err != nil { return nil, err diff --git a/migrate/v1/migratev1_test.go b/migrate/v1/migratev1_test.go index 48d2a23411..d742db5e15 100644 --- a/migrate/v1/migratev1_test.go +++ b/migrate/v1/migratev1_test.go @@ -357,7 +357,7 @@ type mockLayer struct { parent *mockLayer } -func (l *mockLayer) TarStream() (io.Reader, error) { +func (l *mockLayer) TarStream() (io.ReadCloser, error) { return nil, nil }