Merge pull request #10431 from duglin/Issue10391

Clarify ENV non-recursivenes w.r.t. env var substitution + fix escaping

builder/evaluator.go

@@ -312,7 +312,11 @@ func (b *Builder) dispatch(stepN int, ast *parser.Node) error {
 		var str string
 		str = ast.Value
 		if _, ok := replaceEnvAllowed[cmd]; ok {
-			str = b.replaceEnv(ast.Value)
+			var err error
+			str, err = ProcessWord(ast.Value, b.Config.Env)
+			if err != nil {
+				return err
+			}
 		}
 		strList[i+l] = str
 		msgList[i] = ast.Value

builder/parser/line_parsers.go

@@ -90,7 +90,7 @@ func parseNameVal(rest string, key string) (*Node, map[string]bool, error) {
 				if blankOK || len(word) > 0 {
 					words = append(words, word)
 
-					// Look for = and if no there assume
+					// Look for = and if not there assume
 					// we're doing the old stuff and
 					// just read the rest of the line
 					if !strings.Contains(word, "=") {
@@ -107,12 +107,15 @@ func parseNameVal(rest string, key string) (*Node, map[string]bool, error) {
 				quote = ch
 				blankOK = true
 				phase = inQuote
-				continue
 			}
 			if ch == '\\' {
 				if pos+1 == len(rest) {
 					continue // just skip \ at end
 				}
+				// If we're not quoted and we see a \, then always just
+				// add \ plus the char to the word, even if the char
+				// is a quote.
+				word += string(ch)
 				pos++
 				ch = rune(rest[pos])
 			}
@@ -122,15 +125,17 @@ func parseNameVal(rest string, key string) (*Node, map[string]bool, error) {
 		if phase == inQuote {
 			if ch == quote {
 				phase = inWord
-				continue
 			}
-			if ch == '\\' {
+			// \ is special except for ' quotes - can't escape anything for '
+			if ch == '\\' && quote != '\'' {
 				if pos+1 == len(rest) {
 					phase = inWord
 					continue // just skip \ at end
 				}
 				pos++
-				ch = rune(rest[pos])
+				nextCh := rune(rest[pos])
+				word += string(ch)
+				ch = nextCh
 			}
 			word += string(ch)
 		}

builder/parser/testfiles/env/Dockerfile

@@ -7,6 +7,14 @@ ENV name=value\ value2
 ENV name="value'quote space'value2"
 ENV name='value"double quote"value2'
 ENV name=value\ value2 name2=value2\ value3
+ENV name="a\"b"
+ENV name="a\'b"
+ENV name='a\'b'
+ENV name='a\'b''
+ENV name='a\"b'
+ENV name="''"
+# don't put anything after the next line - it must be the last line of the
+# Dockerfile and it must end with \
 ENV name=value \
     name1=value1 \
     name2="value2a \

builder/parser/testfiles/env/result

@@ -2,9 +2,15 @@
 (env "name" "value")
 (env "name" "value")
 (env "name" "value" "name2" "value2")
-(env "name" "value value1")
-(env "name" "value value2")
-(env "name" "value'quote space'value2")
-(env "name" "value\"double quote\"value2")
-(env "name" "value value2" "name2" "value2 value3")
-(env "name" "value" "name1" "value1" "name2" "value2a            value2b" "name3" "value3an\"value3b\"" "name4" "value4a\\nvalue4b")
+(env "name" "\"value value1\"")
+(env "name" "value\\ value2")
+(env "name" "\"value'quote space'value2\"")
+(env "name" "'value\"double quote\"value2'")
+(env "name" "value\\ value2" "name2" "value2\\ value3")
+(env "name" "\"a\\\"b\"")
+(env "name" "\"a\\'b\"")
+(env "name" "'a\\'b'")
+(env "name" "'a\\'b''")
+(env "name" "'a\\\"b'")
+(env "name" "\"''\"")
+(env "name" "value" "name1" "value1" "name2" "\"value2a            value2b\"" "name3" "\"value3a\\n\\\"value3b\\\"\"" "name4" "\"value4a\\\\nvalue4b\"")

builder/shell_parser.go

@@ -0,0 +1,209 @@
+package builder
+
+// This will take a single word and an array of env variables and
+// process all quotes (" and ') as well as $xxx and ${xxx} env variable
+// tokens.  Tries to mimic bash shell process.
+// It doesn't support all flavors of ${xx:...} formats but new ones can
+// be added by adding code to the "special ${} format processing" section
+
+import (
+	"fmt"
+	"strings"
+	"unicode"
+)
+
+type shellWord struct {
+	word string
+	envs []string
+	pos  int
+}
+
+func ProcessWord(word string, env []string) (string, error) {
+	sw := &shellWord{
+		word: word,
+		envs: env,
+		pos:  0,
+	}
+	return sw.process()
+}
+
+func (sw *shellWord) process() (string, error) {
+	return sw.processStopOn('\000')
+}
+
+// Process the word, starting at 'pos', and stop when we get to the
+// end of the word or the 'stopChar' character
+func (sw *shellWord) processStopOn(stopChar rune) (string, error) {
+	var result string
+	var charFuncMapping = map[rune]func() (string, error){
+		'\'': sw.processSingleQuote,
+		'"':  sw.processDoubleQuote,
+		'$':  sw.processDollar,
+	}
+
+	for sw.pos < len(sw.word) {
+		ch := sw.peek()
+		if stopChar != '\000' && ch == stopChar {
+			sw.next()
+			break
+		}
+		if fn, ok := charFuncMapping[ch]; ok {
+			// Call special processing func for certain chars
+			tmp, err := fn()
+			if err != nil {
+				return "", err
+			}
+			result += tmp
+		} else {
+			// Not special, just add it to the result
+			ch = sw.next()
+			if ch == '\\' {
+				// '\' escapes, except end of line
+				ch = sw.next()
+				if ch == '\000' {
+					continue
+				}
+			}
+			result += string(ch)
+		}
+	}
+
+	return result, nil
+}
+
+func (sw *shellWord) peek() rune {
+	if sw.pos == len(sw.word) {
+		return '\000'
+	}
+	return rune(sw.word[sw.pos])
+}
+
+func (sw *shellWord) next() rune {
+	if sw.pos == len(sw.word) {
+		return '\000'
+	}
+	ch := rune(sw.word[sw.pos])
+	sw.pos++
+	return ch
+}
+
+func (sw *shellWord) processSingleQuote() (string, error) {
+	// All chars between single quotes are taken as-is
+	// Note, you can't escape '
+	var result string
+
+	sw.next()
+
+	for {
+		ch := sw.next()
+		if ch == '\000' || ch == '\'' {
+			break
+		}
+		result += string(ch)
+	}
+	return result, nil
+}
+
+func (sw *shellWord) processDoubleQuote() (string, error) {
+	// All chars up to the next " are taken as-is, even ', except any $ chars
+	// But you can escape " with a \
+	var result string
+
+	sw.next()
+
+	for sw.pos < len(sw.word) {
+		ch := sw.peek()
+		if ch == '"' {
+			sw.next()
+			break
+		}
+		if ch == '$' {
+			tmp, err := sw.processDollar()
+			if err != nil {
+				return "", err
+			}
+			result += tmp
+		} else {
+			ch = sw.next()
+			if ch == '\\' {
+				chNext := sw.peek()
+
+				if chNext == '\000' {
+					// Ignore \ at end of word
+					continue
+				}
+
+				if chNext == '"' || chNext == '$' {
+					// \" and \$ can be escaped, all other \'s are left as-is
+					ch = sw.next()
+				}
+			}
+			result += string(ch)
+		}
+	}
+
+	return result, nil
+}
+
+func (sw *shellWord) processDollar() (string, error) {
+	sw.next()
+	ch := sw.peek()
+	if ch == '{' {
+		sw.next()
+		name := sw.processName()
+		ch = sw.peek()
+		if ch == '}' {
+			// Normal ${xx} case
+			sw.next()
+			return sw.getEnv(name), nil
+		}
+		return "", fmt.Errorf("Unsupported ${} substitution: %s", sw.word)
+	} else {
+		// $xxx case
+		name := sw.processName()
+		if name == "" {
+			return "$", nil
+		}
+		return sw.getEnv(name), nil
+	}
+}
+
+func (sw *shellWord) processName() string {
+	// Read in a name (alphanumeric or _)
+	// If it starts with a numeric then just return $#
+	var name string
+
+	for sw.pos < len(sw.word) {
+		ch := sw.peek()
+		if len(name) == 0 && unicode.IsDigit(ch) {
+			ch = sw.next()
+			return string(ch)
+		}
+		if !unicode.IsLetter(ch) && !unicode.IsDigit(ch) && ch != '_' {
+			break
+		}
+		ch = sw.next()
+		name += string(ch)
+	}
+
+	return name
+}
+
+func (sw *shellWord) getEnv(name string) string {
+	for _, env := range sw.envs {
+		i := strings.Index(env, "=")
+		if i < 0 {
+			if name == env {
+				// Should probably never get here, but just in case treat
+				// it like "var" and "var=" are the same
+				return ""
+			}
+			continue
+		}
+		if name != env[:i] {
+			continue
+		}
+		return env[i+1:]
+	}
+	return ""
+}

builder/shell_parser_test.go

@@ -0,0 +1,51 @@
+package builder
+
+import (
+	"bufio"
+	"os"
+	"strings"
+	"testing"
+)
+
+func TestShellParser(t *testing.T) {
+	file, err := os.Open("words")
+	if err != nil {
+		t.Fatalf("Can't open 'words': %s", err)
+	}
+	defer file.Close()
+
+	scanner := bufio.NewScanner(file)
+	envs := []string{"PWD=/home", "SHELL=bash"}
+	for scanner.Scan() {
+		line := scanner.Text()
+
+		// Trim comments and blank lines
+		i := strings.Index(line, "#")
+		if i >= 0 {
+			line = line[:i]
+		}
+		line = strings.TrimSpace(line)
+
+		if line == "" {
+			continue
+		}
+
+		words := strings.Split(line, "|")
+		if len(words) != 2 {
+			t.Fatalf("Error in 'words' - should be 2 words:%q", words)
+		}
+
+		words[0] = strings.TrimSpace(words[0])
+		words[1] = strings.TrimSpace(words[1])
+
+		newWord, err := ProcessWord(words[0], envs)
+
+		if err != nil {
+			newWord = "error"
+		}
+
+		if newWord != words[1] {
+			t.Fatalf("Error. Src: %s  Calc: %s  Expected: %s", words[0], newWord, words[1])
+		}
+	}
+}

builder/support.go

@@ -1,50 +1,9 @@
 package builder
 
 import (
-	"regexp"
 	"strings"
 )
 
-var (
-	// `\\\\+|[^\\]|\b|\A` - match any number of "\\" (ie, properly-escaped backslashes), or a single non-backslash character, or a word boundary, or beginning-of-line
-	// `\$` - match literal $
-	// `[[:alnum:]_]+` - match things like `$SOME_VAR`
-	// `{[[:alnum:]_]+}` - match things like `${SOME_VAR}`
-	tokenEnvInterpolation = regexp.MustCompile(`(\\|\\\\+|[^\\]|\b|\A)\$([[:alnum:]_]+|{[[:alnum:]_]+})`)
-	// this intentionally punts on more exotic interpolations like ${SOME_VAR%suffix} and lets the shell handle those directly
-)
-
-// handle environment replacement. Used in dispatcher.
-func (b *Builder) replaceEnv(str string) string {
-	for _, match := range tokenEnvInterpolation.FindAllString(str, -1) {
-		idx := strings.Index(match, "\\$")
-		if idx != -1 {
-			if idx+2 >= len(match) {
-				str = strings.Replace(str, match, "\\$", -1)
-				continue
-			}
-
-			prefix := match[:idx]
-			stripped := match[idx+2:]
-			str = strings.Replace(str, match, prefix+"$"+stripped, -1)
-			continue
-		}
-
-		match = match[strings.Index(match, "$"):]
-		matchKey := strings.Trim(match, "${}")
-
-		for _, keyval := range b.Config.Env {
-			tmp := strings.SplitN(keyval, "=", 2)
-			if tmp[0] == matchKey {
-				str = strings.Replace(str, match, tmp[1], -1)
-				break
-			}
-		}
-	}
-
-	return str
-}
-
 func handleJsonArgs(args []string, attributes map[string]bool) []string {
 	if len(args) == 0 {
 		return []string{}

builder/words

@@ -0,0 +1,43 @@
+hello                    |     hello
+he'll'o                  |     hello
+he'llo                   |     hello
+he\'llo                  |     he'llo
+he\\'llo                 |     he\llo
+abc\tdef                 |     abctdef
+"abc\tdef"               |     abc\tdef
+'abc\tdef'               |     abc\tdef
+hello\                   |     hello
+hello\\                  |     hello\
+"hello                   |     hello
+"hello\"                 |     hello"
+"hel'lo"                 |     hel'lo
+'hello                   |     hello
+'hello\'                 |     hello\
+"''"                     |     ''
+$.                       |     $.
+$1                       |     
+he$1x                    |     hex
+he$.x                    |     he$.x
+he$pwd.                  |     he.
+he$PWD                   |     he/home
+he\$PWD                  |     he$PWD
+he\\$PWD                 |     he\/home
+he\${}                   |     he${}
+he\${}xx                 |     he${}xx
+he${}                    |     he
+he${}xx                  |     hexx
+he${hi}                  |     he
+he${hi}xx                |     hexx
+he${PWD}                 |     he/home
+he${.}                   |     error
+'he${XX}'                |     he${XX}
+"he${PWD}"               |     he/home
+"he'$PWD'"               |     he'/home'
+"$PWD"                   |     /home
+'$PWD'                   |     $PWD
+'\$PWD'                  |     \$PWD
+'"hello"'                |     "hello"
+he\$PWD                  |     he$PWD
+"he\$PWD"                |     he$PWD
+'he\$PWD'                |     he\$PWD
+he${PWD                  |     error

docs/sources/reference/builder.md

@@ -146,6 +146,17 @@ The instructions that handle environment variables in the `Dockerfile` are:
 `ONBUILD` instructions are **NOT** supported for environment replacement, even
 the instructions above.
 
+Environment variable subtitution will use the same value for each variable
+throughout the entire command.  In other words, in this example:
+
+    ENV abc=hello
+    ENV abc=bye def=$abc
+    ENV ghi=$abc
+
+will result in `def` having a value of `hello`, not `bye`.  However, 
+`ghi` will have a value of `bye` because it is not part of the same command
+that set `abc` to `bye`.
+
 ## The `.dockerignore` file
 
 If a file named `.dockerignore` exists in the source repository, then it

integration-cli/docker_cli_build_test.go

@@ -241,9 +241,18 @@ func TestBuildEnvironmentReplacementEnv(t *testing.T) {
 
 	_, err := buildImage(name,
 		`
-  FROM scratch
-  ENV foo foo
+  FROM busybox
+  ENV foo zzz
   ENV bar ${foo}
+  ENV abc1='$foo'
+  ENV env1=$foo env2=${foo} env3="$foo" env4="${foo}"
+  RUN [ "$abc1" = '$foo' ] && (echo "$abc1" | grep -q foo)
+  ENV abc2="\$foo"
+  RUN [ "$abc2" = '$foo' ] && (echo "$abc2" | grep -q foo)
+  ENV abc3 '$foo'
+  RUN [ "$abc3" = '$foo' ] && (echo "$abc3" | grep -q foo)
+  ENV abc4 "\$foo"
+  RUN [ "$abc4" = '$foo' ] && (echo "$abc4" | grep -q foo)
   `, true)
 
 	if err != nil {
@@ -262,13 +271,19 @@ func TestBuildEnvironmentReplacementEnv(t *testing.T) {
 	}
 
 	found := false
+	envCount := 0
 
 	for _, env := range envResult {
 		parts := strings.SplitN(env, "=", 2)
 		if parts[0] == "bar" {
 			found = true
-			if parts[1] != "foo" {
-				t.Fatalf("Could not find replaced var for env `bar`: got %q instead of `foo`", parts[1])
+			if parts[1] != "zzz" {
+				t.Fatalf("Could not find replaced var for env `bar`: got %q instead of `zzz`", parts[1])
+			}
+		} else if strings.HasPrefix(parts[0], "env") {
+			envCount++
+			if parts[1] != "zzz" {
+				t.Fatalf("%s should be 'foo' but instead its %q", parts[0], parts[1])
 			}
 		}
 	}
@@ -277,6 +292,10 @@ func TestBuildEnvironmentReplacementEnv(t *testing.T) {
 		t.Fatal("Never found the `bar` env variable")
 	}
 
+	if envCount != 4 {
+		t.Fatalf("Didn't find all env vars - only saw %d\n%s", envCount, envResult)
+	}
+
 	logDone("build - env environment replacement")
 }
 
@@ -363,8 +382,8 @@ func TestBuildHandleEscapes(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	if _, ok := result[`\\\\\\${FOO}`]; !ok {
-		t.Fatal(`Could not find volume \\\\\\${FOO} set from env foo in volumes table`)
+	if _, ok := result[`\\\${FOO}`]; !ok {
+		t.Fatal(`Could not find volume \\\${FOO} set from env foo in volumes table`, result)
 	}
 
 	logDone("build - handle escapes")
@@ -2256,7 +2275,7 @@ func TestBuildRelativeWorkdir(t *testing.T) {
 
 func TestBuildWorkdirWithEnvVariables(t *testing.T) {
 	name := "testbuildworkdirwithenvvariables"
-	expected := "/test1/test2/$MISSING_VAR"
+	expected := "/test1/test2"
 	defer deleteImages(name)
 	_, err := buildImage(name,
 		`FROM busybox
@@ -4025,9 +4044,9 @@ ENV    abc=zzz TO=/docker/world/hello
 ADD    $FROM $TO
 RUN    [ "$(cat $TO)" = "hello" ]
 ENV    abc "zzz"
-RUN    [ $abc = \"zzz\" ]
+RUN    [ $abc = "zzz" ]
 ENV    abc 'yyy'
-RUN    [ $abc = \'yyy\' ]
+RUN    [ $abc = 'yyy' ]
 ENV    abc=
 RUN    [ "$abc" = "" ]
 
@@ -4043,13 +4062,34 @@ RUN    [ "$abc" = "'foo'" ]
 ENV    abc=\"foo\"
 RUN    [ "$abc" = "\"foo\"" ]
 ENV    abc "foo"
-RUN    [ "$abc" = "\"foo\"" ]
+RUN    [ "$abc" = "foo" ]
 ENV    abc 'foo'
-RUN    [ "$abc" = "'foo'" ]
+RUN    [ "$abc" = 'foo' ]
 ENV    abc \'foo\'
-RUN    [ "$abc" = "\\'foo\\'" ]
+RUN    [ "$abc" = "'foo'" ]
 ENV    abc \"foo\"
-RUN    [ "$abc" = "\\\"foo\\\"" ]
+RUN    [ "$abc" = '"foo"' ]
+
+ENV    e1=bar
+ENV    e2=$e1
+ENV    e3=$e11
+ENV    e4=\$e1
+ENV    e5=\$e11
+RUN    [ "$e0,$e1,$e2,$e3,$e4,$e5" = ',bar,bar,,$e1,$e11' ]
+
+ENV    ee1 bar
+ENV    ee2 $ee1
+ENV    ee3 $ee11
+ENV    ee4 \$ee1
+ENV    ee5 \$ee11
+RUN    [ "$ee1,$ee2,$ee3,$ee4,$ee5" = 'bar,bar,,$ee1,$ee11' ]
+
+ENV    eee1="foo"
+ENV    eee2='foo'
+ENV    eee3 "foo"
+ENV    eee4 'foo'
+RUN    [ "$eee1,$eee2,$eee3,$eee4" = 'foo,foo,foo,foo' ]
+
 `
 	ctx, err := fakeContext(dockerfile, map[string]string{
 		"hello/docker/world": "hello",