ExportContainer: do not panic - #36586
Conversation
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: 7a7357d ("LCOW: Implemented support for docker cp + build") Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 7a7357d ("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 <kolyshkin@gmail.com>
| // 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") |
There was a problem hiding this comment.
Wonder if we should prefix all of these with the function-name (as we don't do it in other places)
There was a problem hiding this comment.
The chance of such error is quite low (i.e. it should not happen with the current code base, as far as I was able to see), so normally a user won't even see it. If they do see it, though, it is kind of an internal error (as in "this software is buggy"), and having some (internal) context would be great to ease the debugging. Ideally, I would even add a stack trace here...
|
Failure on z/s390x is a flaky test, and being addressed in #36551 |
|
@kolyshkin if there isn't any test that covers the codebase affected by this PR, can we get unit tests for this in a separate PR? |
OK, I will see if I can come up with a unit test, imitating a condition described in #36561 |
7 participants
This is proposed replacement for #36563, fixing #36561 and a few more potential nil deref cases. Please see more detailed description in commit messages.
@vdemeester @tonistiigi @thaJeztah PTAL