Check error return from json.Unmarshal

Signed-off-by: Ted Yu <yuzhihong@gmail.com>
Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

runtime/v1/shim/service.go

@@ -521,13 +521,8 @@ func (s *Service) checkProcesses(e runc.Exit) {
 		}
 
 		if ip, ok := p.(*process.Init); ok {
-			shouldKillAll, err := shouldKillAllOnExit(s.bundle)
-			if err != nil {
-				log.G(s.context).WithError(err).Error("failed to check shouldKillAll")
-			}
-
 			// Ensure all children are killed
-			if shouldKillAll {
+			if shouldKillAllOnExit(s.context, s.bundle) {
 				if err := ip.KillAll(s.context); err != nil {
 					log.G(s.context).WithError(err).WithField("id", ip.ID()).
 						Error("failed to kill init's children")
@@ -547,23 +542,25 @@ func (s *Service) checkProcesses(e runc.Exit) {
 	}
 }
 
-func shouldKillAllOnExit(bundlePath string) (bool, error) {
+func shouldKillAllOnExit(ctx context.Context, bundlePath string) bool {
 	var bundleSpec specs.Spec
 	bundleConfigContents, err := ioutil.ReadFile(filepath.Join(bundlePath, "config.json"))
 	if err != nil {
-		return false, err
+		log.G(ctx).WithError(err).Error("shouldKillAllOnExit: failed to read config.json")
+		return true
+	}
+	if err := json.Unmarshal(bundleConfigContents, &bundleSpec); err != nil {
+		log.G(ctx).WithError(err).Error("shouldKillAllOnExit: failed to unmarshal bundle json")
+		return true
 	}
-	json.Unmarshal(bundleConfigContents, &bundleSpec)
-
 	if bundleSpec.Linux != nil {
 		for _, ns := range bundleSpec.Linux.Namespaces {
 			if ns.Type == specs.PIDNamespace && ns.Path == "" {
-				return false, nil
+				return false
 			}
 		}
 	}
-
+	return true
-	return true, nil
 }
 
 func (s *Service) getContainerPids(ctx context.Context, id string) ([]uint32, error) {

runtime/v2/runc/util.go

@@ -19,8 +19,15 @@
 package runc
 
 import (
+	"context"
+	"encoding/json"
+	"io/ioutil"
+	"path/filepath"
+
 	"github.com/containerd/containerd/api/events"
+	"github.com/containerd/containerd/log"
 	"github.com/containerd/containerd/runtime"
+	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/sirupsen/logrus"
 )
 
@@ -53,3 +60,26 @@ func GetTopic(e interface{}) string {
 	}
 	return runtime.TaskUnknownTopic
 }
+
+// ShouldKillAllOnExit reads the bundle's OCI spec and returns true if
+// there is an error reading the spec or if the container has a private PID namespace
+func ShouldKillAllOnExit(ctx context.Context, bundlePath string) bool {
+	var bundleSpec specs.Spec
+	bundleConfigContents, err := ioutil.ReadFile(filepath.Join(bundlePath, "config.json"))
+	if err != nil {
+		log.G(ctx).WithError(err).Error("shouldKillAllOnExit: failed to read config.json")
+		return true
+	}
+	if err := json.Unmarshal(bundleConfigContents, &bundleSpec); err != nil {
+		log.G(ctx).WithError(err).Error("shouldKillAllOnExit: failed to unmarshal bundle json")
+		return true
+	}
+	if bundleSpec.Linux != nil {
+		for _, ns := range bundleSpec.Linux.Namespaces {
+			if ns.Type == specs.PIDNamespace && ns.Path == "" {
+				return false
+			}
+		}
+	}
+	return true
+}

runtime/v2/runc/v1/service.go

@@ -20,7 +20,6 @@ package v1
 
 import (
 	"context"
-	"encoding/json"
 	"io/ioutil"
 	"os"
 	"os/exec"
@@ -33,7 +32,6 @@ import (
 	eventstypes "github.com/containerd/containerd/api/events"
 	"github.com/containerd/containerd/api/types/task"
 	"github.com/containerd/containerd/errdefs"
-	"github.com/containerd/containerd/log"
 	"github.com/containerd/containerd/mount"
 	"github.com/containerd/containerd/namespaces"
 	"github.com/containerd/containerd/pkg/oom"
@@ -49,7 +47,6 @@ import (
 	"github.com/gogo/protobuf/proto"
 	"github.com/gogo/protobuf/types"
 	ptypes "github.com/gogo/protobuf/types"
-	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
@@ -595,14 +592,9 @@ func (s *service) checkProcesses(e runcC.Exit) {
 		return
 	}
 
-	shouldKillAll, err := shouldKillAllOnExit(container.Bundle)
-	if err != nil {
-		log.G(s.context).WithError(err).Error("failed to check shouldKillAll")
-	}
-
 	for _, p := range container.All() {
 		if p.Pid() == e.Pid {
-			if shouldKillAll {
+			if runc.ShouldKillAllOnExit(s.context, container.Bundle) {
 				if ip, ok := p.(*process.Init); ok {
 					// Ensure all children are killed
 					if err := ip.KillAll(s.context); err != nil {
@@ -624,25 +616,6 @@ func (s *service) checkProcesses(e runcC.Exit) {
 	}
 }
 
-func shouldKillAllOnExit(bundlePath string) (bool, error) {
-	var bundleSpec specs.Spec
-	bundleConfigContents, err := ioutil.ReadFile(filepath.Join(bundlePath, "config.json"))
-	if err != nil {
-		return false, err
-	}
-	json.Unmarshal(bundleConfigContents, &bundleSpec)
-
-	if bundleSpec.Linux != nil {
-		for _, ns := range bundleSpec.Linux.Namespaces {
-			if ns.Type == specs.PIDNamespace && ns.Path == "" {
-				return false, nil
-			}
-		}
-	}
-
-	return true, nil
-}
-
 func (s *service) getContainerPids(ctx context.Context, id string) ([]uint32, error) {
 	p, err := s.container.Process("")
 	if err != nil {

runtime/v2/runc/v2/service.go

@@ -35,7 +35,6 @@ import (
 	eventstypes "github.com/containerd/containerd/api/events"
 	"github.com/containerd/containerd/api/types/task"
 	"github.com/containerd/containerd/errdefs"
-	"github.com/containerd/containerd/log"
 	"github.com/containerd/containerd/mount"
 	"github.com/containerd/containerd/namespaces"
 	"github.com/containerd/containerd/pkg/oom"
@@ -51,7 +50,6 @@ import (
 	"github.com/gogo/protobuf/proto"
 	"github.com/gogo/protobuf/types"
 	ptypes "github.com/gogo/protobuf/types"
-	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
@@ -701,13 +699,8 @@ func (s *service) checkProcesses(e runcC.Exit) {
 			}
 
 			if ip, ok := p.(*process.Init); ok {
-				shouldKillAll, err := shouldKillAllOnExit(container.Bundle)
-				if err != nil {
-					log.G(s.context).WithError(err).Error("failed to check shouldKillAll")
-				}
-
 				// Ensure all children are killed
-				if shouldKillAll {
+				if runc.ShouldKillAllOnExit(s.context, container.Bundle) {
 					if err := ip.KillAll(s.context); err != nil {
 						logrus.WithError(err).WithField("id", ip.ID()).
 							Error("failed to kill init's children")
@@ -729,25 +722,6 @@ func (s *service) checkProcesses(e runcC.Exit) {
 	}
 }
 
-func shouldKillAllOnExit(bundlePath string) (bool, error) {
-	var bundleSpec specs.Spec
-	bundleConfigContents, err := ioutil.ReadFile(filepath.Join(bundlePath, "config.json"))
-	if err != nil {
-		return false, err
-	}
-	json.Unmarshal(bundleConfigContents, &bundleSpec)
-
-	if bundleSpec.Linux != nil {
-		for _, ns := range bundleSpec.Linux.Namespaces {
-			if ns.Type == specs.PIDNamespace && ns.Path == "" {
-				return false, nil
-			}
-		}
-	}
-
-	return true, nil
-}
-
 func (s *service) getContainerPids(ctx context.Context, id string) ([]uint32, error) {
 	container, err := s.getContainer(id)
 	if err != nil {