Skip to content

[20.10 backport] Fix daemon panic when starting container with invalid device cgroup rule#42001

Merged
tiborvass merged 1 commit into
moby:20.10from
thaJeztah:20.10_backport_fix_cgroup_rule_panic
Feb 18, 2021
Merged

[20.10 backport] Fix daemon panic when starting container with invalid device cgroup rule#42001
tiborvass merged 1 commit into
moby:20.10from
thaJeztah:20.10_backport_fix_cgroup_rule_panic

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

backport of #41919

fixes/addresses docker/compose#8052

This fixes a panic when an invalid "device cgroup rule" is passed, resulting in an "index out of range".

This bug was introduced in the original implementation in 1756af6 (#22563), but was not reproducible when using the CLI, because the same commit also added client-side validation on the flag before making an API request. The following example, uses an invalid rule (c *:* rwm - two spaces before the permissions);

$ docker run --rm --network=host --device-cgroup-rule='c *:*  rwm' busybox
invalid argument "c *:*  rwm" for "--device-cgroup-rule" flag: invalid device cgroup format 'c *:*  rwm'

Doing the same, but using the API results in a daemon panic when starting the container;

  1. Create a container with an invalid device cgroup rule:
$ curl -v \
  --unix-socket /var/run/docker.sock \
  "http://localhost/v1.41/containers/create?name=foobar" \
  -H "Content-Type: application/json" \
  -d '{"Image":"busybox:latest", "HostConfig":{"DeviceCgroupRules": ["c *:*  rwm"]}}'
  1. Start the container:
$ curl -v \
  --unix-socket /var/run/docker.sock \
  -X POST \
  "http://localhost/v1.41/containers/foobar/start"
  1. Observe the daemon logs:
2021-01-22 12:53:03.313806 I | http: panic serving @: runtime error: index out of range [0] with length 0
goroutine 571 [running]:
net/http.(*conn).serve.func1(0xc000cb2d20)
	/usr/local/go/src/net/http/server.go:1795 +0x13b
panic(0x2f32380, 0xc000aebfc0)
	/usr/local/go/src/runtime/panic.go:679 +0x1b6
github.com/docker/docker/oci.AppendDevicePermissionsFromCgroupRules(0xc000175c00, 0x8, 0x8, 0xc0000bd380, 0x1, 0x4, 0x0, 0x0, 0xc0000e69c0, 0x0, ...)
	/go/src/github.com/docker/docker/oci/oci.go:34 +0x64f

This patch:

  • fixes the panic, allowing the daemon to return an error on container start
  • adds a unit-test to validate various permutations
  • adds a "todo" to verify the regular expression (and handling) of the "a" (all) value

We should also consider performing this validation when creating the container,
so that an error is produced early.

- Description for the changelog

Fix daemon panic when starting container with invalid device cgroup rule

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

This fixes a panic when an invalid "device cgroup rule" is passed, resulting
in an "index out of range".

This bug was introduced in the original implementation in 1756af6,
but was not reproducible when using the CLI, because the same commit also added
client-side validation on the flag before making an API request. The following
example, uses an invalid rule (`c *:*  rwm` - two spaces before the permissions);

```console
$ docker run --rm --network=host --device-cgroup-rule='c *:*  rwm' busybox
invalid argument "c *:*  rwm" for "--device-cgroup-rule" flag: invalid device cgroup format 'c *:*  rwm'
```

Doing the same, but using the API results in a daemon panic when starting the container;

Create a container with an invalid device cgroup rule:

```console
curl -v \
  --unix-socket /var/run/docker.sock \
  "http://localhost/v1.41/containers/create?name=foobar" \
  -H "Content-Type: application/json" \
  -d '{"Image":"busybox:latest", "HostConfig":{"DeviceCgroupRules": ["c *:*  rwm"]}}'
```

Start the container:

```console
curl -v \
  --unix-socket /var/run/docker.sock \
  -X POST \
  "http://localhost/v1.41/containers/foobar/start"
```

Observe the daemon logs:

```
2021-01-22 12:53:03.313806 I | http: panic serving @: runtime error: index out of range [0] with length 0
goroutine 571 [running]:
net/http.(*conn).serve.func1(0xc000cb2d20)
	/usr/local/go/src/net/http/server.go:1795 +0x13b
panic(0x2f32380, 0xc000aebfc0)
	/usr/local/go/src/runtime/panic.go:679 +0x1b6
github.com/docker/docker/oci.AppendDevicePermissionsFromCgroupRules(0xc000175c00, 0x8, 0x8, 0xc0000bd380, 0x1, 0x4, 0x0, 0x0, 0xc0000e69c0, 0x0, ...)
	/go/src/github.com/docker/docker/oci/oci.go:34 +0x64f
```

This patch:

- fixes the panic, allowing the daemon to return an error on container start
- adds a unit-test to validate various permutations
- adds a "todo" to verify the regular expression (and handling) of the "a" (all) value

We should also consider performing this validation when _creating_ the container,
so that an error is produced early.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
(cherry picked from commit 5cc1753)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
thaJeztah force-pushed the 20.10_backport_fix_cgroup_rule_panic branch from f021c1a to 1640d7b Compare February 17, 2021 20:16
@thaJeztah

Copy link
Copy Markdown
Member Author

rebased to trigger CI with test-fixes that were merged

@cpuguy83 cpuguy83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@tiborvass
tiborvass merged commit 5bb85a9 into moby:20.10 Feb 18, 2021
@thaJeztah
thaJeztah deleted the 20.10_backport_fix_cgroup_rule_panic branch February 18, 2021 21:27
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