Skip to content

Fix npe in exec resize when exec errored#45641

Merged
AkihiroSuda merged 1 commit intomoby:masterfrom
cpuguy83:exec_npe
May 28, 2023
Merged

Fix npe in exec resize when exec errored#45641
AkihiroSuda merged 1 commit intomoby:masterfrom
cpuguy83:exec_npe

Conversation

@cpuguy83
Copy link
Copy Markdown
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.

Details
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
Copy Markdown
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

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
Copy Markdown
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 ☺️

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.

3 participants