From c9c8799088b60ac58c943ee5545ebcba7483cd4c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 13 Mar 2018 21:17:11 -0700 Subject: [PATCH 1/4] daemon.ContainerExport(): do not panic In case ContainerExport() is called for an unmounted container, it leads to a daemon panic as container.BaseFS, which is dereferenced here, is nil. To fix, do not rely on container.BaseFS; use the one returned from rwlayer.Mount(). Fixes: 7a7357dae1bccc ("LCOW: Implemented support for docker cp + build") Signed-off-by: Kir Kolyshkin (cherry picked from commit 81f6307eda44ab3a91de6e29304810a976161d74) Signed-off-by: Sebastiaan van Stijn --- components/engine/daemon/export.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/engine/daemon/export.go b/components/engine/daemon/export.go index 1f788ad9a8e..d6d091d1a90 100644 --- a/components/engine/daemon/export.go +++ b/components/engine/daemon/export.go @@ -61,12 +61,12 @@ func (daemon *Daemon) containerExport(container *container.Container) (arch io.R } }() - _, err = rwlayer.Mount(container.GetMountLabel()) + basefs, err := rwlayer.Mount(container.GetMountLabel()) if err != nil { return nil, err } - archive, err := archivePath(container.BaseFS, container.BaseFS.Path(), &archive.TarOptions{ + archive, err := archivePath(basefs, basefs.Path(), &archive.TarOptions{ Compression: archive.Uncompressed, UIDMaps: daemon.idMappings.UIDs(), GIDMaps: daemon.idMappings.GIDs(), From 46ad9721ef5b4a37f7a43271e708d349f2697d98 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 13 Mar 2018 19:45:21 -0700 Subject: [PATCH 2/4] container.BaseFS: check for nil before deref Commit 7a7357dae1bccc ("LCOW: Implemented support for docker cp + build") changed `container.BaseFS` from being a string (that could be empty but can't lead to nil pointer dereference) to containerfs.ContainerFS, which could be be `nil` and so nil dereference is at least theoretically possible, which leads to panic (i.e. engine crashes). Such a panic can be avoided by carefully analysing the source code in all the places that dereference a variable, to make the variable can't be nil. Practically, this analisys are impossible as code is constantly evolving. Still, we need to avoid panics and crashes. A good way to do so is to explicitly check that a variable is non-nil, returning an error otherwise. Even in case such a check looks absolutely redundant, further changes to the code might make it useful, and having an extra check is not a big price to pay to avoid a panic. This commit adds such checks for all the places where it is not obvious that container.BaseFS is not nil (which in this case means we do not call daemon.Mount() a few lines earlier). Signed-off-by: Kir Kolyshkin (cherry picked from commit d6ea46cedaca0098c15843c5254a337d087f5cd6) Signed-off-by: Sebastiaan van Stijn --- components/engine/container/archive.go | 7 +++++++ components/engine/container/container.go | 3 +++ components/engine/daemon/oci_linux.go | 3 +++ components/engine/daemon/oci_windows.go | 3 +++ 4 files changed, 16 insertions(+) diff --git a/components/engine/container/archive.go b/components/engine/container/archive.go index 960d7bf615c..ed72c4a4051 100644 --- a/components/engine/container/archive.go +++ b/components/engine/container/archive.go @@ -6,6 +6,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/system" + "github.com/pkg/errors" ) // ResolvePath resolves the given path in the container to a resource on the @@ -13,6 +14,9 @@ import ( // the absolute path to the resource relative to the container's rootfs, and // an error if the path points to outside the container's rootfs. func (container *Container) ResolvePath(path string) (resolvedPath, absPath string, err error) { + if container.BaseFS == nil { + return "", "", errors.New("ResolvePath: BaseFS of container " + container.ID + " is unexpectedly nil") + } // Check if a drive letter supplied, it must be the system drive. No-op except on Windows path, err = system.CheckSystemDriveAndRemoveDriveLetter(path, container.BaseFS) if err != nil { @@ -45,6 +49,9 @@ func (container *Container) ResolvePath(path string) (resolvedPath, absPath stri // resolved to a path on the host corresponding to the given absolute path // inside the container. func (container *Container) StatPath(resolvedPath, absPath string) (stat *types.ContainerPathStat, err error) { + if container.BaseFS == nil { + return nil, errors.New("StatPath: BaseFS of container " + container.ID + " is unexpectedly nil") + } driver := container.BaseFS lstat, err := driver.Lstat(resolvedPath) diff --git a/components/engine/container/container.go b/components/engine/container/container.go index 461139b4355..a076e80746e 100644 --- a/components/engine/container/container.go +++ b/components/engine/container/container.go @@ -311,6 +311,9 @@ func (container *Container) SetupWorkingDirectory(rootIDs idtools.IDPair) error // symlinking to a different path) between using this method and using the // path. See symlink.FollowSymlinkInScope for more details. func (container *Container) GetResourcePath(path string) (string, error) { + if container.BaseFS == nil { + return "", errors.New("GetResourcePath: BaseFS of container " + container.ID + " is unexpectedly nil") + } // IMPORTANT - These are paths on the OS where the daemon is running, hence // any filepath operations must be done in an OS agnostic way. r, e := container.BaseFS.ResolveScopedPath(path, false) diff --git a/components/engine/daemon/oci_linux.go b/components/engine/daemon/oci_linux.go index 8d5eebb885f..256e1c3e555 100644 --- a/components/engine/daemon/oci_linux.go +++ b/components/engine/daemon/oci_linux.go @@ -705,6 +705,9 @@ func setMounts(daemon *Daemon, s *specs.Spec, c *container.Container, mounts []c } func (daemon *Daemon) populateCommonSpec(s *specs.Spec, c *container.Container) error { + if c.BaseFS == nil { + return errors.New("populateCommonSpec: BaseFS of container " + c.ID + " is unexpectedly nil") + } linkedEnv, err := daemon.setupLinkedContainers(c) if err != nil { return err diff --git a/components/engine/daemon/oci_windows.go b/components/engine/daemon/oci_windows.go index 64c651c4af1..7af8942910c 100644 --- a/components/engine/daemon/oci_windows.go +++ b/components/engine/daemon/oci_windows.go @@ -241,6 +241,9 @@ func (daemon *Daemon) createSpec(c *container.Container) (*specs.Spec, error) { // Sets the Windows-specific fields of the OCI spec func (daemon *Daemon) createSpecWindowsFields(c *container.Container, s *specs.Spec, isHyperV bool) error { + if c.BaseFS == nil { + return errors.New("createSpecWindowsFields: BaseFS of container " + c.ID + " is unexpectedly nil") + } if len(s.Process.Cwd) == 0 { // We default to C:\ to workaround the oddity of the case that the // default directory for cmd running as LocalSystem (or From 3c896e300b54bb59c5e9432eda0573e45625174e Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 15 Mar 2018 00:30:11 -0700 Subject: [PATCH 3/4] integration/TestExportContainerAfterDaemonRestart: add This test case checks that a container created before start of the currently running dockerd can be exported (as reported in #36561). To satisfy this condition, either a pre-existing container is required, or a daemon restart after container creation. Signed-off-by: Kir Kolyshkin (cherry picked from commit 6e7141c7a2c0de6fa3d6c9dcc56978a81f9d835e) Signed-off-by: Sebastiaan van Stijn --- .../integration/container/export_test.go | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/components/engine/integration/container/export_test.go b/components/engine/integration/container/export_test.go index 657b1fce412..8f846b5a294 100644 --- a/components/engine/integration/container/export_test.go +++ b/components/engine/integration/container/export_test.go @@ -7,7 +7,9 @@ import ( "time" "github.com/docker/docker/api/types" + containerTypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/filters" + "github.com/docker/docker/integration-cli/daemon" "github.com/docker/docker/integration/internal/container" "github.com/docker/docker/integration/internal/request" "github.com/docker/docker/pkg/jsonmessage" @@ -51,3 +53,32 @@ func TestExportContainerAndImportImage(t *testing.T) { require.NoError(t, err) assert.Equal(t, jm.Status, images[0].ID) } + +// TestExportContainerAfterDaemonRestart checks that a container +// created before start of the currently running dockerd +// can be exported (as reported in #36561). To satisfy this +// condition, daemon restart is needed after container creation. +func TestExportContainerAfterDaemonRestart(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType != "linux") + skip.If(t, testEnv.IsRemoteDaemon()) + + d := daemon.New(t, "", "dockerd", daemon.Config{}) + client, err := d.NewClient() + require.NoError(t, err) + + d.StartWithBusybox(t) + defer d.Stop(t) + + ctx := context.Background() + cfg := containerTypes.Config{ + Image: "busybox", + Cmd: []string{"top"}, + } + ctr, err := client.ContainerCreate(ctx, &cfg, nil, nil, "") + require.NoError(t, err) + + d.Restart(t) + + _, err = client.ContainerExport(ctx, ctr.ID) + assert.NoError(t, err) +} From a978050d4f2229d11a23fceff1fcc2c65fac2967 Mon Sep 17 00:00:00 2001 From: John Howard Date: Thu, 15 Mar 2018 15:36:36 -0700 Subject: [PATCH 4/4] Windows: Fix Hyper-V containers regression from 36586 Signed-off-by: John Howard (cherry picked from commit 0f5fe3f9cf17457761dab28473ece5a7c94f4a0c) Signed-off-by: Sebastiaan van Stijn --- components/engine/daemon/oci_windows.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/components/engine/daemon/oci_windows.go b/components/engine/daemon/oci_windows.go index 7af8942910c..6777c389954 100644 --- a/components/engine/daemon/oci_windows.go +++ b/components/engine/daemon/oci_windows.go @@ -241,9 +241,6 @@ func (daemon *Daemon) createSpec(c *container.Container) (*specs.Spec, error) { // Sets the Windows-specific fields of the OCI spec func (daemon *Daemon) createSpecWindowsFields(c *container.Container, s *specs.Spec, isHyperV bool) error { - if c.BaseFS == nil { - return errors.New("createSpecWindowsFields: BaseFS of container " + c.ID + " is unexpectedly nil") - } if len(s.Process.Cwd) == 0 { // We default to C:\ to workaround the oddity of the case that the // default directory for cmd running as LocalSystem (or @@ -257,6 +254,10 @@ func (daemon *Daemon) createSpecWindowsFields(c *container.Container, s *specs.S s.Root.Readonly = false // Windows does not support a read-only root filesystem if !isHyperV { + if c.BaseFS == nil { + return errors.New("createSpecWindowsFields: BaseFS of container " + c.ID + " is unexpectedly nil") + } + s.Root.Path = c.BaseFS.Path() // This is not set for Hyper-V containers if !strings.HasSuffix(s.Root.Path, `\`) { s.Root.Path = s.Root.Path + `\` // Ensure a correctly formatted volume GUID path \\?\Volume{GUID}\