Skip to content
This repository was archived by the owner on Oct 13, 2023. It is now read-only.

[17.12] daemon.cleanupContainer: nullify container RWLayer upon release#424

Closed
thaJeztah wants to merge 2 commits intodocker-archive:17.12from
thaJeztah:17.12-backport-layer-not-retained
Closed

[17.12] daemon.cleanupContainer: nullify container RWLayer upon release#424
thaJeztah wants to merge 2 commits intodocker-archive:17.12from
thaJeztah:17.12-backport-layer-not-retained

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

back port of moby/moby#36160 and moby/moby#36242 for 17.12

git checkout -b 17.12-backport-layer-not-retained upstream/17.12
git cherry-pick -s -S -x -Xsubtree=components/engine e9b9e4ace294230c6b8eb010eda564a2541c4564
git cherry-pick -s -S -x -Xsubtree=components/engine 195893d38160c0893e326b8674e05ef6714aeaa4

Conflict in second commit due to moby/moby@87a1242 (moby/moby#35638) not being included in 17.12;

diff --cc components/engine/daemon/inspect.go
index 20cfa6ce2b,164e1aa2ae..0000000000
--- a/components/engine/daemon/inspect.go
+++ b/components/engine/daemon/inspect.go
@@@ -1,6 -1,7 +1,7 @@@
 -package daemon // import "github.com/docker/docker/daemon"
 +package daemon
  
  import (
+       "errors"
        "fmt"
        "time"
  
@@@ -187,10 -196,13 +195,18 @@@ func (daemon *Daemon) getInspectData(co
        // 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.
++<<<<<<< HEAD
 +      if err != nil && !container.Dead {
 +              return nil, systemError{err}
++=======
+       if err != nil {
+               if !container.Dead {
+                       return nil, errdefs.System(err)
+               }
+       } else {
+               contJSONBase.GraphDriver.Data = graphDriverData
++>>>>>>> 195893d381... c.RWLayer: check for nil before use
        }
-       contJSONBase.GraphDriver.Data = graphDriverData
  
        return contJSONBase, nil
  }

ping @kolyshkin PTAL

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 <kolyshkin@gmail.com>
(cherry picked from commit e9b9e4a)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Since commit e9b9e4a 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: e9b9e4a
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 195893d)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah added this to the 17.12.1 milestone Feb 13, 2018
@thaJeztah
Copy link
Copy Markdown
Member Author

need to have a look at this

15:13:14 daemon/inspect.go:191:26: cannot convert "errors".New("RWLayer of container " + container.ID + " is unexpectedly nil") (type error) to type systemError: need type assertion
15:13:14 daemon/inspect.go:200:27: cannot convert err (type error) to type systemError: need type assertion

@kolyshkin
Copy link
Copy Markdown
Contributor

kolyshkin commented Feb 20, 2018

From the first glance, systemError{err} (rather than systemError(err)) should be used. Here's the definition:

type systemError struct {
        cause error
}

@andrewhsu
Copy link
Copy Markdown
Contributor

Closed in favour of #433

@andrewhsu andrewhsu closed this Feb 21, 2018
@thaJeztah thaJeztah deleted the 17.12-backport-layer-not-retained branch February 21, 2018 17:51
@andrewhsu andrewhsu removed this from the 17.12.1 milestone Feb 21, 2018
docker-jenkins pushed a commit that referenced this pull request Jan 17, 2020
…network_id

[19.03 backport] daemon: Use short libnetwork ID in exec-root & update libnetwork
Upstream-commit: 9077436e6e9c4b4a7050d2412b370ff49d6e161b
Component: engine
docker-jenkins pushed a commit that referenced this pull request Jan 22, 2020
[19.03] Jenkinsfile: fix image-based engine using wrong branch
Upstream-commit: dd6130cdc679c9c7cd5e63e3877fe3c545693ece
Component: packaging
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants