Completely rework --change parsing

The way we were trying to parse was very broken. I originally
attempted to use Buildah's Dockerfile parser here, but dealing
with it (and convincing it to accept only a limited subset, and
only one instruction at a time) was challenging, so I rewrote a
subset of Dockerfile parsing. This should handle most common
cases well, though there are definitely unhandled edge cases for
ENV and LABEL.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>

3 files changed, 434 insertions, 163 deletions

diff --git a/pkg/util/utils.go b/pkg/util/utils.go
index 6906b26d5..5ffb05dda 100644
--- a/pkg/util/utils.go
+++ b/pkg/util/utils.go
@@ -1,11 +1,12 @@
 package util
 
 import (
+	"encoding/json"
 	"fmt"
 	"os"
 	"os/user"
 	"path/filepath"
-	"regexp"
+	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -18,6 +19,7 @@ import (
 	"github.com/containers/libpod/pkg/rootless"
 	"github.com/containers/storage"
 	"github.com/containers/storage/pkg/idtools"
+	"github.com/docker/docker/pkg/signal"
 	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -71,118 +73,221 @@ func StringInSlice(s string, sl []string) bool {
 	return false
 }
 
-// ParseChanges returns key, value(s) pair for given option.
-func ParseChanges(option string) (key string, vals []string, err error) {
-	// Supported format as below
-	// 1. key=value
-	// 2. key value
-	// 3. key ["value","value1"]
-	if strings.Contains(option, " ") {
-		// This handles 2 & 3 conditions.
-		var val string
-		tokens := strings.SplitAfterN(option, " ", 2)
-		if len(tokens) < 2 {
-			return "", []string{}, fmt.Errorf("invalid key value %s", option)
-		}
-		key = strings.Trim(tokens[0], " ") // Need to trim whitespace part of delimiter.
-		val = tokens[1]
-		if strings.Contains(tokens[1], "[") && strings.Contains(tokens[1], "]") {
-			//Trim '[',']' if exist.
-			val = strings.TrimLeft(strings.TrimRight(tokens[1], "]"), "[")
-		}
-		vals = strings.Split(val, ",")
-	} else if strings.Contains(option, "=") {
-		// handles condition 1.
-		tokens := strings.Split(option, "=")
-		key = tokens[0]
-		vals = tokens[1:]
-	} else {
-		// either ` ` or `=` must be provided after command
-		return "", []string{}, fmt.Errorf("invalid format %s", option)
-	}
-
-	if len(vals) == 0 {
-		return "", []string{}, errors.Errorf("no value given for instruction %q", key)
-	}
-
-	for _, v := range vals {
-		//each option must not have ' '., `[`` or `]` & empty strings
-		whitespaces := regexp.MustCompile(`[\[\s\]]`)
-		if whitespaces.MatchString(v) || len(v) == 0 {
-			return "", []string{}, fmt.Errorf("invalid value %s", v)
-		}
-	}
-	return key, vals, nil
-}
-
-// GetImageConfig converts the --change flag values in the format "CMD=/bin/bash USER=example"
-// to a type v1.ImageConfig
+// GetImageConfig produces a v1.ImageConfig from the --change flag that is
+// accepted by several Podman commands. It accepts a (limited subset) of
+// Dockerfile instructions.
 func GetImageConfig(changes []string) (v1.ImageConfig, error) {
-	// USER=value | EXPOSE=value | ENV=value | ENTRYPOINT=value |
-	// CMD=value | VOLUME=value | WORKDIR=value | LABEL=key=value | STOPSIGNAL=value
-
-	var (
-		user       string
-		env        []string
-		entrypoint []string
-		cmd        []string
-		workingDir string
-		stopSignal string
-	)
-
-	exposedPorts := make(map[string]struct{})
-	volumes := make(map[string]struct{})
-	labels := make(map[string]string)
-	for _, ch := range changes {
-		key, vals, err := ParseChanges(ch)
-		if err != nil {
-			return v1.ImageConfig{}, err
+	// Valid changes:
+	// USER
+	// EXPOSE
+	// ENV
+	// ENTRYPOINT
+	// CMD
+	// VOLUME
+	// WORKDIR
+	// LABEL
+	// STOPSIGNAL
+
+	config := v1.ImageConfig{}
+
+	for _, change := range changes {
+		// First, let's assume proper Dockerfile format - space
+		// separator between instruction and value
+		split := strings.SplitN(change, " ", 2)
+
+		if len(split) != 2 {
+			split = strings.SplitN(change, "=", 2)
+			if len(split) != 2 {
+				return v1.ImageConfig{}, errors.Errorf("invalid change %q - must be formatted as KEY VALUE", change)
+			}
 		}
 
-		switch key {
+		outerKey := strings.ToUpper(strings.TrimSpace(split[0]))
+		value := strings.TrimSpace(split[1])
+		switch outerKey {
 		case "USER":
-			user = vals[0]
+			// Assume literal contents are the user.
+			if value == "" {
+				return v1.ImageConfig{}, errors.Errorf("invalid change %q - must provide a value to USER", change)
+			}
+			config.User = value
 		case "EXPOSE":
-			var st struct{}
-			exposedPorts[vals[0]] = st
+			// EXPOSE is either [portnum] or
+			// [portnum]/[proto]
+			// Protocol must be "tcp" or "udp"
+			splitPort := strings.Split(value, "/")
+			if len(splitPort) > 2 {
+				return v1.ImageConfig{}, errors.Errorf("invalid change %q - EXPOSE port must be formatted as PORT[/PROTO]", change)
+			}
+			portNum, err := strconv.Atoi(splitPort[0])
+			if err != nil {
+				return v1.ImageConfig{}, errors.Wrapf(err, "invalid change %q - EXPOSE port must be an integer", change)
+			}
+			if portNum > 65535 || portNum <= 0 {
+				return v1.ImageConfig{}, errors.Errorf("invalid change %q - EXPOSE port must be a valid port number", change)
+			}
+			proto := "tcp"
+			if len(splitPort) > 1 {
+				testProto := strings.ToLower(splitPort[1])
+				switch testProto {
+				case "tcp", "udp":
+					proto = testProto
+				default:
+					return v1.ImageConfig{}, errors.Errorf("invalid change %q - EXPOSE protocol must be TCP or UDP", change)
+				}
+			}
+			if config.ExposedPorts == nil {
+				config.ExposedPorts = make(map[string]struct{})
+			}
+			config.ExposedPorts[fmt.Sprintf("%d/%s", portNum, proto)] = struct{}{}
 		case "ENV":
-			if len(vals) < 2 {
-				return v1.ImageConfig{}, errors.Errorf("no value given for environment variable %q", vals[0])
+			// Format is either:
+			// ENV key=value
+			// ENV key=value key=value ...
+			// ENV key value
+			// Both keys and values can be surrounded by quotes to group them.
+			// For now: we only support key=value
+			// We will attempt to strip quotation marks if present.
+
+			var (
+				key, val string
+			)
+
+			splitEnv := strings.SplitN(value, "=", 2)
+			key = splitEnv[0]
+			// We do need a key
+			if key == "" {
+				return v1.ImageConfig{}, errors.Errorf("invalid change %q - ENV must have at least one argument", change)
+			}
+			// Perfectly valid to not have a value
+			if len(splitEnv) == 2 {
+				val = splitEnv[1]
+			}
+
+			if strings.HasPrefix(key, `"`) && strings.HasSuffix(key, `"`) {
+				key = strings.TrimPrefix(strings.TrimSuffix(key, `"`), `"`)
 			}
-			env = append(env, strings.Join(vals[0:], "="))
+			if strings.HasPrefix(val, `"`) && strings.HasSuffix(val, `"`) {
+				val = strings.TrimPrefix(strings.TrimSuffix(val, `"`), `"`)
+			}
+			config.Env = append(config.Env, fmt.Sprintf("%s=%s", key, val))
 		case "ENTRYPOINT":
-			// ENTRYPOINT and CMD can have array of strings
-			entrypoint = append(entrypoint, vals...)
+			// Two valid forms.
+			// First, JSON array.
+			// Second, not a JSON array - we interpret this as an
+			// argument to `sh -c`, unless empty, in which case we
+			// just use a blank entrypoint.
+			testUnmarshal := []string{}
+			if err := json.Unmarshal([]byte(value), &testUnmarshal); err != nil {
+				// It ain't valid JSON, so assume it's an
+				// argument to sh -c if not empty.
+				if value != "" {
+					config.Entrypoint = []string{"/bin/sh", "-c", value}
+				} else {
+					config.Entrypoint = []string{}
+				}
+			} else {
+				// Valid JSON
+				config.Entrypoint = testUnmarshal
+			}
 		case "CMD":
-			// ENTRYPOINT and CMD can have array of strings
-			cmd = append(cmd, vals...)
+			// Same valid forms as entrypoint.
+			// However, where ENTRYPOINT assumes that 'ENTRYPOINT '
+			// means no entrypoint, CMD assumes it is 'sh -c' with
+			// no third argument.
+			testUnmarshal := []string{}
+			if err := json.Unmarshal([]byte(value), &testUnmarshal); err != nil {
+				// It ain't valid JSON, so assume it's an
+				// argument to sh -c.
+				// Only include volume if it's not ""
+				config.Cmd = []string{"/bin/sh", "-c"}
+				if value != "" {
+					config.Cmd = append(config.Cmd, value)
+				}
+			} else {
+				// Valid JSON
+				config.Cmd = testUnmarshal
+			}
 		case "VOLUME":
-			var st struct{}
-			volumes[vals[0]] = st
+			// Either a JSON array or a set of space-separated
+			// paths.
+			// Acts rather similar to ENTRYPOINT and CMD, but always
+			// appends rather than replacing, and no sh -c prepend.
+			testUnmarshal := []string{}
+			if err := json.Unmarshal([]byte(value), &testUnmarshal); err != nil {
+				// Not valid JSON, so split on spaces
+				testUnmarshal = strings.Split(value, " ")
+			}
+			if len(testUnmarshal) == 0 {
+				return v1.ImageConfig{}, errors.Errorf("invalid change %q - must provide at least one argument to VOLUME", change)
+			}
+			for _, vol := range testUnmarshal {
+				if vol == "" {
+					return v1.ImageConfig{}, errors.Errorf("invalid change %q - VOLUME paths must not be empty", change)
+				}
+				if config.Volumes == nil {
+					config.Volumes = make(map[string]struct{})
+				}
+				config.Volumes[vol] = struct{}{}
+			}
 		case "WORKDIR":
-			workingDir = vals[0]
+			// This can be passed multiple times.
+			// Each successive invocation is treated as relative to
+			// the previous one - so WORKDIR /A, WORKDIR b,
+			// WORKDIR c results in /A/b/c
+			// Just need to check it's not empty...
+			if value == "" {
+				return v1.ImageConfig{}, errors.Errorf("invalid change %q - must provide a non-empty WORKDIR", change)
+			}
+			config.WorkingDir = filepath.Join(config.WorkingDir, value)
 		case "LABEL":
-			if len(vals) == 2 {
-				labels[vals[0]] = vals[1]
-			} else {
-				labels[vals[0]] = ""
+			// Same general idea as ENV, but we no longer allow " "
+			// as a separator.
+			// We didn't do that for ENV either, so nice and easy.
+			// Potentially problematic: LABEL might theoretically
+			// allow an = in the key? If people really do this, we
+			// may need to investigate more advanced parsing.
+			var (
+				key, val string
+			)
+
+			splitLabel := strings.SplitN(value, "=", 2)
+			// Unlike ENV, LABEL must have a value
+			if len(splitLabel) != 2 {
+				return v1.ImageConfig{}, errors.Errorf("invalid change %q - LABEL must be formatted key=value", change)
+			}
+			key = splitLabel[0]
+			val = splitLabel[1]
+
+			if strings.HasPrefix(key, `"`) && strings.HasSuffix(key, `"`) {
+				key = strings.TrimPrefix(strings.TrimSuffix(key, `"`), `"`)
+			}
+			if strings.HasPrefix(val, `"`) && strings.HasSuffix(val, `"`) {
+				val = strings.TrimPrefix(strings.TrimSuffix(val, `"`), `"`)
 			}
+			// Check key after we strip quotations
+			if key == "" {
+				return v1.ImageConfig{}, errors.Errorf("invalid change %q - LABEL must have a non-empty key", change)
+			}
+			if config.Labels == nil {
+				config.Labels = make(map[string]string)
+			}
+			config.Labels[key] = val
 		case "STOPSIGNAL":
-			stopSignal = vals[0]
+			// Check the provided signal for validity.
+			// TODO: Worth checking range? ParseSignal allows
+			// negative numbers.
+			killSignal, err := signal.ParseSignal(value)
+			if err != nil {
+				return v1.ImageConfig{}, errors.Wrapf(err, "invalid change %q - KILLSIGNAL must be given a valid signal", change)
+			}
+			config.StopSignal = fmt.Sprintf("%d", killSignal)
+		default:
+			return v1.ImageConfig{}, errors.Errorf("invalid change %q - invalid instruction %s", change, outerKey)
 		}
 	}
 
-	return v1.ImageConfig{
-		User:         user,
-		ExposedPorts: exposedPorts,
-		Env:          env,
-		Entrypoint:   entrypoint,
-		Cmd:          cmd,
-		Volumes:      volumes,
-		WorkingDir:   workingDir,
-		Labels:       labels,
-		StopSignal:   stopSignal,
-	}, nil
+	return config, nil
 }
 
 // ParseIDMapping takes idmappings and subuid and subgid maps and returns a storage mapping
diff --git a/pkg/util/utils_test.go b/pkg/util/utils_test.go
index c938dc592..e60c644fd 100644
--- a/pkg/util/utils_test.go
+++ b/pkg/util/utils_test.go
@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 var (
@@ -19,70 +20,231 @@ func TestStringInSlice(t *testing.T) {
 	assert.False(t, StringInSlice("one", []string{}))
 }
 
-func TestParseChanges(t *testing.T) {
-	// CMD=/bin/sh
-	_, vals, err := ParseChanges("CMD=/bin/sh")
-	assert.EqualValues(t, []string{"/bin/sh"}, vals)
-	assert.NoError(t, err)
-
-	// CMD [/bin/sh]
-	_, vals, err = ParseChanges("CMD [/bin/sh]")
-	assert.EqualValues(t, []string{"/bin/sh"}, vals)
-	assert.NoError(t, err)
-
-	// CMD ["/bin/sh"]
-	_, vals, err = ParseChanges(`CMD ["/bin/sh"]`)
-	assert.EqualValues(t, []string{`"/bin/sh"`}, vals)
-	assert.NoError(t, err)
-
-	// CMD ["/bin/sh","-c","ls"]
-	_, vals, err = ParseChanges(`CMD ["/bin/sh","c","ls"]`)
-	assert.EqualValues(t, []string{`"/bin/sh"`, `"c"`, `"ls"`}, vals)
-	assert.NoError(t, err)
-
-	// CMD ["/bin/sh","arg-with,comma"]
-	_, vals, err = ParseChanges(`CMD ["/bin/sh","arg-with,comma"]`)
-	assert.EqualValues(t, []string{`"/bin/sh"`, `"arg-with`, `comma"`}, vals)
-	assert.NoError(t, err)
-
-	// CMD "/bin/sh"]
-	_, _, err = ParseChanges(`CMD "/bin/sh"]`)
-	assert.Error(t, err)
-	assert.Equal(t, `invalid value "/bin/sh"]`, err.Error())
-
-	// CMD [bin/sh
-	_, _, err = ParseChanges(`CMD "/bin/sh"]`)
-	assert.Error(t, err)
-	assert.Equal(t, `invalid value "/bin/sh"]`, err.Error())
-
-	// CMD ["/bin /sh"]
-	_, _, err = ParseChanges(`CMD ["/bin /sh"]`)
-	assert.Error(t, err)
-	assert.Equal(t, `invalid value "/bin /sh"`, err.Error())
-
-	// CMD ["/bin/sh", "-c","ls"] whitespace between values
-	_, vals, err = ParseChanges(`CMD ["/bin/sh", "c","ls"]`)
-	assert.Error(t, err)
-	assert.Equal(t, `invalid value  "c"`, err.Error())
-
-	// CMD?
-	_, _, err = ParseChanges(`CMD?`)
-	assert.Error(t, err)
-	assert.Equal(t, `invalid format CMD?`, err.Error())
-
-	// empty values for CMD
-	_, _, err = ParseChanges(`CMD `)
-	assert.Error(t, err)
-	assert.Equal(t, `invalid value `, err.Error())
-
-	// LABEL=blue=image
-	_, vals, err = ParseChanges(`LABEL=blue=image`)
-	assert.EqualValues(t, []string{"blue", "image"}, vals)
-	assert.NoError(t, err)
-
-	// LABEL = blue=image
-	_, vals, err = ParseChanges(`LABEL = blue=image`)
-	assert.Error(t, err)
-	assert.Equal(t, `invalid value = blue=image`, err.Error())
+func TestGetImageConfigUser(t *testing.T) {
+	validUser, err := GetImageConfig([]string{"USER valid"})
+	require.Nil(t, err)
+	assert.Equal(t, validUser.User, "valid")
 
+	validUser2, err := GetImageConfig([]string{"USER test_user_2"})
+	require.Nil(t, err)
+	assert.Equal(t, validUser2.User, "test_user_2")
+
+	_, err = GetImageConfig([]string{"USER "})
+	assert.NotNil(t, err)
+}
+
+func TestGetImageConfigExpose(t *testing.T) {
+	validPortNoProto, err := GetImageConfig([]string{"EXPOSE 80"})
+	require.Nil(t, err)
+	_, exists := validPortNoProto.ExposedPorts["80/tcp"]
+	assert.True(t, exists)
+
+	validPortTCP, err := GetImageConfig([]string{"EXPOSE 80/tcp"})
+	require.Nil(t, err)
+	_, exists = validPortTCP.ExposedPorts["80/tcp"]
+	assert.True(t, exists)
+
+	validPortUDP, err := GetImageConfig([]string{"EXPOSE 80/udp"})
+	require.Nil(t, err)
+	_, exists = validPortUDP.ExposedPorts["80/udp"]
+	assert.True(t, exists)
+
+	_, err = GetImageConfig([]string{"EXPOSE 99999"})
+	assert.NotNil(t, err)
+
+	_, err = GetImageConfig([]string{"EXPOSE 80/notaproto"})
+	assert.NotNil(t, err)
+
+	_, err = GetImageConfig([]string{"EXPOSE "})
+	assert.NotNil(t, err)
+
+	_, err = GetImageConfig([]string{"EXPOSE thisisnotanumber"})
+	assert.NotNil(t, err)
+}
+
+func TestGetImageConfigEnv(t *testing.T) {
+	validEnvNoValue, err := GetImageConfig([]string{"ENV key"})
+	require.Nil(t, err)
+	assert.True(t, StringInSlice("key=", validEnvNoValue.Env))
+
+	validEnvBareEquals, err := GetImageConfig([]string{"ENV key="})
+	require.Nil(t, err)
+	assert.True(t, StringInSlice("key=", validEnvBareEquals.Env))
+
+	validEnvKeyValue, err := GetImageConfig([]string{"ENV key=value"})
+	require.Nil(t, err)
+	assert.True(t, StringInSlice("key=value", validEnvKeyValue.Env))
+
+	validEnvKeyMultiEntryValue, err := GetImageConfig([]string{`ENV key="value1 value2"`})
+	require.Nil(t, err)
+	assert.True(t, StringInSlice("key=value1 value2", validEnvKeyMultiEntryValue.Env))
+
+	_, err = GetImageConfig([]string{"ENV "})
+	assert.NotNil(t, err)
+}
+
+func TestGetImageConfigEntrypoint(t *testing.T) {
+	binShEntrypoint, err := GetImageConfig([]string{"ENTRYPOINT /bin/bash"})
+	require.Nil(t, err)
+	require.Equal(t, 3, len(binShEntrypoint.Entrypoint))
+	assert.Equal(t, binShEntrypoint.Entrypoint[0], "/bin/sh")
+	assert.Equal(t, binShEntrypoint.Entrypoint[1], "-c")
+	assert.Equal(t, binShEntrypoint.Entrypoint[2], "/bin/bash")
+
+	entrypointWithSpaces, err := GetImageConfig([]string{"ENTRYPOINT ls -al"})
+	require.Nil(t, err)
+	require.Equal(t, 3, len(entrypointWithSpaces.Entrypoint))
+	assert.Equal(t, entrypointWithSpaces.Entrypoint[0], "/bin/sh")
+	assert.Equal(t, entrypointWithSpaces.Entrypoint[1], "-c")
+	assert.Equal(t, entrypointWithSpaces.Entrypoint[2], "ls -al")
+
+	jsonArrayEntrypoint, err := GetImageConfig([]string{`ENTRYPOINT ["ls", "-al"]`})
+	require.Nil(t, err)
+	require.Equal(t, 2, len(jsonArrayEntrypoint.Entrypoint))
+	assert.Equal(t, jsonArrayEntrypoint.Entrypoint[0], "ls")
+	assert.Equal(t, jsonArrayEntrypoint.Entrypoint[1], "-al")
+
+	emptyEntrypoint, err := GetImageConfig([]string{"ENTRYPOINT "})
+	require.Nil(t, err)
+	assert.Equal(t, 0, len(emptyEntrypoint.Entrypoint))
+
+	emptyEntrypointArray, err := GetImageConfig([]string{"ENTRYPOINT []"})
+	require.Nil(t, err)
+	assert.Equal(t, 0, len(emptyEntrypointArray.Entrypoint))
+}
+
+func TestGetImageConfigCmd(t *testing.T) {
+	binShCmd, err := GetImageConfig([]string{"CMD /bin/bash"})
+	require.Nil(t, err)
+	require.Equal(t, 3, len(binShCmd.Cmd))
+	assert.Equal(t, binShCmd.Cmd[0], "/bin/sh")
+	assert.Equal(t, binShCmd.Cmd[1], "-c")
+	assert.Equal(t, binShCmd.Cmd[2], "/bin/bash")
+
+	cmdWithSpaces, err := GetImageConfig([]string{"CMD ls -al"})
+	require.Nil(t, err)
+	require.Equal(t, 3, len(cmdWithSpaces.Cmd))
+	assert.Equal(t, cmdWithSpaces.Cmd[0], "/bin/sh")
+	assert.Equal(t, cmdWithSpaces.Cmd[1], "-c")
+	assert.Equal(t, cmdWithSpaces.Cmd[2], "ls -al")
+
+	jsonArrayCmd, err := GetImageConfig([]string{`CMD ["ls", "-al"]`})
+	require.Nil(t, err)
+	require.Equal(t, 2, len(jsonArrayCmd.Cmd))
+	assert.Equal(t, jsonArrayCmd.Cmd[0], "ls")
+	assert.Equal(t, jsonArrayCmd.Cmd[1], "-al")
+
+	emptyCmd, err := GetImageConfig([]string{"CMD "})
+	require.Nil(t, err)
+	require.Equal(t, 2, len(emptyCmd.Cmd))
+	assert.Equal(t, emptyCmd.Cmd[0], "/bin/sh")
+	assert.Equal(t, emptyCmd.Cmd[1], "-c")
+
+	blankCmd, err := GetImageConfig([]string{"CMD []"})
+	require.Nil(t, err)
+	assert.Equal(t, 0, len(blankCmd.Cmd))
+}
+
+func TestGetImageConfigVolume(t *testing.T) {
+	oneLenJSONArrayVol, err := GetImageConfig([]string{`VOLUME ["/test1"]`})
+	require.Nil(t, err)
+	_, exists := oneLenJSONArrayVol.Volumes["/test1"]
+	assert.True(t, exists)
+	assert.Equal(t, 1, len(oneLenJSONArrayVol.Volumes))
+
+	twoLenJSONArrayVol, err := GetImageConfig([]string{`VOLUME ["/test1", "/test2"]`})
+	require.Nil(t, err)
+	assert.Equal(t, 2, len(twoLenJSONArrayVol.Volumes))
+	_, exists = twoLenJSONArrayVol.Volumes["/test1"]
+	assert.True(t, exists)
+	_, exists = twoLenJSONArrayVol.Volumes["/test2"]
+	assert.True(t, exists)
+
+	oneLenVol, err := GetImageConfig([]string{"VOLUME /test1"})
+	require.Nil(t, err)
+	_, exists = oneLenVol.Volumes["/test1"]
+	assert.True(t, exists)
+	assert.Equal(t, 1, len(oneLenVol.Volumes))
+
+	twoLenVol, err := GetImageConfig([]string{"VOLUME /test1 /test2"})
+	require.Nil(t, err)
+	assert.Equal(t, 2, len(twoLenVol.Volumes))
+	_, exists = twoLenVol.Volumes["/test1"]
+	assert.True(t, exists)
+	_, exists = twoLenVol.Volumes["/test2"]
+	assert.True(t, exists)
+
+	_, err = GetImageConfig([]string{"VOLUME []"})
+	assert.NotNil(t, err)
+
+	_, err = GetImageConfig([]string{"VOLUME "})
+	assert.NotNil(t, err)
+
+	_, err = GetImageConfig([]string{`VOLUME [""]`})
+	assert.NotNil(t, err)
+}
+
+func TestGetImageConfigWorkdir(t *testing.T) {
+	singleWorkdir, err := GetImageConfig([]string{"WORKDIR /testdir"})
+	require.Nil(t, err)
+	assert.Equal(t, singleWorkdir.WorkingDir, "/testdir")
+
+	twoWorkdirs, err := GetImageConfig([]string{"WORKDIR /testdir", "WORKDIR a"})
+	require.Nil(t, err)
+	assert.Equal(t, twoWorkdirs.WorkingDir, "/testdir/a")
+
+	_, err = GetImageConfig([]string{"WORKDIR "})
+	assert.NotNil(t, err)
+}
+
+func TestGetImageConfigLabel(t *testing.T) {
+	labelNoQuotes, err := GetImageConfig([]string{"LABEL key1=value1"})
+	require.Nil(t, err)
+	assert.Equal(t, labelNoQuotes.Labels["key1"], "value1")
+
+	labelWithQuotes, err := GetImageConfig([]string{`LABEL "key 1"="value 2"`})
+	require.Nil(t, err)
+	assert.Equal(t, labelWithQuotes.Labels["key 1"], "value 2")
+
+	labelNoValue, err := GetImageConfig([]string{"LABEL key="})
+	require.Nil(t, err)
+	contents, exists := labelNoValue.Labels["key"]
+	assert.True(t, exists)
+	assert.Equal(t, contents, "")
+
+	_, err = GetImageConfig([]string{"LABEL key"})
+	assert.NotNil(t, err)
+
+	_, err = GetImageConfig([]string{"LABEL "})
+	assert.NotNil(t, err)
+}
+
+func TestGetImageConfigStopSignal(t *testing.T) {
+	stopSignalValidInt, err := GetImageConfig([]string{"STOPSIGNAL 9"})
+	require.Nil(t, err)
+	assert.Equal(t, stopSignalValidInt.StopSignal, "9")
+
+	stopSignalValidString, err := GetImageConfig([]string{"STOPSIGNAL SIGKILL"})
+	require.Nil(t, err)
+	assert.Equal(t, stopSignalValidString.StopSignal, "9")
+
+	_, err = GetImageConfig([]string{"STOPSIGNAL 0"})
+	assert.NotNil(t, err)
+
+	_, err = GetImageConfig([]string{"STOPSIGNAL garbage"})
+	assert.NotNil(t, err)
+
+	_, err = GetImageConfig([]string{"STOPSIGNAL "})
+	assert.NotNil(t, err)
+}
+
+func TestGetImageConfigMisc(t *testing.T) {
+	_, err := GetImageConfig([]string{""})
+	assert.NotNil(t, err)
+
+	_, err = GetImageConfig([]string{"USER"})
+	assert.NotNil(t, err)
+
+	_, err = GetImageConfig([]string{"BADINST testvalue"})
+	assert.NotNil(t, err)
 }
diff --git a/test/e2e/import_test.go b/test/e2e/import_test.go
index 979440a50..bceb30f7c 100644
--- a/test/e2e/import_test.go
+++ b/test/e2e/import_test.go
@@ -105,7 +105,9 @@ var _ = Describe("Podman import", func() {
 		results.WaitWithDefaultTimeout()
 		Expect(results.ExitCode()).To(Equal(0))
 		imageData := results.InspectImageJSON()
-		Expect(imageData[0].Config.Cmd[0]).To(Equal("/bin/bash"))
+		Expect(imageData[0].Config.Cmd[0]).To(Equal("/bin/sh"))
+		Expect(imageData[0].Config.Cmd[1]).To(Equal("-c"))
+		Expect(imageData[0].Config.Cmd[2]).To(Equal("/bin/bash"))
 	})
 
 	It("podman import with change flag CMD <path>", func() {
@@ -126,6 +128,8 @@ var _ = Describe("Podman import", func() {
 		Expect(results.ExitCode()).To(Equal(0))
 		imageData := results.InspectImageJSON()
 		Expect(imageData[0].Config.Cmd[0]).To(Equal("/bin/sh"))
+		Expect(imageData[0].Config.Cmd[1]).To(Equal("-c"))
+		Expect(imageData[0].Config.Cmd[2]).To(Equal("/bin/sh"))
 	})
 
 	It("podman import with change flag CMD [\"path\",\"path'\"", func() {
@@ -137,7 +141,7 @@ var _ = Describe("Podman import", func() {
 		export.WaitWithDefaultTimeout()
 		Expect(export.ExitCode()).To(Equal(0))
 
-		importImage := podmanTest.Podman([]string{"import", "--change", "CMD [/bin/bash]", outfile, "imported-image"})
+		importImage := podmanTest.Podman([]string{"import", "--change", "CMD [\"/bin/bash\"]", outfile, "imported-image"})
 		importImage.WaitWithDefaultTimeout()
 		Expect(importImage.ExitCode()).To(Equal(0))