Error out if spec is invalid

Closes #66

PiperOrigin-RevId: 202496258
Change-Id: Ib9287c5bf1279ffba1db21ebd9e6b59305cddf34

9 files changed, 150 insertions, 34 deletions

diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go
index da95fa0e7..f359a0eb0 100644
--- a/runsc/boot/loader.go
+++ b/runsc/boot/loader.go
@@ -267,7 +267,7 @@ func newProcess(spec *specs.Spec, conf *Config, ioFDs []int, console bool, creds
 		Filename:             exec,
 		Argv:                 spec.Process.Args,
 		Envv:                 spec.Process.Env,
-		WorkingDirectory:     spec.Process.Cwd,
+		WorkingDirectory:     spec.Process.Cwd, // Defaults to '/' if empty.
 		Credentials:          creds,
 		Umask:                0022,
 		Limits:               ls,
diff --git a/runsc/cmd/boot.go b/runsc/cmd/boot.go
index 0d0e6b63f..685cb6f00 100644
--- a/runsc/cmd/boot.go
+++ b/runsc/cmd/boot.go
@@ -23,6 +23,7 @@ import (
 	"context"
 	"flag"
 	"github.com/google/subcommands"
+	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"gvisor.googlesource.com/gvisor/pkg/log"
 	"gvisor.googlesource.com/gvisor/runsc/boot"
 	"gvisor.googlesource.com/gvisor/runsc/specutils"
@@ -116,6 +117,9 @@ func (b *Boot) Execute(_ context.Context, f *flag.FlagSet, args ...interface{})
 
 	if b.applyCaps {
 		caps := spec.Process.Capabilities
+		if caps == nil {
+			caps = &specs.LinuxCapabilities{}
+		}
 		if conf.Platform == boot.PlatformPtrace {
 			// Ptrace platform requires extra capabilities.
 			const c = "CAP_SYS_PTRACE"
@@ -131,7 +135,7 @@ func (b *Boot) Execute(_ context.Context, f *flag.FlagSet, args ...interface{})
 				args = append(args, arg)
 			}
 		}
-		if err := setCapsAndCallSelf(spec, args, caps); err != nil {
+		if err := setCapsAndCallSelf(args, caps); err != nil {
 			Fatalf("%v", err)
 		}
 		panic("setCapsAndCallSelf must never return success")
diff --git a/runsc/cmd/cmd.go b/runsc/cmd/cmd.go
index 940c8cd14..44ebd7165 100644
--- a/runsc/cmd/cmd.go
+++ b/runsc/cmd/cmd.go
@@ -72,7 +72,7 @@ func (i *intFlags) Set(s string) error {
 // setCapsAndCallSelf sets capabilities to the current thread and then execve's
 // itself again with the arguments specified in 'args' to restart the process
 // with the desired capabilities.
-func setCapsAndCallSelf(spec *specs.Spec, args []string, caps *specs.LinuxCapabilities) error {
+func setCapsAndCallSelf(args []string, caps *specs.LinuxCapabilities) error {
 	// Keep thread locked while capabilities are changed.
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
diff --git a/runsc/cmd/gofer.go b/runsc/cmd/gofer.go
index 8e1060a35..55315c0e8 100644
--- a/runsc/cmd/gofer.go
+++ b/runsc/cmd/gofer.go
@@ -95,7 +95,7 @@ func (g *Gofer) Execute(_ context.Context, f *flag.FlagSet, args ...interface{})
 		// Note: minimal argument handling for the default case to keep it simple.
 		args := os.Args
 		args = append(args, "--apply-caps=false")
-		if err := setCapsAndCallSelf(spec, args, lc); err != nil {
+		if err := setCapsAndCallSelf(args, lc); err != nil {
 			Fatalf("Unable to apply caps: %v", err)
 		}
 		panic("unreachable")
diff --git a/runsc/container/container.go b/runsc/container/container.go
index 428aa5c62..c7dc6ec10 100644
--- a/runsc/container/container.go
+++ b/runsc/container/container.go
@@ -193,9 +193,6 @@ func Create(id string, spec *specs.Spec, conf *boot.Config, bundleDir, consoleSo
 	if err := validateID(id); err != nil {
 		return nil, err
 	}
-	if err := specutils.ValidateSpec(spec); err != nil {
-		return nil, err
-	}
 
 	containerRoot := filepath.Join(conf.RootDir, id)
 	if _, err := os.Stat(containerRoot); err == nil {
@@ -434,8 +431,10 @@ func (c *Container) Destroy() error {
 	log.Debugf("Destroy container %q", c.ID)
 
 	// First stop the container.
-	if err := c.Sandbox.Stop(c.ID); err != nil {
-		return err
+	if c.Sandbox != nil {
+		if err := c.Sandbox.Stop(c.ID); err != nil {
+			return err
+		}
 	}
 
 	// "If any poststop hook fails, the runtime MUST log a warning, but the
diff --git a/runsc/container/container_test.go b/runsc/container/container_test.go
index de487ea97..11285a123 100644
--- a/runsc/container/container_test.go
+++ b/runsc/container/container_test.go
@@ -812,28 +812,6 @@ func TestConsoleSocket(t *testing.T) {
 	}
 }
 
-func TestSpecUnsupported(t *testing.T) {
-	spec := testutil.NewSpecWithArgs("/bin/true")
-	spec.Process.SelinuxLabel = "somelabel"
-
-	// These are normally set by docker and will just cause warnings to be logged.
-	spec.Process.ApparmorProfile = "someprofile"
-	spec.Linux = &specs.Linux{Seccomp: &specs.LinuxSeccomp{}}
-
-	rootDir, bundleDir, conf, err := testutil.SetupContainer(spec)
-	if err != nil {
-		t.Fatalf("error setting up container: %v", err)
-	}
-	defer os.RemoveAll(rootDir)
-	defer os.RemoveAll(bundleDir)
-
-	id := testutil.UniqueContainerID()
-	_, err = container.Create(id, spec, conf, bundleDir, "", "", "")
-	if err == nil || !strings.Contains(err.Error(), "is not supported") {
-		t.Errorf("container.Create() wrong error, got: %v, want: *is not supported, spec.Process: %+v", err, spec.Process)
-	}
-}
-
 // TestRunNonRoot checks that sandbox can be configured when running as
 // non-privileged user.
 func TestRunNonRoot(t *testing.T) {
diff --git a/runsc/specutils/BUILD b/runsc/specutils/BUILD
index 1b6d265bc..34c952bdf 100644
--- a/runsc/specutils/BUILD
+++ b/runsc/specutils/BUILD
@@ -22,4 +22,5 @@ go_test(
     size = "small",
     srcs = ["specutils_test.go"],
     embed = [":specutils"],
+    deps = ["@com_github_opencontainers_runtime-spec//specs-go:go_default_library"],
 )
diff --git a/runsc/specutils/specutils.go b/runsc/specutils/specutils.go
index c552111f2..0d9e09e9d 100644
--- a/runsc/specutils/specutils.go
+++ b/runsc/specutils/specutils.go
@@ -47,10 +47,28 @@ func LogSpec(spec *specs.Spec) {
 
 // ValidateSpec validates that the spec is compatible with runsc.
 func ValidateSpec(spec *specs.Spec) error {
+	// Mandatory fields.
 	if spec.Process == nil {
-		return fmt.Errorf("Process must be defined")
+		return fmt.Errorf("Spec.Process must be defined: %+v", spec)
 	}
-	if spec.Process.SelinuxLabel != "" {
+	if len(spec.Process.Args) == 0 {
+		return fmt.Errorf("Spec.Process.Arg must be defined: %+v", spec.Process)
+	}
+	if spec.Root == nil {
+		return fmt.Errorf("Spec.Root must be defined: %+v", spec)
+	}
+	if len(spec.Root.Path) == 0 {
+		return fmt.Errorf("Spec.Root.Path must be defined: %+v", spec.Root)
+	}
+
+	// Unsupported fields.
+	if spec.Solaris != nil {
+		return fmt.Errorf("Spec.Solaris is not supported: %+v", spec)
+	}
+	if spec.Windows != nil {
+		return fmt.Errorf("Spec.Windows is not supported: %+v", spec)
+	}
+	if len(spec.Process.SelinuxLabel) != 0 {
 		return fmt.Errorf("SELinux is not supported: %s", spec.Process.SelinuxLabel)
 	}
 
@@ -64,7 +82,7 @@ func ValidateSpec(spec *specs.Spec) error {
 		log.Warningf("Seccomp spec is being ignored")
 	}
 
-	// 2 annotations are use by containerd to support multi-container pods.
+	// Two annotations are use by containerd to support multi-container pods.
 	//   "io.kubernetes.cri.container-type"
 	//   "io.kubernetes.cri.sandbox-id"
 	containerType, hasContainerType := spec.Annotations[ContainerdContainerTypeAnnotation]
@@ -98,6 +116,9 @@ func ReadSpec(bundleDir string) (*specs.Spec, error) {
 	if err := json.Unmarshal(specBytes, &spec); err != nil {
 		return nil, fmt.Errorf("error unmarshaling spec from file %q: %v\n %s", specFile, err, string(specBytes))
 	}
+	if err := ValidateSpec(&spec); err != nil {
+		return nil, err
+	}
 	return &spec, nil
 }
 
diff --git a/runsc/specutils/specutils_test.go b/runsc/specutils/specutils_test.go
index ef293e608..959be3af3 100644
--- a/runsc/specutils/specutils_test.go
+++ b/runsc/specutils/specutils_test.go
@@ -20,6 +20,8 @@ import (
 	"strings"
 	"testing"
 	"time"
+
+	specs "github.com/opencontainers/runtime-spec/specs-go"
 )
 
 func TestWaitForReadyHappy(t *testing.T) {
@@ -94,3 +96,114 @@ func TestWaitForReadyTimeout(t *testing.T) {
 	}
 	cmd.Process.Kill()
 }
+
+func TestSpecInvalid(t *testing.T) {
+	for _, test := range []struct {
+		name  string
+		spec  specs.Spec
+		error string
+	}{
+		{
+			name: "valid",
+			spec: specs.Spec{
+				Root: &specs.Root{Path: "/"},
+				Process: &specs.Process{
+					Args: []string{"/bin/true"},
+				},
+			},
+			error: "",
+		},
+		{
+			name: "valid+warning",
+			spec: specs.Spec{
+				Root: &specs.Root{Path: "/"},
+				Process: &specs.Process{
+					Args: []string{"/bin/true"},
+					// This is normally set by docker and will just cause warnings to be logged.
+					ApparmorProfile: "someprofile",
+				},
+				// This is normally set by docker and will just cause warnings to be logged.
+				Linux: &specs.Linux{Seccomp: &specs.LinuxSeccomp{}},
+			},
+			error: "",
+		},
+		{
+			name: "no root",
+			spec: specs.Spec{
+				Process: &specs.Process{
+					Args: []string{"/bin/true"},
+				},
+			},
+			error: "must be defined",
+		},
+		{
+			name: "empty root",
+			spec: specs.Spec{
+				Root: &specs.Root{},
+				Process: &specs.Process{
+					Args: []string{"/bin/true"},
+				},
+			},
+			error: "must be defined",
+		},
+		{
+			name: "no process",
+			spec: specs.Spec{
+				Root: &specs.Root{Path: "/"},
+			},
+			error: "must be defined",
+		},
+		{
+			name: "empty args",
+			spec: specs.Spec{
+				Root:    &specs.Root{Path: "/"},
+				Process: &specs.Process{},
+			},
+			error: "must be defined",
+		},
+		{
+			name: "selinux",
+			spec: specs.Spec{
+				Root: &specs.Root{Path: "/"},
+				Process: &specs.Process{
+					Args:         []string{"/bin/true"},
+					SelinuxLabel: "somelabel",
+				},
+			},
+			error: "is not supported",
+		},
+		{
+			name: "solaris",
+			spec: specs.Spec{
+				Root: &specs.Root{Path: "/"},
+				Process: &specs.Process{
+					Args: []string{"/bin/true"},
+				},
+				Solaris: &specs.Solaris{},
+			},
+			error: "is not supported",
+		},
+		{
+			name: "windows",
+			spec: specs.Spec{
+				Root: &specs.Root{Path: "/"},
+				Process: &specs.Process{
+					Args: []string{"/bin/true"},
+				},
+				Windows: &specs.Windows{},
+			},
+			error: "is not supported",
+		},
+	} {
+		err := ValidateSpec(&test.spec)
+		if len(test.error) == 0 {
+			if err != nil {
+				t.Errorf("ValidateSpec(%q) failed, err: %v", test.name, err)
+			}
+		} else {
+			if err == nil || !strings.Contains(err.Error(), test.error) {
+				t.Errorf("ValidateSpec(%q) wrong error, got: %v, want: .*%s.*", test.name, err, test.error)
+			}
+		}
+	}
+}