Skip to content

Fix capability option race and panic.#3137

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
Random-Liu:fix-race-and-panic
Mar 28, 2019
Merged

Fix capability option race and panic.#3137
crosbymichael merged 1 commit intocontainerd:masterfrom
Random-Liu:fix-race-and-panic

Conversation

@Random-Liu
Copy link
Copy Markdown
Member

@Random-Liu Random-Liu commented Mar 28, 2019 •
edited
Loading

Race

With the following code:

var WithAllCapabilities = WithCapabilities(GetAllCapabilities())

All WithAllCapabilities option will share the same instance of capability list returned by GetAllCapabilities(). This means that dropping a cap will actually drop the cap for all WithAllCapabilities callers.

This is very bad, and we may want to cherry-pick this fix.

Without the fix, the newly added test will fail:

--- FAIL: TestDropCaps (0.00s)
    spec_opts_test.go:487: cap list 0 doesn't contain non-dropped cap
    spec_opts_test.go:487: cap list 1 doesn't contain non-dropped cap
    spec_opts_test.go:487: cap list 2 doesn't contain non-dropped cap
    spec_opts_test.go:487: cap list 3 doesn't contain non-dropped cap
FAIL
exit status 1
FAIL	github.com/containerd/containerd/oci	0.032s

Panic

When pushing the change, I found #3117 has already handled the panic.

But it seems better not to assume there is no duplicated cap.

Without the fix, the newly added test will panic:

--- FAIL: TestDropCaps (0.00s)
panic: runtime error: slice bounds out of range [recovered]
	panic: runtime error: slice bounds out of range

goroutine 13 [running]:
testing.tRunner.func1(0xc0000bcc00)
	/usr/local/go/src/testing/testing.go:792 +0x387
panic(0x635e60, 0xa43ad0)
	/usr/local/go/src/runtime/panic.go:513 +0x1b9
github.com/containerd/containerd/oci.removeCap(0xc0000bb080, 0x67743b, 0x9)
	/usr/local/google/home/lantaol/workspace/src/github.com/containerd/containerd/oci/spec_opts.go:778 +0x24e
github.com/containerd/containerd/oci.WithDroppedCapabilities.func1(0x0, 0x0, 0x0, 0x0, 0x0, 0xc0000baf80, 0x0, 0x0)
	/usr/local/google/home/lantaol/workspace/src/github.com/containerd/containerd/oci/spec_opts.go:826 +0x8f
github.com/containerd/containerd/oci.TestDropCaps(0xc0000bcc00)
	/usr/local/google/home/lantaol/workspace/src/github.com/containerd/containerd/oci/spec_opts_test.go:494 +0x7a0
testing.tRunner(0xc0000bcc00, 0x684ed0)
	/usr/local/go/src/testing/testing.go:827 +0xbf
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:878 +0x353
exit status 2
FAIL	github.com/containerd/containerd/oci	0.035s

Signed-off-by: Lantao Liu [email protected]

@Random-Liu
Fix race and panic.
808b223 
Signed-off-by: Lantao Liu <[email protected]>
@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 28, 2019

Codecov Report

Merging #3137 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3137      +/-   ##
==========================================
+ Coverage   43.58%   43.59%   +0.01%     
==========================================
  Files         104      104              
  Lines       11156    11159       +3     
==========================================
+ Hits         4862     4865       +3     
  Misses       5555     5555              
  Partials      739      739
Flag Coverage Δ
#linux 47.59% <100%> (+0.02%) ⬆️
#windows 40.49% <100%> (-0.01%) ⬇️
Impacted Files Coverage Δ
oci/spec_opts.go 30.58% <100%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90a7da8...808b223. Read the comment docs.

estesp
estesp approved these changes Mar 28, 2019
View reviewed changes
Copy link
Copy Markdown
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

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Mar 28, 2019

LGTM

@crosbymichael crosbymichael merged commit f2a20ea into containerd:master Mar 28, 2019
@Random-Liu Random-Liu deleted the fix-race-and-panic branch March 28, 2019 18:14
This was referenced Mar 28, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estesp estesp estesp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Random-Liu @codecov-io @crosbymichael @estesp @AkihiroSuda