From 955a6ad95f7891a45692d975793abf1eeb07cdd5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 23 May 2018 16:31:49 +0200 Subject: [PATCH 1/6] builder: fix processing of invalid substitusion syntax The builder did not detect syntax errors in substitusions in the Dockerfile, causing those values to be processed incorrectly instead of producing an error. Example 1: missing `}` docker build --no-cache -<<'EOF' FROM busybox ARG var=${aaa:-bbb RUN echo $var EOF Before: Step 3/3 : RUN echo $var ---> Running in f06571e77146 bbb After: Step 2/3 : ARG var=${aaa:-bbb failed to process "${aaa:-bbb": syntax error: missing '}' Example 2: missing closing `}`, no default value docker build --no-cache -<<'EOF' FROM busybox ARG var=${aaa RUN echo $var EOF Before: Step 2/3 : ARG var=${aaa failed to process "${aaa": missing ':' in substitution After: Step 2/3 : ARG var=${aaa failed to process "${aaa": syntax error: missing '}' Example 3: double opening bracket (`{`) docker build --no-cache -<<'EOF' FROM busybox ARG var=${{aaa:-bbb} RUN echo $var EOF Before: Step 2/3 : ARG var=${{aaa:-bbb} failed to process "${{aaa:-bbb}": missing ':' in substitution After: Step 2/3 : ARG var=${{aaa:-bbb} failed to process "${{aaa:-bbb}": syntax error: bad substitution Example 4: double opening bracket (`{`), no default value docker build --no-cache -<<'EOF' FROM busybox ARG var=${{aaa} RUN echo $var EOF Before: Step 2/3 : ARG var=${{aaa} failed to process "${{aaa}": missing ':' in substitution After: Step 2/3 : ARG var=${{aaa} failed to process "${{aaa}": syntax error: bad substitution Signed-off-by: Sebastiaan van Stijn --- builder/dockerfile/shell/envVarTest | 11 +++++++++++ builder/dockerfile/shell/lex.go | 20 +++++++++++++++----- builder/dockerfile/shell/lex_test.go | 2 +- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/builder/dockerfile/shell/envVarTest b/builder/dockerfile/shell/envVarTest index 946b278592..677b33d9a7 100644 --- a/builder/dockerfile/shell/envVarTest +++ b/builder/dockerfile/shell/envVarTest @@ -119,3 +119,14 @@ A|안녕${XXX:-\$PWD:}xx | 안녕$PWD:xx A|안녕${XXX:-\${PWD}z}xx | 안녕${PWDz}xx A|$KOREAN | 한국어 A|안녕$KOREAN | 안녕한국어 +A|${{aaa} | error +A|${aaa}} | } +A|${aaa | error +A|${{aaa:-bbb} | error +A|${aaa:-bbb}} | bbb} +A|${aaa:-bbb | error +A|${aaa:-bbb} | bbb +A|${aaa:-${bbb:-ccc}} | ccc +A|${aaa:-bbb ${foo} | error +A|${aaa:-bbb {foo} | bbb {foo + diff --git a/builder/dockerfile/shell/lex.go b/builder/dockerfile/shell/lex.go index bd3fac525a..d0d6f53e66 100644 --- a/builder/dockerfile/shell/lex.go +++ b/builder/dockerfile/shell/lex.go @@ -131,7 +131,7 @@ func (sw *shellWord) processStopOn(stopChar rune) (string, []string, error) { if stopChar != scanner.EOF && ch == stopChar { sw.scanner.Next() - break + return result.String(), words.getWords(), nil } if fn, ok := charFuncMapping[ch]; ok { // Call special processing func for certain chars @@ -166,7 +166,9 @@ func (sw *shellWord) processStopOn(stopChar rune) (string, []string, error) { result.WriteRune(ch) } } - + if stopChar != scanner.EOF { + return "", []string{}, errors.Errorf("unexpected end of statement while looking for matching %s", string(stopChar)) + } return result.String(), words.getWords(), nil } @@ -261,12 +263,17 @@ func (sw *shellWord) processDollar() (string, error) { sw.scanner.Next() name := sw.processName() ch := sw.scanner.Peek() - if ch == '}' { + switch ch { + case '}': // Normal ${xx} case sw.scanner.Next() return sw.getEnv(name), nil - } - if ch == ':' { + case '{': + // Invalid ${{xx} case + return "", errors.New("syntax error: bad substitution") + case scanner.EOF: + return "", errors.New("syntax error: missing '}'") + case ':': // Special ${xx:...} format processing // Yes it allows for recursive $'s in the ... spot @@ -275,6 +282,9 @@ func (sw *shellWord) processDollar() (string, error) { word, _, err := sw.processStopOn('}') if err != nil { + if sw.scanner.Peek() == scanner.EOF { + return "", errors.New("syntax error: missing '}'") + } return "", err } diff --git a/builder/dockerfile/shell/lex_test.go b/builder/dockerfile/shell/lex_test.go index f38da2026f..765ae14b25 100644 --- a/builder/dockerfile/shell/lex_test.go +++ b/builder/dockerfile/shell/lex_test.go @@ -53,7 +53,7 @@ func TestShellParser4EnvVars(t *testing.T) { ((platform == "U" || platform == "A") && runtime.GOOS != "windows") { newWord, err := shlex.ProcessWord(source, envs) if expected == "error" { - assert.Check(t, is.ErrorContains(err, "")) + assert.Check(t, is.ErrorContains(err, ""), "input: %q, result: %q", source, newWord) } else { assert.Check(t, err) assert.Check(t, is.Equal(newWord, expected)) From 334bf3ea76004d0abe02dd1698989f9eaf87a86a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 25 May 2018 17:31:14 +0200 Subject: [PATCH 2/6] Fix detection for missing parameter in substitution `${}`, `${:}` and so on are invalid because there's no parameter within the brackets; fix detection for this situation and add/update tests. There were some existing test-cases that were testing for the wrong behavior, which are now updated. Signed-off-by: Sebastiaan van Stijn --- builder/dockerfile/shell/envVarTest | 15 +++++++++++---- builder/dockerfile/shell/lex.go | 17 ++++++++--------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/builder/dockerfile/shell/envVarTest b/builder/dockerfile/shell/envVarTest index 677b33d9a7..2c0cc1fcf4 100644 --- a/builder/dockerfile/shell/envVarTest +++ b/builder/dockerfile/shell/envVarTest @@ -29,10 +29,14 @@ A|he\$PWD | he$PWD A|he\\$PWD | he\/home A|"he\$PWD" | he$PWD A|"he\\$PWD" | he\/home +A|\${} | ${} +A|\${}aaa | ${}aaa A|he\${} | he${} A|he\${}xx | he${}xx -A|he${} | he -A|he${}xx | hexx +A|${} | error +A|${}aaa | error +A|he${} | error +A|he${}xx | error A|he${hi} | he A|he${hi}xx | hexx A|he${PWD} | he/home @@ -88,8 +92,8 @@ A|안녕\$PWD | 안녕$PWD A|안녕\\$PWD | 안녕\/home A|안녕\${} | 안녕${} A|안녕\${}xx | 안녕${}xx -A|안녕${} | 안녕 -A|안녕${}xx | 안녕xx +A|안녕${} | error +A|안녕${}xx | error A|안녕${hi} | 안녕 A|안녕${hi}xx | 안녕xx A|안녕${PWD} | 안녕/home @@ -129,4 +133,7 @@ A|${aaa:-bbb} | bbb A|${aaa:-${bbb:-ccc}} | ccc A|${aaa:-bbb ${foo} | error A|${aaa:-bbb {foo} | bbb {foo +A|${:} | error +A|${:-bbb} | error +A|${:+bbb} | error diff --git a/builder/dockerfile/shell/lex.go b/builder/dockerfile/shell/lex.go index d0d6f53e66..983e5c0517 100644 --- a/builder/dockerfile/shell/lex.go +++ b/builder/dockerfile/shell/lex.go @@ -261,23 +261,22 @@ func (sw *shellWord) processDollar() (string, error) { } sw.scanner.Next() + switch sw.scanner.Peek() { + case scanner.EOF: + return "", errors.New("syntax error: missing '}'") + case '{', '}', ':': + // Invalid ${{xx}, ${:xx}, ${:}. ${} case + return "", errors.New("syntax error: bad substitution") + } name := sw.processName() - ch := sw.scanner.Peek() + ch := sw.scanner.Next() switch ch { case '}': // Normal ${xx} case - sw.scanner.Next() return sw.getEnv(name), nil - case '{': - // Invalid ${{xx} case - return "", errors.New("syntax error: bad substitution") - case scanner.EOF: - return "", errors.New("syntax error: missing '}'") case ':': // Special ${xx:...} format processing // Yes it allows for recursive $'s in the ... spot - - sw.scanner.Next() // skip over : modifier := sw.scanner.Next() word, _, err := sw.processStopOn('}') From b80e0309d220268a2b9e6aec5bb05d7af330e591 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 25 May 2018 17:44:35 +0200 Subject: [PATCH 3/6] Add line-numbers to asserts Signed-off-by: Sebastiaan van Stijn --- builder/dockerfile/shell/lex_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builder/dockerfile/shell/lex_test.go b/builder/dockerfile/shell/lex_test.go index 765ae14b25..fa42c92989 100644 --- a/builder/dockerfile/shell/lex_test.go +++ b/builder/dockerfile/shell/lex_test.go @@ -55,8 +55,8 @@ func TestShellParser4EnvVars(t *testing.T) { if expected == "error" { assert.Check(t, is.ErrorContains(err, ""), "input: %q, result: %q", source, newWord) } else { - assert.Check(t, err) - assert.Check(t, is.Equal(newWord, expected)) + assert.Check(t, err, "at line %d of %s", lineCount, fn) + assert.Check(t, is.Equal(newWord, expected), "at line %d of %s", lineCount, fn) } } } From 8687a3f4b8b7ffbc3f32dfe959cb771d662211e6 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 25 May 2018 17:45:25 +0200 Subject: [PATCH 4/6] Add more test-cases for positional parameters Signed-off-by: Sebastiaan van Stijn --- builder/dockerfile/shell/envVarTest | 39 ++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/builder/dockerfile/shell/envVarTest b/builder/dockerfile/shell/envVarTest index 2c0cc1fcf4..005a54e99d 100644 --- a/builder/dockerfile/shell/envVarTest +++ b/builder/dockerfile/shell/envVarTest @@ -18,7 +18,6 @@ A|'hello\there' | hello\there A|'hello\\there' | hello\\there A|"''" | '' A|$. | $. -A|$1 | A|he$1x | hex A|he$.x | he$.x # Next one is different on Windows as $pwd==$PWD @@ -137,3 +136,41 @@ A|${:} | error A|${:-bbb} | error A|${:+bbb} | error +# Positional parameters won't be set: +# http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_01 +A|$1 | +A|${1} | +A|${1:+bbb} | +A|${1:-bbb} | bbb +A|$2 | +A|${2} | +A|${2:+bbb} | +A|${2:-bbb} | bbb +A|$3 | +A|${3} | +A|${3:+bbb} | +A|${3:-bbb} | bbb +A|$4 | +A|${4} | +A|${4:+bbb} | +A|${4:-bbb} | bbb +A|$5 | +A|${5} | +A|${5:+bbb} | +A|${5:-bbb} | bbb +A|$6 | +A|${6} | +A|${6:+bbb} | +A|${6:-bbb} | bbb +A|$7 | +A|${7} | +A|${7:+bbb} | +A|${7:-bbb} | bbb +A|$8 | +A|${8} | +A|${8:+bbb} | +A|${8:-bbb} | bbb +A|$9 | +A|${9} | +A|${9:+bbb} | +A|${9:-bbb} | bbb From 9654e9b6f80e1b931763c04400a19596e256c99a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 25 May 2018 17:59:32 +0200 Subject: [PATCH 5/6] Add detection of "special parameters" for substitution Detect Special parameters as defined in http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_02 Treat these as parameters that are not set, instead of producing an error that a modifier is missing. Signed-off-by: Sebastiaan van Stijn --- builder/dockerfile/shell/envVarTest | 35 ++++++++++++++++++++++++++++ builder/dockerfile/shell/lex.go | 14 ++++++++++- builder/dockerfile/shell/lex_test.go | 8 +++---- 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/builder/dockerfile/shell/envVarTest b/builder/dockerfile/shell/envVarTest index 005a54e99d..6071caebd2 100644 --- a/builder/dockerfile/shell/envVarTest +++ b/builder/dockerfile/shell/envVarTest @@ -174,3 +174,38 @@ A|$9 | A|${9} | A|${9:+bbb} | A|${9:-bbb} | bbb + +# Special parameters won't be set in the Dockerfile: +# http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_02 +A|$@ | +A|${@} | +A|${@:+bbb} | +A|${@:-bbb} | bbb +A|$* | +A|${*} | +A|${*:+bbb} | +A|${*:-bbb} | bbb +A|$# | +A|${#} | +A|${#:+bbb} | +A|${#:-bbb} | bbb +A|$? | +A|${?} | +A|${?:+bbb} | +A|${?:-bbb} | bbb +A|$- | +A|${-} | +A|${-:+bbb} | +A|${-:-bbb} | bbb +A|$$ | +A|${$} | +A|${$:+bbb} | +A|${$:-bbb} | bbb +A|$! | +A|${!} | +A|${!:+bbb} | +A|${!:-bbb} | bbb +A|$0 | +A|${0} | +A|${0:+bbb} | +A|${0:-bbb} | bbb diff --git a/builder/dockerfile/shell/lex.go b/builder/dockerfile/shell/lex.go index 983e5c0517..a51a2665aa 100644 --- a/builder/dockerfile/shell/lex.go +++ b/builder/dockerfile/shell/lex.go @@ -318,7 +318,7 @@ func (sw *shellWord) processName() string { for sw.scanner.Peek() != scanner.EOF { ch := sw.scanner.Peek() - if name.Len() == 0 && unicode.IsDigit(ch) { + if name.Len() == 0 && (unicode.IsDigit(ch) || isSpecialParam(ch)) { ch = sw.scanner.Next() return string(ch) } @@ -332,6 +332,18 @@ func (sw *shellWord) processName() string { return name.String() } +// isSpecialParam checks if the provided character is a special parameters, +// as defined in http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_02 +func isSpecialParam(char rune) bool { + switch char { + case '@', '*', '#', '?', '-', '$', '!', '0': + // Special parameters + // http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_02 + return true + } + return false +} + func (sw *shellWord) getEnv(name string) string { for _, env := range sw.envs { i := strings.Index(env, "=") diff --git a/builder/dockerfile/shell/lex_test.go b/builder/dockerfile/shell/lex_test.go index fa42c92989..4b30c32f2b 100644 --- a/builder/dockerfile/shell/lex_test.go +++ b/builder/dockerfile/shell/lex_test.go @@ -26,13 +26,11 @@ func TestShellParser4EnvVars(t *testing.T) { line := scanner.Text() lineCount++ - // Trim comments and blank lines - i := strings.Index(line, "#") - if i >= 0 { - line = line[:i] + // Skip comments and blank lines + if strings.HasPrefix(line, "#") { + continue } line = strings.TrimSpace(line) - if line == "" { continue } From 2628896b5e813491f8767ec7fa9d0f057ed4a86e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 25 May 2018 18:49:54 +0200 Subject: [PATCH 6/6] Handle multi-digit positional parameters Signed-off-by: Sebastiaan van Stijn --- builder/dockerfile/shell/envVarTest | 21 +++++++++++++++++++++ builder/dockerfile/shell/lex.go | 10 +++++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/builder/dockerfile/shell/envVarTest b/builder/dockerfile/shell/envVarTest index 6071caebd2..08011801c5 100644 --- a/builder/dockerfile/shell/envVarTest +++ b/builder/dockerfile/shell/envVarTest @@ -174,6 +174,22 @@ A|$9 | A|${9} | A|${9:+bbb} | A|${9:-bbb} | bbb +A|$999 | +A|${999} | +A|${999:+bbb} | +A|${999:-bbb} | bbb +A|$999aaa | aaa +A|${999}aaa | aaa +A|${999:+bbb}aaa | aaa +A|${999:-bbb}aaa | bbbaaa +A|$001 | +A|${001} | +A|${001:+bbb} | +A|${001:-bbb} | bbb +A|$001aaa | aaa +A|${001}aaa | aaa +A|${001:+bbb}aaa | aaa +A|${001:-bbb}aaa | bbbaaa # Special parameters won't be set in the Dockerfile: # http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_02 @@ -181,6 +197,11 @@ A|$@ | A|${@} | A|${@:+bbb} | A|${@:-bbb} | bbb +A|$@@@ | @@ +A|$@aaa | aaa +A|${@}aaa | aaa +A|${@:+bbb}aaa | aaa +A|${@:-bbb}aaa | bbbaaa A|$* | A|${*} | A|${*:+bbb} | diff --git a/builder/dockerfile/shell/lex.go b/builder/dockerfile/shell/lex.go index a51a2665aa..0c80900ade 100644 --- a/builder/dockerfile/shell/lex.go +++ b/builder/dockerfile/shell/lex.go @@ -318,7 +318,15 @@ func (sw *shellWord) processName() string { for sw.scanner.Peek() != scanner.EOF { ch := sw.scanner.Peek() - if name.Len() == 0 && (unicode.IsDigit(ch) || isSpecialParam(ch)) { + if name.Len() == 0 && unicode.IsDigit(ch) { + for sw.scanner.Peek() != scanner.EOF && unicode.IsDigit(sw.scanner.Peek()) { + // Keep reading until the first non-digit character, or EOF + ch = sw.scanner.Next() + name.WriteRune(ch) + } + return name.String() + } + if name.Len() == 0 && isSpecialParam(ch) { ch = sw.scanner.Next() return string(ch) }