Merge pull request #6228 from puppetlabs:fix-shim-pid-leaking-on-stopped-processes

PiperOrigin-RevId: 381341920

5 files changed, 122 insertions, 9 deletions

diff --git a/pkg/shim/BUILD b/pkg/shim/BUILD
index 367765209..b115556f5 100644
--- a/pkg/shim/BUILD
+++ b/pkg/shim/BUILD
@@ -8,6 +8,7 @@ go_library(
         "api.go",
         "debug.go",
         "epoll.go",
+        "errors.go",
         "options.go",
         "service.go",
         "service_linux.go",
@@ -44,6 +45,8 @@ go_library(
         "@com_github_gogo_protobuf//types:go_default_library",
         "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
         "@com_github_sirupsen_logrus//:go_default_library",
+        "@org_golang_google_grpc//codes:go_default_library",
+        "@org_golang_google_grpc//status:go_default_library",
         "@org_golang_x_sys//unix:go_default_library",
     ],
 )
@@ -51,10 +54,14 @@ go_library(
 go_test(
     name = "shim_test",
     size = "small",
-    srcs = ["service_test.go"],
+    srcs = [
+        "errors_test.go",
+        "service_test.go",
+    ],
     library = ":shim",
     deps = [
         "//pkg/shim/utils",
+        "@com_github_containerd_containerd//errdefs:go_default_library",
         "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
     ],
 )
diff --git a/pkg/shim/errors.go b/pkg/shim/errors.go
new file mode 100644
index 000000000..75d036411
--- /dev/null
+++ b/pkg/shim/errors.go
@@ -0,0 +1,59 @@
+// Copyright 2021 The gVisor Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package shim
+
+import (
+	"context"
+	"errors"
+
+	"github.com/containerd/containerd/errdefs"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+)
+
+// errToGRPC wraps containerd's ToGRPC error mapper which depends on
+// github.com/pkg/errors to work correctly. Once we upgrade to containerd v1.4,
+// this function can go away and we can use errdefs.ToGRPC directly instead.
+//
+// TODO(gvisor.dev/issue/6232): Remove after upgrading to containerd v1.4
+func errToGRPC(err error) error {
+	if err == nil {
+		return nil
+	}
+	if _, ok := status.FromError(err); ok {
+		return err
+	}
+
+	switch {
+	case errors.Is(err, errdefs.ErrInvalidArgument):
+		return status.Errorf(codes.InvalidArgument, err.Error())
+	case errors.Is(err, errdefs.ErrNotFound):
+		return status.Errorf(codes.NotFound, err.Error())
+	case errors.Is(err, errdefs.ErrAlreadyExists):
+		return status.Errorf(codes.AlreadyExists, err.Error())
+	case errors.Is(err, errdefs.ErrFailedPrecondition):
+		return status.Errorf(codes.FailedPrecondition, err.Error())
+	case errors.Is(err, errdefs.ErrUnavailable):
+		return status.Errorf(codes.Unavailable, err.Error())
+	case errors.Is(err, errdefs.ErrNotImplemented):
+		return status.Errorf(codes.Unimplemented, err.Error())
+	case errors.Is(err, context.Canceled):
+		return status.Errorf(codes.Canceled, err.Error())
+	case errors.Is(err, context.DeadlineExceeded):
+		return status.Errorf(codes.DeadlineExceeded, err.Error())
+	}
+
+	return errdefs.ToGRPC(err)
+}
diff --git a/pkg/shim/errors_test.go b/pkg/shim/errors_test.go
new file mode 100644
index 000000000..3c10866cc
--- /dev/null
+++ b/pkg/shim/errors_test.go
@@ -0,0 +1,47 @@
+// Copyright 2021 The gVisor Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package shim
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/containerd/containerd/errdefs"
+)
+
+func TestGRPCRoundTripsErrors(t *testing.T) {
+	for _, tc := range []struct {
+		name string
+		err  error
+		test func(err error) bool
+	}{
+		{
+			name: "passthrough",
+			err:  errdefs.ErrNotFound,
+			test: errdefs.IsNotFound,
+		},
+		{
+			name: "wrapped",
+			err:  fmt.Errorf("oh no: %w", errdefs.ErrNotFound),
+			test: errdefs.IsNotFound,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			if err := errdefs.FromGRPC(errToGRPC(tc.err)); !tc.test(err) {
+				t.Errorf("got %+v", err)
+			}
+		})
+	}
+}
diff --git a/pkg/shim/proc/exec_state.go b/pkg/shim/proc/exec_state.go
index 04a5d19b4..9c6edd3f5 100644
--- a/pkg/shim/proc/exec_state.go
+++ b/pkg/shim/proc/exec_state.go
@@ -151,8 +151,8 @@ func (s *execStoppedState) Delete(ctx context.Context) error {
 	return nil
 }
 
-func (s *execStoppedState) Kill(ctx context.Context, sig uint32, all bool) error {
-	return s.p.kill(ctx, sig, all)
+func (s *execStoppedState) Kill(_ context.Context, sig uint32, _ bool) error {
+	return handleStoppedKill(sig)
 }
 
 func (s *execStoppedState) SetExited(int) {
diff --git a/pkg/shim/service.go b/pkg/shim/service.go
index ea9a1ae10..0b41f0e72 100644
--- a/pkg/shim/service.go
+++ b/pkg/shim/service.go
@@ -452,10 +452,10 @@ func (s *service) Create(ctx context.Context, r *taskAPI.CreateTaskRequest) (*ta
 	}
 	process, err := newInit(r.Bundle, filepath.Join(r.Bundle, "work"), ns, s.platform, config, &s.opts, st.Rootfs)
 	if err != nil {
-		return nil, errdefs.ToGRPC(err)
+		return nil, errToGRPC(err)
 	}
 	if err := process.Create(ctx, config); err != nil {
-		return nil, errdefs.ToGRPC(err)
+		return nil, errToGRPC(err)
 	}
 
 	// Set up OOM notification on the sandbox's cgroup. This is done on
@@ -544,7 +544,7 @@ func (s *service) Exec(ctx context.Context, r *taskAPI.ExecProcessRequest) (*typ
 		Spec:     r.Spec,
 	})
 	if err != nil {
-		return nil, errdefs.ToGRPC(err)
+		return nil, errToGRPC(err)
 	}
 	s.mu.Lock()
 	s.processes[r.ExecID] = process
@@ -565,7 +565,7 @@ func (s *service) ResizePty(ctx context.Context, r *taskAPI.ResizePtyRequest) (*
 		Height: uint16(r.Height),
 	}
 	if err := p.Resize(ws); err != nil {
-		return nil, errdefs.ToGRPC(err)
+		return nil, errToGRPC(err)
 	}
 	return empty, nil
 }
@@ -648,7 +648,7 @@ func (s *service) Kill(ctx context.Context, r *taskAPI.KillRequest) (*types.Empt
 	}
 	if err := p.Kill(ctx, r.Signal, r.All); err != nil {
 		log.L.Debugf("Kill failed: %v", err)
-		return nil, errdefs.ToGRPC(err)
+		return nil, errToGRPC(err)
 	}
 	log.L.Debugf("Kill succeeded")
 	return empty, nil
@@ -660,7 +660,7 @@ func (s *service) Pids(ctx context.Context, r *taskAPI.PidsRequest) (*taskAPI.Pi
 
 	pids, err := s.getContainerPids(ctx, r.ID)
 	if err != nil {
-		return nil, errdefs.ToGRPC(err)
+		return nil, errToGRPC(err)
 	}
 	var processes []*task.ProcessInfo
 	for _, pid := range pids {