Merge pull request containerd#3137 from Random-Liu/fix-race-and-panic

crosbymichael · web-flow · commit f2a20ead833f · 2019-03-28T11:43:28.000-04:00
Fix capability option race and panic.
diff --git a/oci/spec_opts.go b/oci/spec_opts.go
@@ -741,7 +741,9 @@ func WithCapabilities(caps []string) SpecOpts {
 }
 
 // WithAllCapabilities sets all linux capabilities for the process
-var WithAllCapabilities = WithCapabilities(GetAllCapabilities())
+var WithAllCapabilities = func(ctx context.Context, client Client, c *containers.Container, s *Spec) error {
+	return WithCapabilities(GetAllCapabilities())(ctx, client, c, s)
+}
 
 // GetAllCapabilities returns all caps up to CAP_LAST_CAP
 // or CAP_BLOCK_SUSPEND on RHEL6
@@ -771,12 +773,14 @@ func capsContain(caps []string, s string) bool {
 }
 
 func removeCap(caps *[]string, s string) {
-	for i, c := range *caps {
+	var newcaps []string
+	for _, c := range *caps {
 		if c == s {
-			*caps = append((*caps)[:i], (*caps)[i+1:]...)
-			return
+			continue
 		}
+		newcaps = append(newcaps, c)
 	}
+	*caps = newcaps
 }
 
 // WithAddedCapabilities adds the provided capabilities
diff --git a/oci/spec_opts_test.go b/oci/spec_opts_test.go
@@ -166,25 +166,25 @@ func TestWithEnv(t *testing.T) {
 		Env: []string{"DEFAULT=test"},
 	}
 
-	WithEnv([]string{"env=1"})(nil, nil, nil, &s)
+	WithEnv([]string{"env=1"})(context.Background(), nil, nil, &s)
 
 	if len(s.Process.Env) != 2 {
 		t.Fatal("didn't append")
 	}
 
-	WithEnv([]string{"env2=1"})(nil, nil, nil, &s)
+	WithEnv([]string{"env2=1"})(context.Background(), nil, nil, &s)
 
 	if len(s.Process.Env) != 3 {
 		t.Fatal("didn't append")
 	}
 
-	WithEnv([]string{"env2=2"})(nil, nil, nil, &s)
+	WithEnv([]string{"env2=2"})(context.Background(), nil, nil, &s)
 
 	if s.Process.Env[2] != "env2=2" {
 		t.Fatal("couldn't update")
 	}
 
-	WithEnv([]string{"env2"})(nil, nil, nil, &s)
+	WithEnv([]string{"env2"})(context.Background(), nil, nil, &s)
 
 	if len(s.Process.Env) != 2 {
 		t.Fatal("couldn't unset")
@@ -428,7 +428,7 @@ func TestAddCaps(t *testing.T) {
 
 	var s specs.Spec
 
-	if err := WithAddedCapabilities([]string{"CAP_CHOWN"})(nil, nil, nil, &s); err != nil {
+	if err := WithAddedCapabilities([]string{"CAP_CHOWN"})(context.Background(), nil, nil, &s); err != nil {
 		t.Fatal(err)
 	}
 	for i, cl := range [][]string{
@@ -448,10 +448,10 @@ func TestDropCaps(t *testing.T) {
 
 	var s specs.Spec
 
-	if err := WithAllCapabilities(nil, nil, nil, &s); err != nil {
+	if err := WithAllCapabilities(context.Background(), nil, nil, &s); err != nil {
 		t.Fatal(err)
 	}
-	if err := WithDroppedCapabilities([]string{"CAP_CHOWN"})(nil, nil, nil, &s); err != nil {
+	if err := WithDroppedCapabilities([]string{"CAP_CHOWN"})(context.Background(), nil, nil, &s); err != nil {
 		t.Fatal(err)
 	}
 
@@ -465,4 +465,44 @@ func TestDropCaps(t *testing.T) {
 			t.Errorf("cap list %d contains dropped cap", i)
 		}
 	}
+
+	// Add all capabilities back and drop a different cap.
+	if err := WithAllCapabilities(context.Background(), nil, nil, &s); err != nil {
+		t.Fatal(err)
+	}
+	if err := WithDroppedCapabilities([]string{"CAP_FOWNER"})(context.Background(), nil, nil, &s); err != nil {
+		t.Fatal(err)
+	}
+
+	for i, cl := range [][]string{
+		s.Process.Capabilities.Bounding,
+		s.Process.Capabilities.Effective,
+		s.Process.Capabilities.Permitted,
+		s.Process.Capabilities.Inheritable,
+	} {
+		if capsContain(cl, "CAP_FOWNER") {
+			t.Errorf("cap list %d contains dropped cap", i)
+		}
+		if !capsContain(cl, "CAP_CHOWN") {
+			t.Errorf("cap list %d doesn't contain non-dropped cap", i)
+		}
+	}
+
+	// Drop all duplicated caps.
+	if err := WithCapabilities([]string{"CAP_CHOWN", "CAP_CHOWN"})(context.Background(), nil, nil, &s); err != nil {
+		t.Fatal(err)
+	}
+	if err := WithDroppedCapabilities([]string{"CAP_CHOWN"})(context.Background(), nil, nil, &s); err != nil {
+		t.Fatal(err)
+	}
+	for i, cl := range [][]string{
+		s.Process.Capabilities.Bounding,
+		s.Process.Capabilities.Effective,
+		s.Process.Capabilities.Permitted,
+		s.Process.Capabilities.Inheritable,
+	} {
+		if len(cl) != 0 {
+			t.Errorf("cap list %d is not empty", i)
+		}
+	}
 }

Original file line number	Diff line number	Diff line change
`@@ -741,7 +741,9 @@ func WithCapabilities(caps []string) SpecOpts {`
`741`	`741`	`}`
`742`	`742`
`743`	`743`	`// WithAllCapabilities sets all linux capabilities for the process`
`744`		`-var WithAllCapabilities = WithCapabilities(GetAllCapabilities())`
	`744`	`+var WithAllCapabilities = func(ctx context.Context, client Client, c containers.Container, s Spec) error {`
	`745`	`+ return WithCapabilities(GetAllCapabilities())(ctx, client, c, s)`
	`746`	`+}`
`745`	`747`
`746`	`748`	`// GetAllCapabilities returns all caps up to CAP_LAST_CAP`
`747`	`749`	`// or CAP_BLOCK_SUSPEND on RHEL6`
`@@ -771,12 +773,14 @@ func capsContain(caps []string, s string) bool {`
`771`	`773`	`}`
`772`	`774`
`773`	`775`	`func removeCap(caps *[]string, s string) {`
`774`		`- for i, c := range *caps {`
	`776`	`+ var newcaps []string`
	`777`	`+ for _, c := range *caps {`
`775`	`778`	`if c == s {`
`776`		`- caps = append((caps)[:i], (*caps)[i+1:]...)`
`777`		`- return`
	`779`	`+ continue`
`778`	`780`	`}`
	`781`	`+ newcaps = append(newcaps, c)`
`779`	`782`	`}`
	`783`	`+ *caps = newcaps`
`780`	`784`	`}`
`781`	`785`
`782`	`786`	`// WithAddedCapabilities adds the provided capabilities`