From 7ad41d53df94c4277574d14809211b42dca2becc Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Fri, 16 Feb 2018 13:50:57 -0800 Subject: [PATCH] builder: fix layer lifecycle leak Signed-off-by: Tonis Tiigi --- builder/builder.go | 16 ++- builder/dockerfile/copy.go | 20 +++- builder/dockerfile/dispatchers_test.go | 6 +- builder/dockerfile/imagecontext.go | 31 +---- builder/dockerfile/internals.go | 26 +++-- builder/dockerfile/mockbackend_test.go | 27 +++-- daemon/build.go | 156 ++++++++++++------------- integration/build/build_test.go | 40 +++++++ 8 files changed, 186 insertions(+), 136 deletions(-) diff --git a/builder/builder.go b/builder/builder.go index 95320c9a6c..6da0e49e10 100644 --- a/builder/builder.go +++ b/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) +} diff --git a/builder/dockerfile/copy.go b/builder/dockerfile/copy.go index f0b27b4c68..cb9f24d205 100644 --- a/builder/dockerfile/copy.go +++ b/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, errors.Wrapf(err, "failed to copy from %s", imageSource.ImageID()) + return nil, err + } + o.activeLayer = rwLayer + + o.source, err = remotecontext.NewLazySource(rwLayer.Root()) + if err != nil { + return nil, errors.Wrapf(err, "failed to create context for copy from %s", rwLayer.Root().Path()) } } diff --git a/builder/dockerfile/dispatchers_test.go b/builder/dockerfile/dispatchers_test.go index a96d579905..6d52e7e619 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/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}, diff --git a/builder/dockerfile/imagecontext.go b/builder/dockerfile/imagecontext.go index f83528e104..0d4af384af 100644 --- a/builder/dockerfile/imagecontext.go +++ b/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 { diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 4b4f638b1f..c8b34f8f65 100644 --- a/builder/dockerfile/internals.go +++ b/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 diff --git a/builder/dockerfile/mockbackend_test.go b/builder/dockerfile/mockbackend_test.go index 06adb928fd..77bc8b5715 100644 --- a/builder/dockerfile/mockbackend_test.go +++ b/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) Commit() (builder.ReleaseableLayer, error) { - return nil, nil +func (l *mockLayer) NewRWLayer() (builder.RWLayer, error) { + return &mockRWLayer{}, nil } 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 *mockRWLayer) Root() containerfs.ContainerFS { + return nil +} diff --git a/daemon/build.go b/daemon/build.go index 0384c13f32..d1396fb50e 100644 --- a/daemon/build.go +++ b/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 +func (l *roLayer) DiffID() layer.DiffID { + if l.roLayer == nil { + return layer.DigestSHA256EmptyTar + } + return l.roLayer.DiffID() +} + +func (l *roLayer) Release() error { + if l.released { + return nil + } + if l.roLayer != nil { + metadata, err := l.layerStore.Release(l.roLayer) + layer.LogReleaseMetadata(metadata) + if err != nil { + return errors.Wrap(err, "failed to release ROLayer") + } + } + l.roLayer = nil + l.released = true + return nil +} + +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() } mountID := stringid.GenerateRandomID() - rl.rwLayer, err = rl.layerStore.CreateRWLayer(mountID, chainID, nil) + newLayer, err := l.layerStore.CreateRWLayer(mountID, chainID, nil) if err != nil { return nil, errors.Wrap(err, "failed to create rwlayer") } - mountPath, err = rl.rwLayer.Mount("") + rwLayer := &rwLayer{layerStore: l.layerStore, rwLayer: newLayer} + + fs, err := newLayer.Mount("") if err != nil { - // Clean up the layer if we fail to mount it here. - metadata, err := rl.layerStore.ReleaseRWLayer(rl.rwLayer) - layer.LogReleaseMetadata(metadata) - if err != nil { - logrus.Errorf("Failed to release RWLayer: %s", err) - } - rl.rwLayer = nil + rwLayer.Release() return nil, err } - return mountPath, nil + rwLayer.fs = fs + + return rwLayer, nil } -func (rl *releaseableLayer) Commit() (builder.ReleaseableLayer, error) { - var chainID layer.ChainID - if rl.roLayer != nil { - chainID = rl.roLayer.ChainID() - } +type rwLayer struct { + released bool + layerStore layer.Store + rwLayer layer.RWLayer + fs containerfs.ContainerFS +} - stream, err := rl.rwLayer.TarStream() +func (l *rwLayer) Root() containerfs.ContainerFS { + return l.fs +} + +func (l *rwLayer) Commit() (builder.ROLayer, error) { + stream, err := l.rwLayer.TarStream() if err != nil { return nil, err } defer stream.Close() - newLayer, err := rl.layerStore.Register(stream, chainID) + var chainID layer.ChainID + if parent := l.rwLayer.Parent(); parent != nil { + chainID = parent.ChainID() + } + + newLayer, err := l.layerStore.Register(stream, chainID) if err != nil { return nil, err } // TODO: An optimization would be to handle empty layers before returning - return &releaseableLayer{layerStore: rl.layerStore, roLayer: newLayer}, nil + return &roLayer{layerStore: l.layerStore, roLayer: newLayer}, nil } -func (rl *releaseableLayer) DiffID() layer.DiffID { - if rl.roLayer == nil { - return layer.DigestSHA256EmptyTar - } - return rl.roLayer.DiffID() -} - -func (rl *releaseableLayer) Release() error { - if rl.released { +func (l *rwLayer) Release() error { + if l.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 l.fs != nil { + if err := l.rwLayer.Unmount(); err != nil { + return errors.Wrap(err, "failed to unmount RWLayer") + } + l.fs = nil } - if err := rl.releaseROLayer(); err != nil { - return err + + metadata, err := l.layerStore.ReleaseRWLayer(l.rwLayer) + layer.LogReleaseMetadata(metadata) + if err != nil { + return errors.Wrap(err, "failed to release RWLayer") } - rl.released = true + l.released = true return nil } -func (rl *releaseableLayer) releaseRWLayer() error { - if rl.rwLayer == nil { - return nil - } - if err := rl.rwLayer.Unmount(); err != nil { - logrus.Errorf("Failed to unmount RWLayer: %s", err) - return err - } - metadata, err := rl.layerStore.ReleaseRWLayer(rl.rwLayer) - layer.LogReleaseMetadata(metadata) - if err != nil { - logrus.Errorf("Failed to release RWLayer: %s", err) - } - rl.rwLayer = nil - return err -} - -func (rl *releaseableLayer) releaseROLayer() error { - if rl.roLayer == nil { - return nil - } - metadata, err := rl.layerStore.Release(rl.roLayer) - layer.LogReleaseMetadata(metadata) - if err != nil { - logrus.Errorf("Failed to release ROLayer: %s", err) - } - rl.roLayer = nil - return err -} - -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 } diff --git a/integration/build/build_test.go b/integration/build/build_test.go index 3f7b1dcf34..124f1107fb 100644 --- a/integration/build/build_test.go +++ b/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,