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 panic in NewSystemd on nil values #219

Merged
merged 1 commit into from
Feb 17, 2022
Merged

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented Feb 16, 2022

NewSystemd previously panicked if it was given a nil resource value for
resources.Memory.Max or resources.CPU.Weight (if resources.CPU or
resources.Memory is non-nil)

This also checks the value of Max and Weight before dereferencing them.

Previous to this change, the unit tests I added would fail like this:

cpu:

# go test ./v2/...
--- FAIL: TestSystemdCgroupCpuController_NilWeight (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x6dde61]

goroutine 36 [running]:
testing.tRunner.func1.2({0x7250e0, 0xa12af0})
	/usr/local/go/src/testing/testing.go:1209 +0x24e
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1212 +0x218
panic({0x7250e0, 0xa12af0})
	/usr/local/go/src/runtime/panic.go:1038 +0x215
github.com/containerd/cgroups/v2.NewSystemd({0x7e4144, 0x1}, {0x7809c5, 0x19}, 0xffffffffffffffff, 0xc00014ff10)
	/home/ec2-user/cgroups/v2/manager.go:742 +0xdc1
github.com/containerd/cgroups/v2.TestSystemdCgroupCpuController_NilWeight(0xc000210340)
	/home/ec2-user/cgroups/v2/cpuv2_test.go:84 +0x125
testing.tRunner(0xc000210340, 0x7960d0)
	/usr/local/go/src/testing/testing.go:1259 +0x102
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1306 +0x35a
FAIL	github.com/containerd/cgroups/v2	0.043s
?   	github.com/containerd/cgroups/v2/stats	[no test files]
FAIL

@sparrc sparrc force-pushed the fix-panic branch 5 times, most recently from 2cb7322 to a3067c1 Compare February 17, 2022 00:56
NewSystemd previously panicked if it was given a nil resource value for
resources.Memory.Max or resources.CPU.Weight (if resources.CPU or
resources.Memory is non-nil)

This also checks the value of Max and Weight before dereferencing them.

Signed-off-by: Cameron Sparr <[email protected]>
Copy link
Contributor

@Zyqsempai Zyqsempai left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit cf1b326 into containerd:main Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants