Merge pull request #36338 from tonistiigi/fix-copy-leak

builder: fix layer lifecycle leak

builder/builder.go

@@ -53,7 +53,7 @@ type Backend interface {
 
 // ImageBackend are the interface methods required from an image component
 type ImageBackend interface {
-	GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (Image, ReleaseableLayer, error)
+	GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (Image, ROLayer, error)
 }
 
 // ExecBackend contains the interface methods required for executing containers
@@ -100,10 +100,16 @@ type Image interface {
 	OperatingSystem() string
 }
 
-// ReleaseableLayer is an image layer that can be mounted and released
-type ReleaseableLayer interface {
+// ROLayer is a reference to image rootfs layer
+type ROLayer interface {
 	Release() error
-	Mount() (containerfs.ContainerFS, error)
-	Commit() (ReleaseableLayer, error)
+	NewRWLayer() (RWLayer, error)
 	DiffID() layer.DiffID
 }
+
+// RWLayer is active layer that can be read/modified
+type RWLayer interface {
+	Release() error
+	Root() containerfs.ContainerFS
+	Commit() (ROLayer, error)
+}

builder/dockerfile/copy.go

@@ -72,8 +72,12 @@ type copier struct {
 	source      builder.Source
 	pathCache   pathCache
 	download    sourceDownloader
-	tmpPaths    []string
 	platform    string
+	// for cleanup. TODO: having copier.cleanup() is error prone and hard to
+	// follow. Code calling performCopy should manage the lifecycle of its params.
+	// Copier should take override source as input, not imageMount.
+	activeLayer builder.RWLayer
+	tmpPaths    []string
 }
 
 func copierFromDispatchRequest(req dispatchRequest, download sourceDownloader, imageSource *imageMount) copier {
@@ -155,6 +159,10 @@ func (o *copier) Cleanup() {
 		os.RemoveAll(path)
 	}
 	o.tmpPaths = []string{}
+	if o.activeLayer != nil {
+		o.activeLayer.Release()
+		o.activeLayer = nil
+	}
 }
 
 // TODO: allowWildcards can probably be removed by refactoring this function further.
@@ -166,9 +174,15 @@ func (o *copier) calcCopyInfo(origPath string, allowWildcards bool) ([]copyInfo,
 	// done on image Source?
 	if imageSource != nil {
 		var err error
-		o.source, err = imageSource.Source()
+		rwLayer, err := imageSource.NewRWLayer()
+		if err != nil {
+			return nil, err
+		}
+		o.activeLayer = rwLayer
+
+		o.source, err = remotecontext.NewLazySource(rwLayer.Root())
 		if err != nil {
-			return nil, errors.Wrapf(err, "failed to copy from %s", imageSource.ImageID())
+			return nil, errors.Wrapf(err, "failed to create context for copy from %s", rwLayer.Root().Path())
 		}
 	}
 

builder/dockerfile/dispatchers_test.go

@@ -127,7 +127,7 @@ func TestFromScratch(t *testing.T) {
 func TestFromWithArg(t *testing.T) {
 	tag, expected := ":sometag", "expectedthisid"
 
-	getImage := func(name string) (builder.Image, builder.ReleaseableLayer, error) {
+	getImage := func(name string) (builder.Image, builder.ROLayer, error) {
 		assert.Equal(t, "alpine"+tag, name)
 		return &mockImage{id: "expectedthisid"}, nil, nil
 	}
@@ -159,7 +159,7 @@ func TestFromWithArg(t *testing.T) {
 func TestFromWithUndefinedArg(t *testing.T) {
 	tag, expected := "sometag", "expectedthisid"
 
-	getImage := func(name string) (builder.Image, builder.ReleaseableLayer, error) {
+	getImage := func(name string) (builder.Image, builder.ROLayer, error) {
 		assert.Equal(t, "alpine", name)
 		return &mockImage{id: "expectedthisid"}, nil, nil
 	}
@@ -433,7 +433,7 @@ func TestRunWithBuildArgs(t *testing.T) {
 		return imageCache
 	}
 	b.imageProber = newImageProber(mockBackend, nil, false)
-	mockBackend.getImageFunc = func(_ string) (builder.Image, builder.ReleaseableLayer, error) {
+	mockBackend.getImageFunc = func(_ string) (builder.Image, builder.ROLayer, error) {
 		return &mockImage{
 			id:     "abcdef",
 			config: &container.Config{Cmd: origCmd},

builder/dockerfile/imagecontext.go

@@ -5,7 +5,6 @@ import (
 
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/builder"
-	"github.com/docker/docker/builder/remotecontext"
 	dockerimage "github.com/docker/docker/image"
 	"github.com/docker/docker/pkg/system"
 	"github.com/pkg/errors"
@@ -13,7 +12,7 @@ import (
 	"golang.org/x/net/context"
 )
 
-type getAndMountFunc func(string, bool) (builder.Image, builder.ReleaseableLayer, error)
+type getAndMountFunc func(string, bool) (builder.Image, builder.ROLayer, error)
 
 // imageSources mounts images and provides a cache for mounted images. It tracks
 // all images so they can be unmounted at the end of the build.
@@ -24,7 +23,7 @@ type imageSources struct {
 }
 
 func newImageSources(ctx context.Context, options builderOptions) *imageSources {
-	getAndMount := func(idOrRef string, localOnly bool) (builder.Image, builder.ReleaseableLayer, error) {
+	getAndMount := func(idOrRef string, localOnly bool) (builder.Image, builder.ROLayer, error) {
 		pullOption := backend.PullOptionNoPull
 		if !localOnly {
 			if options.Options.PullParent {
@@ -92,32 +91,14 @@ func (m *imageSources) Add(im *imageMount) {
 type imageMount struct {
 	image  builder.Image
 	source builder.Source
-	layer  builder.ReleaseableLayer
+	layer  builder.ROLayer
 }
 
-func newImageMount(image builder.Image, layer builder.ReleaseableLayer) *imageMount {
+func newImageMount(image builder.Image, layer builder.ROLayer) *imageMount {
 	im := &imageMount{image: image, layer: layer}
 	return im
 }
 
-func (im *imageMount) Source() (builder.Source, error) {
-	if im.source == nil {
-		if im.layer == nil {
-			return nil, errors.Errorf("empty context")
-		}
-		mountPath, err := im.layer.Mount()
-		if err != nil {
-			return nil, errors.Wrapf(err, "failed to mount %s", im.image.ImageID())
-		}
-		source, err := remotecontext.NewLazySource(mountPath)
-		if err != nil {
-			return nil, errors.Wrapf(err, "failed to create lazycontext for %s", mountPath)
-		}
-		im.source = source
-	}
-	return im.source, nil
-}
-
 func (im *imageMount) unmount() error {
 	if im.layer == nil {
 		return nil
@@ -133,8 +114,8 @@ func (im *imageMount) Image() builder.Image {
 	return im.image
 }
 
-func (im *imageMount) Layer() builder.ReleaseableLayer {
-	return im.layer
+func (im *imageMount) NewRWLayer() (builder.RWLayer, error) {
+	return im.layer.NewRWLayer()
 }
 
 func (im *imageMount) ImageID() string {

builder/dockerfile/internals.go

@@ -17,6 +17,7 @@ import (
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/api/types/container"
+	"github.com/docker/docker/builder"
 	"github.com/docker/docker/image"
 	"github.com/docker/docker/pkg/archive"
 	"github.com/docker/docker/pkg/chrootarchive"
@@ -114,8 +115,8 @@ func (b *Builder) commitContainer(dispatchState *dispatchState, id string, conta
 	return err
 }
 
-func (b *Builder) exportImage(state *dispatchState, imageMount *imageMount, runConfig *container.Config) error {
-	newLayer, err := imageMount.Layer().Commit()
+func (b *Builder) exportImage(state *dispatchState, layer builder.RWLayer, parent builder.Image, runConfig *container.Config) error {
+	newLayer, err := layer.Commit()
 	if err != nil {
 		return err
 	}
@@ -124,7 +125,7 @@ func (b *Builder) exportImage(state *dispatchState, imageMount *imageMount, runC
 	// if there is an error before we can add the full mount with image
 	b.imageSources.Add(newImageMount(nil, newLayer))
 
-	parentImage, ok := imageMount.Image().(*image.Image)
+	parentImage, ok := parent.(*image.Image)
 	if !ok {
 		return errors.Errorf("unexpected image type")
 	}
@@ -177,7 +178,13 @@ func (b *Builder) performCopy(state *dispatchState, inst copyInstruction) error
 		return errors.Wrapf(err, "failed to get destination image %q", state.imageID)
 	}
 
-	destInfo, err := createDestInfo(state.runConfig.WorkingDir, inst, imageMount, b.options.Platform)
+	rwLayer, err := imageMount.NewRWLayer()
+	if err != nil {
+		return err
+	}
+	defer rwLayer.Release()
+
+	destInfo, err := createDestInfo(state.runConfig.WorkingDir, inst, rwLayer, b.options.Platform)
 	if err != nil {
 		return err
 	}
@@ -203,10 +210,10 @@ func (b *Builder) performCopy(state *dispatchState, inst copyInstruction) error
 			return errors.Wrapf(err, "failed to copy files")
 		}
 	}
-	return b.exportImage(state, imageMount, runConfigWithCommentCmd)
+	return b.exportImage(state, rwLayer, imageMount.Image(), runConfigWithCommentCmd)
 }
 
-func createDestInfo(workingDir string, inst copyInstruction, imageMount *imageMount, platform string) (copyInfo, error) {
+func createDestInfo(workingDir string, inst copyInstruction, rwLayer builder.RWLayer, platform string) (copyInfo, error) {
 	// Twiddle the destination when it's a relative path - meaning, make it
 	// relative to the WORKINGDIR
 	dest, err := normalizeDest(workingDir, inst.dest, platform)
@@ -214,12 +221,7 @@ func createDestInfo(workingDir string, inst copyInstruction, imageMount *imageMo
 		return copyInfo{}, errors.Wrapf(err, "invalid %s", inst.cmdName)
 	}
 
-	destMount, err := imageMount.Source()
-	if err != nil {
-		return copyInfo{}, errors.Wrapf(err, "failed to mount copy source")
-	}
-
-	return newCopyInfoFromSource(destMount, dest, ""), nil
+	return copyInfo{root: rwLayer.Root(), path: dest}, nil
 }
 
 // normalizeDest normalises the destination of a COPY/ADD command in a

builder/dockerfile/mockbackend_test.go

@@ -20,7 +20,7 @@ import (
 type MockBackend struct {
 	containerCreateFunc func(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error)
 	commitFunc          func(backend.CommitConfig) (image.ID, error)
-	getImageFunc        func(string) (builder.Image, builder.ReleaseableLayer, error)
+	getImageFunc        func(string) (builder.Image, builder.ROLayer, error)
 	makeImageCacheFunc  func(cacheFrom []string) builder.ImageCache
 }
 
@@ -66,7 +66,7 @@ func (m *MockBackend) CopyOnBuild(containerID string, destPath string, srcRoot s
 	return nil
 }
 
-func (m *MockBackend) GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ReleaseableLayer, error) {
+func (m *MockBackend) GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ROLayer, error) {
 	if m.getImageFunc != nil {
 		return m.getImageFunc(refOrID)
 	}
@@ -124,14 +124,25 @@ func (l *mockLayer) Release() error {
 	return nil
 }
 
-func (l *mockLayer) Mount() (containerfs.ContainerFS, error) {
-	return containerfs.NewLocalContainerFS("mountPath"), nil
+func (l *mockLayer) NewRWLayer() (builder.RWLayer, error) {
+	return &mockRWLayer{}, nil
 }
 
-func (l *mockLayer) Commit() (builder.ReleaseableLayer, error) {
+func (l *mockLayer) DiffID() layer.DiffID {
+	return layer.DiffID("abcdef")
+}
+
+type mockRWLayer struct {
+}
+
+func (l *mockRWLayer) Release() error {
+	return nil
+}
+
+func (l *mockRWLayer) Commit() (builder.ROLayer, error) {
 	return nil, nil
 }
 
-func (l *mockLayer) DiffID() layer.DiffID {
-	return layer.DiffID("abcdef")
+func (l *mockRWLayer) Root() containerfs.ContainerFS {
+	return nil
 }

daemon/build.go

@@ -15,130 +15,126 @@ import (
 	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/registry"
 	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
 	"golang.org/x/net/context"
 )
 
-type releaseableLayer struct {
+type roLayer struct {
 	released   bool
 	layerStore layer.Store
 	roLayer    layer.Layer
-	rwLayer    layer.RWLayer
 }
 
-func (rl *releaseableLayer) Mount() (containerfs.ContainerFS, error) {
-	var err error
-	var mountPath containerfs.ContainerFS
-	var chainID layer.ChainID
-	if rl.roLayer != nil {
-		chainID = rl.roLayer.ChainID()
+func (l *roLayer) DiffID() layer.DiffID {
+	if l.roLayer == nil {
+		return layer.DigestSHA256EmptyTar
 	}
+	return l.roLayer.DiffID()
+}
 
-	mountID := stringid.GenerateRandomID()
-	rl.rwLayer, err = rl.layerStore.CreateRWLayer(mountID, chainID, nil)
-	if err != nil {
-		return nil, errors.Wrap(err, "failed to create rwlayer")
+func (l *roLayer) Release() error {
+	if l.released {
+		return nil
 	}
-
-	mountPath, err = rl.rwLayer.Mount("")
-	if err != nil {
-		// Clean up the layer if we fail to mount it here.
-		metadata, err := rl.layerStore.ReleaseRWLayer(rl.rwLayer)
+	if l.roLayer != nil {
+		metadata, err := l.layerStore.Release(l.roLayer)
 		layer.LogReleaseMetadata(metadata)
 		if err != nil {
-			logrus.Errorf("Failed to release RWLayer: %s", err)
+			return errors.Wrap(err, "failed to release ROLayer")
 		}
-		rl.rwLayer = nil
-		return nil, err
 	}
-
-	return mountPath, nil
+	l.roLayer = nil
+	l.released = true
+	return nil
 }
 
-func (rl *releaseableLayer) Commit() (builder.ReleaseableLayer, error) {
+func (l *roLayer) NewRWLayer() (builder.RWLayer, error) {
 	var chainID layer.ChainID
-	if rl.roLayer != nil {
-		chainID = rl.roLayer.ChainID()
+	if l.roLayer != nil {
+		chainID = l.roLayer.ChainID()
 	}
 
-	stream, err := rl.rwLayer.TarStream()
+	mountID := stringid.GenerateRandomID()
+	newLayer, err := l.layerStore.CreateRWLayer(mountID, chainID, nil)
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrap(err, "failed to create rwlayer")
 	}
-	defer stream.Close()
 
-	newLayer, err := rl.layerStore.Register(stream, chainID)
+	rwLayer := &rwLayer{layerStore: l.layerStore, rwLayer: newLayer}
+
+	fs, err := newLayer.Mount("")
 	if err != nil {
+		rwLayer.Release()
 		return nil, err
 	}
-	// TODO: An optimization would be to handle empty layers before returning
-	return &releaseableLayer{layerStore: rl.layerStore, roLayer: newLayer}, nil
+
+	rwLayer.fs = fs
+
+	return rwLayer, nil
 }
 
-func (rl *releaseableLayer) DiffID() layer.DiffID {
-	if rl.roLayer == nil {
-		return layer.DigestSHA256EmptyTar
-	}
-	return rl.roLayer.DiffID()
+type rwLayer struct {
+	released   bool
+	layerStore layer.Store
+	rwLayer    layer.RWLayer
+	fs         containerfs.ContainerFS
 }
 
-func (rl *releaseableLayer) Release() error {
-	if rl.released {
-		return nil
-	}
-	if err := rl.releaseRWLayer(); err != nil {
-		// Best effort attempt at releasing read-only layer before returning original error.
-		rl.releaseROLayer()
-		return err
-	}
-	if err := rl.releaseROLayer(); err != nil {
-		return err
-	}
-	rl.released = true
-	return nil
+func (l *rwLayer) Root() containerfs.ContainerFS {
+	return l.fs
 }
 
-func (rl *releaseableLayer) releaseRWLayer() error {
-	if rl.rwLayer == nil {
-		return nil
+func (l *rwLayer) Commit() (builder.ROLayer, error) {
+	stream, err := l.rwLayer.TarStream()
+	if err != nil {
+		return nil, err
 	}
-	if err := rl.rwLayer.Unmount(); err != nil {
-		logrus.Errorf("Failed to unmount RWLayer: %s", err)
-		return err
+	defer stream.Close()
+
+	var chainID layer.ChainID
+	if parent := l.rwLayer.Parent(); parent != nil {
+		chainID = parent.ChainID()
 	}
-	metadata, err := rl.layerStore.ReleaseRWLayer(rl.rwLayer)
-	layer.LogReleaseMetadata(metadata)
+
+	newLayer, err := l.layerStore.Register(stream, chainID)
 	if err != nil {
-		logrus.Errorf("Failed to release RWLayer: %s", err)
+		return nil, err
 	}
-	rl.rwLayer = nil
-	return err
+	// TODO: An optimization would be to handle empty layers before returning
+	return &roLayer{layerStore: l.layerStore, roLayer: newLayer}, nil
 }
 
-func (rl *releaseableLayer) releaseROLayer() error {
-	if rl.roLayer == nil {
+func (l *rwLayer) Release() error {
+	if l.released {
 		return nil
 	}
-	metadata, err := rl.layerStore.Release(rl.roLayer)
+
+	if l.fs != nil {
+		if err := l.rwLayer.Unmount(); err != nil {
+			return errors.Wrap(err, "failed to unmount RWLayer")
+		}
+		l.fs = nil
+	}
+
+	metadata, err := l.layerStore.ReleaseRWLayer(l.rwLayer)
 	layer.LogReleaseMetadata(metadata)
 	if err != nil {
-		logrus.Errorf("Failed to release ROLayer: %s", err)
+		return errors.Wrap(err, "failed to release RWLayer")
 	}
-	rl.roLayer = nil
-	return err
+	l.released = true
+	return nil
 }
 
-func newReleasableLayerForImage(img *image.Image, layerStore layer.Store) (builder.ReleaseableLayer, error) {
+func newROLayerForImage(img *image.Image, layerStore layer.Store) (builder.ROLayer, error) {
 	if img == nil || img.RootFS.ChainID() == "" {
-		return &releaseableLayer{layerStore: layerStore}, nil
+		return &roLayer{layerStore: layerStore}, nil
 	}
 	// Hold a reference to the image layer so that it can't be removed before
 	// it is released
-	roLayer, err := layerStore.Get(img.RootFS.ChainID())
+	layer, err := layerStore.Get(img.RootFS.ChainID())
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to get layer for image %s", img.ImageID())
 	}
-	return &releaseableLayer{layerStore: layerStore, roLayer: roLayer}, nil
+	return &roLayer{layerStore: layerStore, roLayer: layer}, nil
 }
 
 // TODO: could this use the regular daemon PullImage ?
@@ -170,12 +166,12 @@ func (daemon *Daemon) pullForBuilder(ctx context.Context, name string, authConfi
 // GetImageAndReleasableLayer returns an image and releaseable layer for a reference or ID.
 // Every call to GetImageAndReleasableLayer MUST call releasableLayer.Release() to prevent
 // leaking of layers.
-func (daemon *Daemon) GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ReleaseableLayer, error) {
+func (daemon *Daemon) GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ROLayer, error) {
 	if refOrID == "" {
 		if !system.IsOSSupported(opts.OS) {
 			return nil, nil, system.ErrNotSupportedOperatingSystem
 		}
-		layer, err := newReleasableLayerForImage(nil, daemon.layerStores[opts.OS])
+		layer, err := newROLayerForImage(nil, daemon.layerStores[opts.OS])
 		return nil, layer, err
 	}
 
@@ -189,7 +185,7 @@ func (daemon *Daemon) GetImageAndReleasableLayer(ctx context.Context, refOrID st
 			if !system.IsOSSupported(image.OperatingSystem()) {
 				return nil, nil, system.ErrNotSupportedOperatingSystem
 			}
-			layer, err := newReleasableLayerForImage(image, daemon.layerStores[image.OperatingSystem()])
+			layer, err := newROLayerForImage(image, daemon.layerStores[image.OperatingSystem()])
 			return image, layer, err
 		}
 	}
@@ -201,7 +197,7 @@ func (daemon *Daemon) GetImageAndReleasableLayer(ctx context.Context, refOrID st
 	if !system.IsOSSupported(image.OperatingSystem()) {
 		return nil, nil, system.ErrNotSupportedOperatingSystem
 	}
-	layer, err := newReleasableLayerForImage(image, daemon.layerStores[image.OperatingSystem()])
+	layer, err := newROLayerForImage(image, daemon.layerStores[image.OperatingSystem()])
 	return image, layer, err
 }
 

integration/build/build_test.go

@@ -301,6 +301,46 @@ COPY bar /`
 	require.NotContains(t, out.String(), "Using cache")
 }
 
+// docker/for-linux#135
+// #35641
+func TestBuildMultiStageLayerLeak(t *testing.T) {
+	ctx := context.TODO()
+	defer setupTest(t)()
+
+	// all commands need to match until COPY
+	dockerfile := `FROM busybox
+WORKDIR /foo
+COPY foo .
+FROM busybox
+WORKDIR /foo
+COPY bar .
+RUN [ -f bar ]
+RUN [ ! -f foo ]
+`
+
+	source := fakecontext.New(t, "",
+		fakecontext.WithFile("foo", "0"),
+		fakecontext.WithFile("bar", "1"),
+		fakecontext.WithDockerfile(dockerfile))
+	defer source.Close()
+
+	apiclient := testEnv.APIClient()
+	resp, err := apiclient.ImageBuild(ctx,
+		source.AsTarReader(t),
+		types.ImageBuildOptions{
+			Remove:      true,
+			ForceRemove: true,
+		})
+
+	out := bytes.NewBuffer(nil)
+	require.NoError(t, err)
+	_, err = io.Copy(out, resp.Body)
+	resp.Body.Close()
+	require.NoError(t, err)
+
+	assert.Contains(t, out.String(), "Successfully built")
+}
+
 func writeTarRecord(t *testing.T, w *tar.Writer, fn, contents string) {
 	err := w.WriteHeader(&tar.Header{
 		Name:     fn,