From c8db46ad053daa297bf3b3f1a4e8b6e840d1e81f 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 Upstream-commit: 955a6ad95f7891a45692d975793abf1eeb07cdd5 Component: engine --- .../builder/dockerfile/shell/envVarTest | 11 ++++++++++ .../engine/builder/dockerfile/shell/lex.go | 20 ++++++++++++++----- .../builder/dockerfile/shell/lex_test.go | 2 +- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/components/engine/builder/dockerfile/shell/envVarTest b/components/engine/builder/dockerfile/shell/envVarTest index 946b278592..677b33d9a7 100644 --- a/components/engine/builder/dockerfile/shell/envVarTest +++ b/components/engine/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/components/engine/builder/dockerfile/shell/lex.go b/components/engine/builder/dockerfile/shell/lex.go index bd3fac525a..d0d6f53e66 100644 --- a/components/engine/builder/dockerfile/shell/lex.go +++ b/components/engine/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/components/engine/builder/dockerfile/shell/lex_test.go b/components/engine/builder/dockerfile/shell/lex_test.go index f38da2026f..765ae14b25 100644 --- a/components/engine/builder/dockerfile/shell/lex_test.go +++ b/components/engine/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 1f9af8d4f95f861f4bc323f33cd768f759692925 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 Upstream-commit: 334bf3ea76004d0abe02dd1698989f9eaf87a86a Component: engine --- .../engine/builder/dockerfile/shell/envVarTest | 15 +++++++++++---- .../engine/builder/dockerfile/shell/lex.go | 17 ++++++++--------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/components/engine/builder/dockerfile/shell/envVarTest b/components/engine/builder/dockerfile/shell/envVarTest index 677b33d9a7..2c0cc1fcf4 100644 --- a/components/engine/builder/dockerfile/shell/envVarTest +++ b/components/engine/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/components/engine/builder/dockerfile/shell/lex.go b/components/engine/builder/dockerfile/shell/lex.go index d0d6f53e66..983e5c0517 100644 --- a/components/engine/builder/dockerfile/shell/lex.go +++ b/components/engine/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 ad9f08c7259aa480579f8607fb20e529f1d6cdd6 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 Upstream-commit: b80e0309d220268a2b9e6aec5bb05d7af330e591 Component: engine --- components/engine/builder/dockerfile/shell/lex_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/engine/builder/dockerfile/shell/lex_test.go b/components/engine/builder/dockerfile/shell/lex_test.go index 765ae14b25..fa42c92989 100644 --- a/components/engine/builder/dockerfile/shell/lex_test.go +++ b/components/engine/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 0ed9886f87c0aa7f092c5350e54728834a86753b 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 Upstream-commit: 8687a3f4b8b7ffbc3f32dfe959cb771d662211e6 Component: engine --- .../builder/dockerfile/shell/envVarTest | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/components/engine/builder/dockerfile/shell/envVarTest b/components/engine/builder/dockerfile/shell/envVarTest index 2c0cc1fcf4..005a54e99d 100644 --- a/components/engine/builder/dockerfile/shell/envVarTest +++ b/components/engine/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 45e37846106deda69399e3198e0c671c4b416b58 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 Upstream-commit: 9654e9b6f80e1b931763c04400a19596e256c99a Component: engine --- .../builder/dockerfile/shell/envVarTest | 35 +++++++++++++++++++ .../engine/builder/dockerfile/shell/lex.go | 14 +++++++- .../builder/dockerfile/shell/lex_test.go | 8 ++--- 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/components/engine/builder/dockerfile/shell/envVarTest b/components/engine/builder/dockerfile/shell/envVarTest index 005a54e99d..6071caebd2 100644 --- a/components/engine/builder/dockerfile/shell/envVarTest +++ b/components/engine/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/components/engine/builder/dockerfile/shell/lex.go b/components/engine/builder/dockerfile/shell/lex.go index 983e5c0517..a51a2665aa 100644 --- a/components/engine/builder/dockerfile/shell/lex.go +++ b/components/engine/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/components/engine/builder/dockerfile/shell/lex_test.go b/components/engine/builder/dockerfile/shell/lex_test.go index fa42c92989..4b30c32f2b 100644 --- a/components/engine/builder/dockerfile/shell/lex_test.go +++ b/components/engine/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 87e43e176248537b74f9d5f919d6fd4c801ced7e 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 Upstream-commit: 2628896b5e813491f8767ec7fa9d0f057ed4a86e Component: engine --- .../builder/dockerfile/shell/envVarTest | 21 +++++++++++++++++++ .../engine/builder/dockerfile/shell/lex.go | 10 ++++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/components/engine/builder/dockerfile/shell/envVarTest b/components/engine/builder/dockerfile/shell/envVarTest index 6071caebd2..08011801c5 100644 --- a/components/engine/builder/dockerfile/shell/envVarTest +++ b/components/engine/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/components/engine/builder/dockerfile/shell/lex.go b/components/engine/builder/dockerfile/shell/lex.go index a51a2665aa..0c80900ade 100644 --- a/components/engine/builder/dockerfile/shell/lex.go +++ b/components/engine/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) }