Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix npe in exec resize when exec errored #45641

Merged
merged 1 commit into from May 28, 2023
Merged

Conversation

cpuguy83
Copy link
Member

- What I did

In cases where an exec start failed the exec process will be nil even though the channel to signal that the exec started was closed.

Ideally ExecConfig would get a nice refactor to handle this case better (ie. it's not started so don't close that channel). This is a minimal fix to prevent NPE. Luckilly this would only get called by a client and only the http request goroutine gets the panic (http lib recovers the panic).

- How I did it

Add nil check to exec process

- How to verify it

id="$(docker run -d busybox top)"
docker exec -it ${id} command_doesnt_exist

Watch the daemon logs there will be (or potentially will be) a nil pointer exception.

May 27 15:52:50 dev2 dockerd[1023]: 2023/05/27 15:52:50 http: panic serving @: runtime error: invalid memory address or nil pointer dereference
May 27 15:52:50 dev2 dockerd[1023]: goroutine 297 [running]:
May 27 15:52:50 dev2 dockerd[1023]: net/http.(*conn).serve.func1()
May 27 15:52:50 dev2 dockerd[1023]:         /usr/local/go/src/net/http/server.go:1854 +0xbf
May 27 15:52:50 dev2 dockerd[1023]: panic({0x55769d1eb920, 0x55769e9653d0})
May 27 15:52:50 dev2 dockerd[1023]:         /usr/local/go/src/runtime/panic.go:890 +0x263
May 27 15:52:50 dev2 dockerd[1023]: github.com/docker/docker/daemon.(*Daemon).ContainerExecResize(0x55769d1bd080?, {0xc000d75201?, 0x55769cb48910?}, 0x31, 0x118)
May 27 15:52:50 dev2 dockerd[1023]:         /home/cpuguy83/go/src/github.com/docker/docker/daemon/resize.go:51 +0x159
May 27 15:52:50 dev2 dockerd[1023]: github.com/docker/docker/api/server/router/container.(*containerRouter).postContainerExecResize(0xc0011dfb00, {0x0?, 0xc0008e7410?}, {0x55769af8a3e7?, 0x10?}, 0xc000b95500, 0xc000cd5080?)
May 27 15:52:50 dev2 dockerd[1023]:         /home/cpuguy83/go/src/github.com/docker/docker/api/server/router/container/exec.go:175 +0x14e
May 27 15:52:50 dev2 dockerd[1023]: github.com/docker/docker/api/server/middleware.ExperimentalMiddleware.WrapHandler.func1({0x55769d5c9180, 0xc001102b40}, {0x55769d5c7940?, 0xc001232540?}, 0x55769d0b3e20?, 0xc0019dac70?)
May 27 15:52:50 dev2 dockerd[1023]:         /home/cpuguy83/go/src/github.com/docker/docker/api/server/middleware/experimental.go:26 +0x15b
May 27 15:52:50 dev2 dockerd[1023]: github.com/docker/docker/api/server/middleware.VersionMiddleware.WrapHandler.func1({0x55769d5c9180, 0xc001102a20}, {0x55769d5c7940, 0xc001232540}, 0x55769d3c2dc0?, 0x769af86ded?)
May 27 15:52:50 dev2 dockerd[1023]:         /home/cpuguy83/go/src/github.com/docker/docker/api/server/middleware/version.go:62 +0x4d7
May 27 15:52:50 dev2 dockerd[1023]: github.com/docker/docker/pkg/authorization.(*Middleware).WrapHandler.func1({0x55769d5c9180, 0xc001102a20}, {0x55769d5c7940?, 0xc001232540?}, 0xc000b95500, 0x2?)
May 27 15:52:50 dev2 dockerd[1023]:         /home/cpuguy83/go/src/github.com/docker/docker/pkg/authorization/middleware.go:59 +0x649
May 27 15:52:50 dev2 dockerd[1023]: github.com/docker/docker/api/server/middleware.DebugRequestMiddleware.func1({0x55769d5c9180, 0xc001102a20}, {0x55769d5c7940, 0xc001232540}, 0xc000b95500, 0xc0019dab60?)
May 27 15:52:50 dev2 dockerd[1023]:         /home/cpuguy83/go/src/github.com/docker/docker/api/server/middleware/debug.go:25 +0x653
May 27 15:52:50 dev2 dockerd[1023]: github.com/docker/docker/api/server.(*Server).makeHTTPHandler.func1({0x55769d5c7940, 0xc001232540}, 0xc000b95400)
May 27 15:52:50 dev2 dockerd[1023]:         /home/cpuguy83/go/src/github.com/docker/docker/api/server/server.go:123 +0x1ce
May 27 15:52:50 dev2 dockerd[1023]: net/http.HandlerFunc.ServeHTTP(0xc000b95300?, {0x55769d5c7940?, 0xc001232540?}, 0xc0008e79e8?)
May 27 15:52:50 dev2 dockerd[1023]:         /usr/local/go/src/net/http/server.go:2122 +0x2f
May 27 15:52:50 dev2 dockerd[1023]: github.com/docker/docker/vendor/github.com/gorilla/mux.(*Router).ServeHTTP(0xc000a3d740, {0x55769d5c7940, 0xc001232540}, 0xc000b95200)
May 27 15:52:50 dev2 dockerd[1023]:         /home/cpuguy83/go/src/github.com/docker/docker/vendor/github.com/gorilla/mux/mux.go:210 +0x1cf
May 27 15:52:50 dev2 dockerd[1023]: net/http.serverHandler.ServeHTTP({0xc0010ec5a0?}, {0x55769d5c7940, 0xc001232540}, 0xc000b95200)
May 27 15:52:50 dev2 dockerd[1023]:         /usr/local/go/src/net/http/server.go:2936 +0x316
May 27 15:52:50 dev2 dockerd[1023]: net/http.(*conn).serve(0xc001490990, {0x55769d5c9180, 0xc000dd2b10})
May 27 15:52:50 dev2 dockerd[1023]:         /usr/local/go/src/net/http/server.go:1995 +0x612
May 27 15:52:50 dev2 dockerd[1023]: created by net/http.(*Server).Serve
May 27 15:52:50 dev2 dockerd[1023]:         /usr/local/go/src/net/http/server.go:3089 +0x5ed

- Description for the changelog

Fix nil pointer exception in exec resize when exec command errored

- A picture of a cute animal (not mandatory but encouraged)
IMG_0902

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but left one suggestion

daemon/resize.go Outdated Show resolved Hide resolved
In cases where an exec start failed the exec process will be nil even
though the channel to signal that the exec started was closed.

Ideally ExecConfig would get a nice refactor to handle this case better
(ie. it's not started so don't close that channel).
This is a minimal fix to prevent NPE. Luckilly this would only get
called by a client and only the http request goroutine gets the panic
(http lib recovers the panic).

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ☺️

@AkihiroSuda AkihiroSuda merged commit 2ebd97d into moby:master May 28, 2023
101 checks passed
@cpuguy83 cpuguy83 deleted the exec_npe branch September 20, 2023 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants