From deff200c90606564d8b7a6fad3e8334f4d40d848 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 30 Jan 2018 15:01:45 -0800 Subject: [PATCH 1/2] daemon.cleanupContainer: nullify container RWLayer upon release ReleaseRWLayer can and should only be called once (unless it returns an error), but might be called twice in case of a failure from `system.EnsureRemoveAll(container.Root)`. This results in the following error: > Error response from daemon: driver "XXX" failed to remove root filesystem for YYY: layer not retained The obvious fix is to set container.RWLayer to nil as soon as ReleaseRWLayer() succeeds. Signed-off-by: Kir Kolyshkin (cherry picked from commit e9b9e4ace294230c6b8eb010eda564a2541c4564) Signed-off-by: Sebastiaan van Stijn --- components/engine/daemon/delete.go | 1 + 1 file changed, 1 insertion(+) diff --git a/components/engine/daemon/delete.go b/components/engine/daemon/delete.go index 4d56d14529..525e95873b 100644 --- a/components/engine/daemon/delete.go +++ b/components/engine/daemon/delete.go @@ -124,6 +124,7 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, forceRemo container.SetRemovalError(e) return e } + container.RWLayer = nil } if err := system.EnsureRemoveAll(container.Root); err != nil { From 71498a13be1109b21935717ebfc1ce8c0c0c5177 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 7 Feb 2018 15:25:50 -0800 Subject: [PATCH 2/2] c.RWLayer: check for nil before use Since commit e9b9e4ace294230c6b8eb has landed, there is a chance that container.RWLayer is nil (due to some half-removed container). Let's check the pointer before use to avoid any potential nil pointer dereferences, resulting in a daemon crash. Note that even without the abovementioned commit, it's better to perform an extra check (even it's totally redundant) rather than to have a possibility of a daemon crash. In other words, better be safe than sorry. [v2: add a test case for daemon.getInspectData] [v3: add a check for container.Dead and a special error for the case] Fixes: e9b9e4ace294230c6b8eb Signed-off-by: Kir Kolyshkin (cherry picked from commit 195893d38160c0893e326b8674e05ef6714aeaa4) Signed-off-by: Sebastiaan van Stijn Signed-off-by: Kir Kolyshkin --- components/engine/daemon/changes.go | 3 +++ components/engine/daemon/daemon.go | 6 +++++ components/engine/daemon/inspect.go | 17 +++++++++--- components/engine/daemon/inspect_test.go | 33 ++++++++++++++++++++++++ components/engine/daemon/oci_windows.go | 4 +++ 5 files changed, 60 insertions(+), 3 deletions(-) create mode 100644 components/engine/daemon/inspect_test.go diff --git a/components/engine/daemon/changes.go b/components/engine/daemon/changes.go index fc8cd2752c..8b163b8f79 100644 --- a/components/engine/daemon/changes.go +++ b/components/engine/daemon/changes.go @@ -22,6 +22,9 @@ func (daemon *Daemon) ContainerChanges(name string) ([]archive.Change, error) { container.Lock() defer container.Unlock() + if container.RWLayer == nil { + return nil, errors.New("RWLayer of container " + name + " is unexpectedly nil") + } c, err := container.RWLayer.Changes() if err != nil { return nil, err diff --git a/components/engine/daemon/daemon.go b/components/engine/daemon/daemon.go index dd8c100c83..3727d95b7b 100644 --- a/components/engine/daemon/daemon.go +++ b/components/engine/daemon/daemon.go @@ -1056,6 +1056,9 @@ func (daemon *Daemon) Shutdown() error { // Mount sets container.BaseFS // (is it not set coming in? why is it unset?) func (daemon *Daemon) Mount(container *container.Container) error { + if container.RWLayer == nil { + return errors.New("RWLayer of container " + container.ID + " is unexpectedly nil") + } dir, err := container.RWLayer.Mount(container.GetMountLabel()) if err != nil { return err @@ -1078,6 +1081,9 @@ func (daemon *Daemon) Mount(container *container.Container) error { // Unmount unsets the container base filesystem func (daemon *Daemon) Unmount(container *container.Container) error { + if container.RWLayer == nil { + return errors.New("RWLayer of container " + container.ID + " is unexpectedly nil") + } if err := container.RWLayer.Unmount(); err != nil { logrus.Errorf("Error unmounting container %s: %s", container.ID, err) return err diff --git a/components/engine/daemon/inspect.go b/components/engine/daemon/inspect.go index 20cfa6ce2b..4e5fc72ac1 100644 --- a/components/engine/daemon/inspect.go +++ b/components/engine/daemon/inspect.go @@ -1,6 +1,7 @@ package daemon import ( + "errors" "fmt" "time" @@ -183,14 +184,24 @@ func (daemon *Daemon) getInspectData(container *container.Container) (*types.Con contJSONBase.GraphDriver.Name = container.Driver + if container.RWLayer == nil { + if container.Dead { + return contJSONBase, nil + } + return nil, systemError{errors.New("RWLayer of container " + container.ID + " is unexpectedly nil")} + } + graphDriverData, err := container.RWLayer.Metadata() // If container is marked as Dead, the container's graphdriver metadata // could have been removed, it will cause error if we try to get the metadata, // we can ignore the error if the container is dead. - if err != nil && !container.Dead { - return nil, systemError{err} + if err != nil { + if !container.Dead { + return nil, systemError{err} + } + } else { + contJSONBase.GraphDriver.Data = graphDriverData } - contJSONBase.GraphDriver.Data = graphDriverData return contJSONBase, nil } diff --git a/components/engine/daemon/inspect_test.go b/components/engine/daemon/inspect_test.go new file mode 100644 index 0000000000..c10cc56796 --- /dev/null +++ b/components/engine/daemon/inspect_test.go @@ -0,0 +1,33 @@ +package daemon // import "github.com/docker/docker/daemon" + +import ( + "testing" + + containertypes "github.com/docker/docker/api/types/container" + "github.com/docker/docker/container" + "github.com/docker/docker/daemon/config" + "github.com/docker/docker/daemon/exec" + + "github.com/stretchr/testify/assert" +) + +func TestGetInspectData(t *testing.T) { + c := &container.Container{ + ID: "inspect-me", + HostConfig: &containertypes.HostConfig{}, + State: container.NewState(), + ExecCommands: exec.NewStore(), + } + + d := &Daemon{ + linkIndex: newLinkIndex(), + configStore: &config.Config{}, + } + + _, err := d.getInspectData(c) + assert.Error(t, err) + + c.Dead = true + _, err = d.getInspectData(c) + assert.NoError(t, err) +} diff --git a/components/engine/daemon/oci_windows.go b/components/engine/daemon/oci_windows.go index c7c94f327a..8647b3e2a7 100644 --- a/components/engine/daemon/oci_windows.go +++ b/components/engine/daemon/oci_windows.go @@ -1,6 +1,7 @@ package daemon import ( + "errors" "fmt" "io/ioutil" "path/filepath" @@ -145,6 +146,9 @@ func (daemon *Daemon) createSpec(c *container.Container) (*specs.Spec, error) { // Reverse order, expecting parent most first s.Windows.LayerFolders = append([]string{layerPath}, s.Windows.LayerFolders...) } + if c.RWLayer == nil { + return nil, errors.New("RWLayer of container " + c.ID + " is unexpectedly nil") + } m, err := c.RWLayer.Metadata() if err != nil { return nil, fmt.Errorf("failed to get layer metadata - %s", err)