diff --git a/builder/dispatchers.go b/builder/dispatchers.go index 2bc56e46e..b00268ed6 100644 --- a/builder/dispatchers.go +++ b/builder/dispatchers.go @@ -39,7 +39,7 @@ func nullDispatch(b *Builder, args []string, attributes map[string]bool, origina // func env(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) == 0 { - return fmt.Errorf("ENV is missing arguments") + return fmt.Errorf("ENV requires at least one argument") } if len(args)%2 != 0 { @@ -78,7 +78,7 @@ func env(b *Builder, args []string, attributes map[string]bool, original string) // Sets the maintainer metadata. func maintainer(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) != 1 { - return fmt.Errorf("MAINTAINER requires only one argument") + return fmt.Errorf("MAINTAINER requires exactly one argument") } b.maintainer = args[0] @@ -159,6 +159,10 @@ func from(b *Builder, args []string, attributes map[string]bool, original string // cases. // func onbuild(b *Builder, args []string, attributes map[string]bool, original string) error { + if len(args) == 0 { + return fmt.Errorf("ONBUILD requires at least one argument") + } + triggerInstruction := strings.ToUpper(strings.TrimSpace(args[0])) switch triggerInstruction { case "ONBUILD": @@ -327,6 +331,10 @@ func entrypoint(b *Builder, args []string, attributes map[string]bool, original func expose(b *Builder, args []string, attributes map[string]bool, original string) error { portsTab := args + if len(args) == 0 { + return fmt.Errorf("EXPOSE requires at least one argument") + } + if b.Config.ExposedPorts == nil { b.Config.ExposedPorts = make(nat.PortSet) } @@ -373,7 +381,7 @@ func user(b *Builder, args []string, attributes map[string]bool, original string // func volume(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) == 0 { - return fmt.Errorf("Volume cannot be empty") + return fmt.Errorf("VOLUME requires at least one argument") } if b.Config.Volumes == nil { diff --git a/builder/evaluator.go b/builder/evaluator.go index b76c7f29b..3806b3837 100644 --- a/builder/evaluator.go +++ b/builder/evaluator.go @@ -54,10 +54,12 @@ var replaceEnvAllowed = map[string]struct{}{ "user": {}, } -var evaluateTable map[string]func(*Builder, []string, map[string]bool, string) error +// EvaluateTable is public so that we can get the list of Dockerfile +// commands from within the test cases +var EvaluateTable map[string]func(*Builder, []string, map[string]bool, string) error func init() { - evaluateTable = map[string]func(*Builder, []string, map[string]bool, string) error{ + EvaluateTable = map[string]func(*Builder, []string, map[string]bool, string) error{ "env": env, "maintainer": maintainer, "add": add, @@ -224,7 +226,7 @@ func (b *Builder) readDockerfile(origFile string) error { // Child[Node, Node, Node] where Child is from parser.Node.Children and each // node comes from parser.Node.Next. This forms a "line" with a statement and // arguments and we process them in this normalized form by hitting -// evaluateTable with the leaf nodes of the command and the Builder object. +// EvaluateTable with the leaf nodes of the command and the Builder object. // // ONBUILD is a special case; in this case the parser will emit: // Child[Node, Child[Node, Node...]] where the first node is the literal @@ -240,6 +242,9 @@ func (b *Builder) dispatch(stepN int, ast *parser.Node) error { msg := fmt.Sprintf("Step %d : %s", stepN, strings.ToUpper(cmd)) if cmd == "onbuild" { + if ast.Next == nil { + return fmt.Errorf("ONBUILD requires at least one argument") + } ast = ast.Next.Children[0] strs = append(strs, ast.Value) msg += " " + ast.Value @@ -277,7 +282,7 @@ func (b *Builder) dispatch(stepN int, ast *parser.Node) error { // XXX yes, we skip any cmds that are not valid; the parser should have // picked these out already. - if f, ok := evaluateTable[cmd]; ok { + if f, ok := EvaluateTable[cmd]; ok { return f(b, strList, attrs, original) } diff --git a/builder/parser/line_parsers.go b/builder/parser/line_parsers.go index 8a94e1e5d..c7fed13db 100644 --- a/builder/parser/line_parsers.go +++ b/builder/parser/line_parsers.go @@ -30,6 +30,10 @@ func parseIgnore(rest string) (*Node, map[string]bool, error) { // ONBUILD RUN foo bar -> (onbuild (run foo bar)) // func parseSubCommand(rest string) (*Node, map[string]bool, error) { + if rest == "" { + return nil, nil, nil + } + _, child, err := parseLine(rest) if err != nil { return nil, nil, err @@ -133,7 +137,7 @@ func parseEnv(rest string) (*Node, map[string]bool, error) { } if len(words) == 0 { - return nil, nil, fmt.Errorf("ENV must have some arguments") + return nil, nil, fmt.Errorf("ENV requires at least one argument") } // Old format (ENV name value) @@ -181,6 +185,10 @@ func parseEnv(rest string) (*Node, map[string]bool, error) { // parses a whitespace-delimited set of arguments. The result is effectively a // linked list of string arguments. func parseStringsWhitespaceDelimited(rest string) (*Node, map[string]bool, error) { + if rest == "" { + return nil, nil, nil + } + node := &Node{} rootnode := node prevnode := node @@ -201,6 +209,9 @@ func parseStringsWhitespaceDelimited(rest string) (*Node, map[string]bool, error // parsestring just wraps the string in quotes and returns a working node. func parseString(rest string) (*Node, map[string]bool, error) { + if rest == "" { + return nil, nil, nil + } n := &Node{} n.Value = rest return n, nil, nil @@ -235,7 +246,9 @@ func parseJSON(rest string) (*Node, map[string]bool, error) { // so, passes to parseJSON; if not, quotes the result and returns a single // node. func parseMaybeJSON(rest string) (*Node, map[string]bool, error) { - rest = strings.TrimSpace(rest) + if rest == "" { + return nil, nil, nil + } node, attrs, err := parseJSON(rest) @@ -255,8 +268,6 @@ func parseMaybeJSON(rest string) (*Node, map[string]bool, error) { // so, passes to parseJSON; if not, attmpts to parse it as a whitespace // delimited string. func parseMaybeJSONToList(rest string) (*Node, map[string]bool, error) { - rest = strings.TrimSpace(rest) - node, attrs, err := parseJSON(rest) if err == nil { diff --git a/builder/parser/parser.go b/builder/parser/parser.go index 957ecd629..b8685767b 100644 --- a/builder/parser/parser.go +++ b/builder/parser/parser.go @@ -3,7 +3,6 @@ package parser import ( "bufio" - "fmt" "io" "regexp" "strings" @@ -78,10 +77,6 @@ func parseLine(line string) (string, *Node, error) { return "", nil, err } - if len(args) == 0 { - return "", nil, fmt.Errorf("Instruction %q is empty; cannot continue", cmd) - } - node := &Node{} node.Value = cmd diff --git a/builder/parser/testfiles-negative/empty-instruction/Dockerfile b/builder/parser/testfiles-negative/empty-instruction/Dockerfile deleted file mode 100644 index 74e625a40..000000000 --- a/builder/parser/testfiles-negative/empty-instruction/Dockerfile +++ /dev/null @@ -1,8 +0,0 @@ -FROM dockerfile/rabbitmq - -RUN - rabbitmq-plugins enable \ - rabbitmq_shovel \ - rabbitmq_shovel_management \ - rabbitmq_federation \ - rabbitmq_federation_management diff --git a/builder/parser/testfiles-negative/html-page-yes-really-thanks-lk4d4/Dockerfile b/builder/parser/testfiles-negative/html-page-yes-really-thanks-lk4d4/Dockerfile deleted file mode 100644 index 90531a4b3..000000000 --- a/builder/parser/testfiles-negative/html-page-yes-really-thanks-lk4d4/Dockerfile +++ /dev/null @@ -1,2 +0,0 @@ - - diff --git a/builder/parser/utils.go b/builder/parser/utils.go index b64f67f11..a60ad129f 100644 --- a/builder/parser/utils.go +++ b/builder/parser/utils.go @@ -1,7 +1,6 @@ package parser import ( - "fmt" "strconv" "strings" ) @@ -50,17 +49,19 @@ func fullDispatch(cmd, args string) (*Node, map[string]bool, error) { // splitCommand takes a single line of text and parses out the cmd and args, // which are used for dispatching to more exact parsing functions. func splitCommand(line string) (string, string, error) { + var args string + // Make sure we get the same results irrespective of leading/trailing spaces cmdline := TOKEN_WHITESPACE.Split(strings.TrimSpace(line), 2) + cmd := strings.ToLower(cmdline[0]) - if len(cmdline) != 2 { - return "", "", fmt.Errorf("We do not understand this file. Please ensure it is a valid Dockerfile. Parser error at %q", line) + if len(cmdline) == 2 { + args = strings.TrimSpace(cmdline[1]) } - cmd := strings.ToLower(cmdline[0]) // the cmd should never have whitespace, but it's possible for the args to // have trailing whitespace. - return cmd, strings.TrimSpace(cmdline[1]), nil + return cmd, args, nil } // covers comments and empty lines. Lines should be trimmed before passing to diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index a692a2b87..49c7db4a4 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -19,6 +19,7 @@ import ( "text/template" "time" + "github.com/docker/docker/builder" "github.com/docker/docker/pkg/archive" ) @@ -49,14 +50,14 @@ func TestBuildEmptyWhitespace(t *testing.T) { name, ` FROM busybox - RUN + COPY quux \ bar `, true) if err == nil { - t.Fatal("no error when dealing with a RUN statement with no content on the same line") + t.Fatal("no error when dealing with a COPY statement with no content on the same line") } logDone("build - statements with whitespace and no content should generate a parse error") @@ -4726,9 +4727,9 @@ func TestBuildSpaces(t *testing.T) { name := "testspaces" defer deleteImages(name) - ctx, err := fakeContext("FROM busybox\nRUN\n", + ctx, err := fakeContext("FROM busybox\nCOPY\n", map[string]string{ - "Dockerfile": "FROM busybox\nRUN\n", + "Dockerfile": "FROM busybox\nCOPY\n", }) if err != nil { t.Fatal(err) @@ -4739,7 +4740,7 @@ func TestBuildSpaces(t *testing.T) { t.Fatal("Build 1 was supposed to fail, but didn't") } - ctx.Add("Dockerfile", "FROM busybox\nRUN ") + ctx.Add("Dockerfile", "FROM busybox\nCOPY ") if _, err2 = buildImageFromContext(name, ctx, false); err2 == nil { t.Fatal("Build 2 was supposed to fail, but didn't") } @@ -4753,7 +4754,7 @@ func TestBuildSpaces(t *testing.T) { t.Fatalf("Build 2's error wasn't the same as build 1's\n1:%s\n2:%s", err1, err2) } - ctx.Add("Dockerfile", "FROM busybox\n RUN") + ctx.Add("Dockerfile", "FROM busybox\n COPY") if _, err2 = buildImageFromContext(name, ctx, false); err2 == nil { t.Fatal("Build 3 was supposed to fail, but didn't") } @@ -4767,7 +4768,7 @@ func TestBuildSpaces(t *testing.T) { t.Fatalf("Build 3's error wasn't the same as build 1's\n1:%s\n3:%s", err1, err2) } - ctx.Add("Dockerfile", "FROM busybox\n RUN ") + ctx.Add("Dockerfile", "FROM busybox\n COPY ") if _, err2 = buildImageFromContext(name, ctx, false); err2 == nil { t.Fatal("Build 4 was supposed to fail, but didn't") } @@ -4822,3 +4823,50 @@ func TestBuildVolumeFileExistsinContainer(t *testing.T) { logDone("build - errors when volume is specified where a file exists") } + +func TestBuildMissingArgs(t *testing.T) { + // Test to make sure that all Dockerfile commands (except the ones listed + // in skipCmds) will generate an error if no args are provided. + // Note: INSERT is deprecated so we exclude it because of that. + + skipCmds := map[string]struct{}{ + "CMD": {}, + "RUN": {}, + "ENTRYPOINT": {}, + "INSERT": {}, + } + + defer deleteAllContainers() + + for cmd := range builder.EvaluateTable { + var dockerfile string + + cmd = strings.ToUpper(cmd) + + if _, ok := skipCmds[cmd]; ok { + continue + } + + if cmd == "FROM" { + dockerfile = cmd + } else { + // Add FROM to make sure we don't complain about it missing + dockerfile = "FROM busybox\n" + cmd + } + + ctx, err := fakeContext(dockerfile, map[string]string{}) + if err != nil { + t.Fatal(err) + } + defer ctx.Close() + var out string + if out, err = buildImageFromContext("args", ctx, true); err == nil { + t.Fatalf("%s was supposed to fail. Out:%s", cmd, out) + } + if !strings.Contains(err.Error(), cmd+" requires") { + t.Fatalf("%s returned the wrong type of error:%s", cmd, err) + } + } + + logDone("build - verify missing args") +}