From 1ba484186592a904ac08e433ac4053fee2439fb1 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Fri, 24 Aug 2018 15:43:05 +0100 Subject: [PATCH 1/2] config: Fix test that was using system files The `TestMinimalRuntimeConfig` should not be using the real resource files that might be installed on a system so make temporary files instead to better control the test. Split out `TestMinimalRuntimeConfigWithVsock` to reduce cyclomatic complexity (along with dropping the config file delete at the end - not required as the entire test-specific directory gets auto-deleted). Signed-off-by: James O. D. Hunt --- cli/config_test.go | 83 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 76 insertions(+), 7 deletions(-) diff --git a/cli/config_test.go b/cli/config_test.go index e89542ce3..eaa17950b 100644 --- a/cli/config_test.go +++ b/cli/config_test.go @@ -474,6 +474,38 @@ func TestMinimalRuntimeConfig(t *testing.T) { shimPath := path.Join(dir, "shim") proxyPath := path.Join(dir, "proxy") + imagePath := path.Join(dir, "image.img") + initrdPath := path.Join(dir, "initrd.img") + + hypervisorPath := path.Join(dir, "hypervisor") + kernelPath := path.Join(dir, "kernel") + + savedDefaultImagePath := defaultImagePath + savedDefaultInitrdPath := defaultInitrdPath + savedDefaultHypervisorPath := defaultHypervisorPath + savedDefaultKernelPath := defaultKernelPath + + defer func() { + defaultImagePath = savedDefaultImagePath + defaultInitrdPath = savedDefaultInitrdPath + defaultHypervisorPath = savedDefaultHypervisorPath + defaultKernelPath = savedDefaultKernelPath + }() + + // Temporarily change the defaults to avoid this test using the real + // resource files that might be installed on the system! + defaultImagePath = imagePath + defaultInitrdPath = initrdPath + defaultHypervisorPath = hypervisorPath + defaultKernelPath = kernelPath + + for _, file := range []string{defaultImagePath, defaultInitrdPath, defaultHypervisorPath, defaultKernelPath} { + err = writeFile(file, "foo", testFileMode) + if err != nil { + t.Fatal(err) + } + } + runtimeMinimalConfig := ` # Runtime configuration file @@ -555,9 +587,50 @@ func TestMinimalRuntimeConfig(t *testing.T) { if reflect.DeepEqual(config, expectedConfig) == false { t.Fatalf("Got %+v\n expecting %+v", config, expectedConfig) } +} + +func TestMinimalRuntimeConfigWithVsock(t *testing.T) { + dir, err := ioutil.TempDir(testDir, "minimal-runtime-config-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + imagePath := path.Join(dir, "image.img") + initrdPath := path.Join(dir, "initrd.img") + proxyPath := path.Join(dir, "proxy") + shimPath := path.Join(dir, "shim") + hypervisorPath := path.Join(dir, "hypervisor") + kernelPath := path.Join(dir, "kernel") + + savedDefaultImagePath := defaultImagePath + savedDefaultInitrdPath := defaultInitrdPath + savedDefaultHypervisorPath := defaultHypervisorPath + savedDefaultKernelPath := defaultKernelPath + + defer func() { + defaultImagePath = savedDefaultImagePath + defaultInitrdPath = savedDefaultInitrdPath + defaultHypervisorPath = savedDefaultHypervisorPath + defaultKernelPath = savedDefaultKernelPath + }() + + // Temporarily change the defaults to avoid this test using the real + // resource files that might be installed on the system! + defaultImagePath = imagePath + defaultInitrdPath = initrdPath + defaultHypervisorPath = hypervisorPath + defaultKernelPath = kernelPath + + for _, file := range []string{proxyPath, shimPath, hypervisorPath, kernelPath} { + err = writeFile(file, "foo", testFileMode) + if err != nil { + t.Fatal(err) + } + } // minimal config with vsock enabled - runtimeMinimalConfig = ` + runtimeMinimalConfig := ` # Runtime configuration file [hypervisor.qemu] use_vsock = true @@ -579,13 +652,13 @@ func TestMinimalRuntimeConfig(t *testing.T) { utils.VHostVSockDevicePath = "/dev/null" utils.VSockDevicePath = "/dev/null" - configPath = path.Join(dir, "runtime.toml") + configPath := path.Join(dir, "runtime.toml") err = createConfig(configPath, runtimeMinimalConfig) if err != nil { t.Fatal(err) } - _, config, err = loadConfiguration(configPath, false) + _, config, err := loadConfiguration(configPath, false) if err != nil { t.Fatal(err) } @@ -601,10 +674,6 @@ func TestMinimalRuntimeConfig(t *testing.T) { if config.HypervisorConfig.UseVSock != true { t.Fatalf("use_vsock must be true, got %v", config.HypervisorConfig.UseVSock) } - - if err := os.Remove(configPath); err != nil { - t.Fatal(err) - } } func TestNewQemuHypervisorConfig(t *testing.T) { From b5ea753ff401de379bfe132c95249c20b7c5e0fb Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 28 Aug 2018 16:17:55 +0100 Subject: [PATCH 2/2] config: Detect if VM memory smaller than image Add a heuristic to ensure the amount of memory allocated to the hypervisor is bigger than the size of the image. This catches simple configuration issues where `default_memory=` is set to a smaller value than the size of either the `image=` or `initrd=` files. If the configured image type is `initrd`, fail but only warn in the logs for `image` as although it seems a highly unlikely scenario, it is permitted. Update tests to ensure that created resources have `>0` bytes. Fixes #636. Signed-off-by: James O. D. Hunt --- cli/config.go | 70 +++++++++++++++++++++++++++ cli/config_test.go | 109 ++++++++++++++++++++++++++++++++++++++++++- cli/kata-env_test.go | 3 +- cli/utils.go | 13 ++++++ cli/utils_test.go | 34 ++++++++++++++ 5 files changed, 226 insertions(+), 3 deletions(-) diff --git a/cli/config.go b/cli/config.go index 09f53252c..2d631736a 100644 --- a/cli/config.go +++ b/cli/config.go @@ -577,9 +577,79 @@ func loadConfiguration(configPath string, ignoreLogging bool) (resolvedConfigPat config.ProxyConfig = vc.ProxyConfig{} } + if err := checkHypervisorConfig(config.HypervisorConfig); err != nil { + return "", config, err + } + return resolved, config, nil } +// checkHypervisorConfig performs basic "sanity checks" on the hypervisor +// config. +func checkHypervisorConfig(config vc.HypervisorConfig) error { + type image struct { + path string + initrd bool + } + + images := []image{ + { + path: config.ImagePath, + initrd: false, + }, + { + path: config.InitrdPath, + initrd: true, + }, + } + + memSizeMB := int64(config.DefaultMemSz) + + if memSizeMB == 0 { + return errors.New("VM memory cannot be zero") + } + + mb := int64(1024 * 1024) + + for _, image := range images { + if image.path == "" { + continue + } + + imageSizeBytes, err := fileSize(image.path) + if err != nil { + return err + } + + if imageSizeBytes == 0 { + return fmt.Errorf("image %q is empty", image.path) + } + + if imageSizeBytes > mb { + imageSizeMB := imageSizeBytes / mb + + msg := fmt.Sprintf("VM memory (%dMB) smaller than image %q size (%dMB)", + memSizeMB, image.path, imageSizeMB) + + if imageSizeMB >= memSizeMB { + if image.initrd { + // Initrd's need to be fully read into memory + return errors.New(msg) + } + + // Images do not need to be fully read + // into memory, but it would be highly + // unusual to have an image larger + // than the amount of memory assigned + // to the VM. + kataLog.Warn(msg) + } + } + } + + return nil +} + // getDefaultConfigFilePaths returns a list of paths that will be // considered as configuration files in priority order. func getDefaultConfigFilePaths() []string { diff --git a/cli/config_test.go b/cli/config_test.go index eaa17950b..494419134 100644 --- a/cli/config_test.go +++ b/cli/config_test.go @@ -6,6 +6,7 @@ package main import ( + "bytes" "fmt" "io/ioutil" "os" @@ -119,8 +120,8 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config testRuntimeConf files := []string{hypervisorPath, kernelPath, imagePath, shimPath, proxyPath} for _, file := range files { - // create the resource - err = createEmptyFile(file) + // create the resource (which must be >0 bytes) + err := writeFile(file, "foo", testFileMode) if err != nil { return config, err } @@ -1306,3 +1307,107 @@ func TestUpdateRuntimeConfigurationFactoryConfig(t *testing.T) { assert.Equal(expectedFactoryConfig, config.FactoryConfig) } + +func TestCheckHypervisorConfig(t *testing.T) { + assert := assert.New(t) + + dir, err := ioutil.TempDir(testDir, "") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + // Not created on purpose + imageENOENT := filepath.Join(dir, "image-ENOENT.img") + initrdENOENT := filepath.Join(dir, "initrd-ENOENT.img") + + imageEmpty := filepath.Join(dir, "image-empty.img") + initrdEmpty := filepath.Join(dir, "initrd-empty.img") + + for _, file := range []string{imageEmpty, initrdEmpty} { + err = createEmptyFile(file) + assert.NoError(err) + } + + image := filepath.Join(dir, "image.img") + initrd := filepath.Join(dir, "initrd.img") + + mb := uint32(1024 * 1024) + + fileSizeMB := uint32(3) + fileSizeBytes := fileSizeMB * mb + + fileData := strings.Repeat("X", int(fileSizeBytes)) + + for _, file := range []string{image, initrd} { + err = writeFile(file, fileData, testFileMode) + assert.NoError(err) + } + + type testData struct { + imagePath string + initrdPath string + memBytes uint32 + expectError bool + expectLogWarning bool + } + + // Note that checkHypervisorConfig() does not check to ensure an image + // or an initrd has been specified - that's handled by a separate + // function, hence no test for it here. + + data := []testData{ + {"", "", 0, true, false}, + + {imageENOENT, "", 2, true, false}, + {"", initrdENOENT, 2, true, false}, + + {imageEmpty, "", 2, true, false}, + {"", initrdEmpty, 2, true, false}, + + {image, "", fileSizeMB + 2, false, false}, + {image, "", fileSizeMB + 1, false, false}, + {image, "", fileSizeMB + 0, false, true}, + {image, "", fileSizeMB - 1, false, true}, + {image, "", fileSizeMB - 2, false, true}, + + {"", initrd, fileSizeMB + 2, false, false}, + {"", initrd, fileSizeMB + 1, false, false}, + {"", initrd, fileSizeMB + 0, true, false}, + {"", initrd, fileSizeMB - 1, true, false}, + {"", initrd, fileSizeMB - 2, true, false}, + } + + for i, d := range data { + savedOut := kataLog.Logger.Out + + // create buffer to save logger output + logBuf := &bytes.Buffer{} + + // capture output to buffer + kataLog.Logger.Out = logBuf + + config := vc.HypervisorConfig{ + ImagePath: d.imagePath, + InitrdPath: d.initrdPath, + DefaultMemSz: d.memBytes, + } + + err := checkHypervisorConfig(config) + + if d.expectError { + assert.Error(err, "test %d (%+v)", i, d) + } else { + assert.NoError(err, "test %d (%+v)", i, d) + } + + if d.expectLogWarning { + assert.True(strings.Contains(logBuf.String(), "warning")) + } else { + assert.Empty(logBuf.String()) + } + + // reset logger + kataLog.Logger.Out = savedOut + } +} diff --git a/cli/kata-env_test.go b/cli/kata-env_test.go index 5281f3891..515ba434b 100644 --- a/cli/kata-env_test.go +++ b/cli/kata-env_test.go @@ -76,7 +76,8 @@ func makeRuntimeConfig(prefixDir string) (configFile string, config oci.RuntimeC } for _, file := range filesToCreate { - err := createEmptyFile(file) + // files must exist and be >0 bytes. + err := writeFile(file, "foo", testFileMode) if err != nil { return "", oci.RuntimeConfig{}, err } diff --git a/cli/utils.go b/cli/utils.go index 3993f3e08..b544e0ba7 100644 --- a/cli/utils.go +++ b/cli/utils.go @@ -13,6 +13,7 @@ import ( "os/exec" "path/filepath" "strings" + "syscall" ) const ( @@ -230,3 +231,15 @@ func writeFile(filePath string, data string, fileMode os.FileMode) error { func isEmptyString(b []byte) bool { return len(bytes.Trim(b, "\n")) == 0 } + +// fileSize returns the number of bytes in the specified file +func fileSize(file string) (int64, error) { + st := syscall.Stat_t{} + + err := syscall.Stat(file, &st) + if err != nil { + return 0, err + } + + return st.Size, nil +} diff --git a/cli/utils_test.go b/cli/utils_test.go index ec8f81a4d..4fa64e0d6 100644 --- a/cli/utils_test.go +++ b/cli/utils_test.go @@ -362,3 +362,37 @@ func TestWriteFileErrNoPath(t *testing.T) { err = writeFile(dir, "", 0000) assert.Error(err) } + +func TestFileSize(t *testing.T) { + assert := assert.New(t) + + dir, err := ioutil.TempDir(testDir, "") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + file := filepath.Join(dir, "foo") + + // ENOENT + _, err = fileSize(file) + assert.Error(err) + + err = createEmptyFile(file) + assert.NoError(err) + + // zero size + size, err := fileSize(file) + assert.NoError(err) + assert.Equal(size, int64(0)) + + msg := "hello" + msgLen := len(msg) + + err = writeFile(file, msg, testFileMode) + assert.NoError(err) + + size, err = fileSize(file) + assert.NoError(err) + assert.Equal(size, int64(msgLen)) +}