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] 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)) +}