From 7d46b6d7f12151c317ff3fa2e980d7fd12046830 Mon Sep 17 00:00:00 2001 From: Robert Liebowitz Date: Fri, 13 Sep 2019 05:30:07 -0400 Subject: [PATCH] Avoid panic for missing flag value Currently, in cases where a flag value is required but not passed and short-option handling is enabled, a panic will occur due to a nil pointer dereference. This prevents that situation from occurring, instead propagating the appropriate error. --- app.go | 8 +++---- app_test.go | 61 ++++++++++++++++++++++++++++++++++++++++++------- command.go | 12 +++++----- command_test.go | 43 +++++++++++++++++++--------------- parse.go | 22 ++++++++++-------- 5 files changed, 99 insertions(+), 47 deletions(-) diff --git a/app.go b/app.go index 3300cc5..d657bc4 100644 --- a/app.go +++ b/app.go @@ -218,12 +218,12 @@ func (a *App) Run(arguments []string) (err error) { // always appends the completion flag at the end of the command shellComplete, arguments := checkShellCompleteFlag(a, arguments) - _, err = a.newFlagSet() + set, err := a.newFlagSet() if err != nil { return err } - set, err := parseIter(a, arguments[1:]) + err = parseIter(set, a, arguments[1:]) nerr := normalizeFlags(a.Flags, set) context := NewContext(a, set, nil) if nerr != nil { @@ -344,12 +344,12 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) { } a.Commands = newCmds - _, err = a.newFlagSet() + set, err := a.newFlagSet() if err != nil { return err } - set, err := parseIter(a, ctx.Args().Tail()) + err = parseIter(set, a, ctx.Args().Tail()) nerr := normalizeFlags(a.Flags, set) context := NewContext(a, set, ctx) diff --git a/app_test.go b/app_test.go index 7a73724..fb82457 100644 --- a/app_test.go +++ b/app_test.go @@ -239,11 +239,11 @@ func ExampleApp_Run_bashComplete_withShortFlag() { app.EnableBashCompletion = true app.Flags = []Flag{ &IntFlag{ - Name: "other", + Name: "other", Aliases: []string{"o"}, }, &StringFlag{ - Name: "xyz", + Name: "xyz", Aliases: []string{"x"}, }, } @@ -268,11 +268,11 @@ func ExampleApp_Run_bashComplete_withLongFlag() { app.EnableBashCompletion = true app.Flags = []Flag{ &IntFlag{ - Name: "other", + Name: "other", Aliases: []string{"o"}, }, &StringFlag{ - Name: "xyz", + Name: "xyz", Aliases: []string{"x"}, }, &StringFlag{ @@ -296,11 +296,11 @@ func ExampleApp_Run_bashComplete_withMultipleLongFlag() { app.EnableBashCompletion = true app.Flags = []Flag{ &IntFlag{ - Name: "int-flag", + Name: "int-flag", Aliases: []string{"i"}, }, &StringFlag{ - Name: "string", + Name: "string", Aliases: []string{"s"}, }, &StringFlag{ @@ -326,7 +326,7 @@ func ExampleApp_Run_bashComplete() { os.Args = []string{"greet", "--generate-bash-completion"} app := &App{ - Name: "greet", + Name: "greet", EnableBashCompletion: true, Commands: []*Command{ { @@ -638,6 +638,17 @@ func TestApp_UseShortOptionHandling(t *testing.T) { expect(t, name, expected) } +func TestApp_UseShortOptionHandling_missing_value(t *testing.T) { + app := NewApp() + app.UseShortOptionHandling = true + app.Flags = []Flag{ + &StringFlag{Name: "name", Aliases: []string{"n"}}, + } + + err := app.Run([]string{"", "-n"}) + expect(t, err, errors.New("flag needs an argument: -n")) +} + func TestApp_UseShortOptionHandlingCommand(t *testing.T) { var one, two bool var name string @@ -667,6 +678,21 @@ func TestApp_UseShortOptionHandlingCommand(t *testing.T) { expect(t, name, expected) } +func TestApp_UseShortOptionHandlingCommand_missing_value(t *testing.T) { + app := NewApp() + app.UseShortOptionHandling = true + command := &Command{ + Name: "cmd", + Flags: []Flag{ + &StringFlag{Name: "name", Aliases: []string{"n"}}, + }, + } + app.Commands = []*Command{command} + + err := app.Run([]string{"", "cmd", "-n"}) + expect(t, err, errors.New("flag needs an argument: -n")) +} + func TestApp_UseShortOptionHandlingSubCommand(t *testing.T) { var one, two bool var name string @@ -692,7 +718,7 @@ func TestApp_UseShortOptionHandlingSubCommand(t *testing.T) { }, } command.Subcommands = []*Command{subCommand} - app.Commands = []*Command{command} + app.Commands = []*Command{command} err := app.Run([]string{"", "cmd", "sub", "-on", expected}) expect(t, err, nil) @@ -701,6 +727,25 @@ func TestApp_UseShortOptionHandlingSubCommand(t *testing.T) { expect(t, name, expected) } +func TestApp_UseShortOptionHandlingSubCommand_missing_value(t *testing.T) { + app := NewApp() + app.UseShortOptionHandling = true + command := &Command{ + Name: "cmd", + } + subCommand := &Command{ + Name: "sub", + Flags: []Flag{ + &StringFlag{Name: "name", Aliases: []string{"n"}}, + }, + } + command.Subcommands = []*Command{subCommand} + app.Commands = []*Command{command} + + err := app.Run([]string{"", "cmd", "sub", "-n"}) + expect(t, err, errors.New("flag needs an argument: -n")) +} + func TestApp_Float64Flag(t *testing.T) { var meters float64 diff --git a/command.go b/command.go index 3a1f21b..fcaafb6 100644 --- a/command.go +++ b/command.go @@ -175,16 +175,16 @@ func (c *Command) useShortOptionHandling() bool { } func (c *Command) parseFlags(args Args) (*flag.FlagSet, error) { - if c.SkipFlagParsing { - set, err := c.newFlagSet() - if err != nil { - return nil, err - } + set, err := c.newFlagSet() + if err != nil { + return nil, err + } + if c.SkipFlagParsing { return set, set.Parse(append([]string{"--"}, args.Tail()...)) } - set, err := parseIter(c, args.Tail()) + err = parseIter(set, c, args.Tail()) if err != nil { return nil, err } diff --git a/command_test.go b/command_test.go index 25e1ca5..f43b4ae 100644 --- a/command_test.go +++ b/command_test.go @@ -52,7 +52,7 @@ func TestParseAndRunShortOpts(t *testing.T) { cases := []struct { testArgs args expectedErr error - expectedArgs *args + expectedArgs Args }{ {testArgs: args{"foo", "test", "-a"}, expectedErr: nil, expectedArgs: &args{}}, {testArgs: args{"foo", "test", "-c", "arg1", "arg2"}, expectedErr: nil, expectedArgs: &args{"arg1", "arg2"}}, @@ -61,28 +61,33 @@ func TestParseAndRunShortOpts(t *testing.T) { {testArgs: args{"foo", "test", "-af"}, expectedErr: nil, expectedArgs: &args{}}, {testArgs: args{"foo", "test", "-cf"}, expectedErr: nil, expectedArgs: &args{}}, {testArgs: args{"foo", "test", "-acf"}, expectedErr: nil, expectedArgs: &args{}}, - {testArgs: args{"foo", "test", "-invalid"}, expectedErr: errors.New("flag provided but not defined: -invalid"), expectedArgs: &args{}}, + {testArgs: args{"foo", "test", "-invalid"}, expectedErr: errors.New("flag provided but not defined: -invalid"), expectedArgs: nil}, {testArgs: args{"foo", "test", "-acf", "arg1", "-invalid"}, expectedErr: nil, expectedArgs: &args{"arg1", "-invalid"}}, - } - - var args Args - cmd := &Command{ - Name: "test", - Usage: "this is for testing", - Description: "testing", - Action: func(c *Context) error { - args = c.Args() - return nil - }, - UseShortOptionHandling: true, - Flags: []Flag{ - &BoolFlag{Name: "abc", Aliases: []string{"a"}}, - &BoolFlag{Name: "cde", Aliases: []string{"c"}}, - &BoolFlag{Name: "fgh", Aliases: []string{"f"}}, - }, + {testArgs: args{"foo", "test", "-acfi", "not-arg", "arg1", "-invalid"}, expectedErr: nil, expectedArgs: &args{"arg1", "-invalid"}}, + {testArgs: args{"foo", "test", "-i", "ivalue"}, expectedErr: nil, expectedArgs: &args{}}, + {testArgs: args{"foo", "test", "-i", "ivalue", "arg1"}, expectedErr: nil, expectedArgs: &args{"arg1"}}, + {testArgs: args{"foo", "test", "-i"}, expectedErr: errors.New("flag needs an argument: -i"), expectedArgs: nil}, } for _, c := range cases { + var args Args + cmd := &Command{ + Name: "test", + Usage: "this is for testing", + Description: "testing", + Action: func(c *Context) error { + args = c.Args() + return nil + }, + UseShortOptionHandling: true, + Flags: []Flag{ + &BoolFlag{Name: "abc", Aliases: []string{"a"}}, + &BoolFlag{Name: "cde", Aliases: []string{"c"}}, + &BoolFlag{Name: "fgh", Aliases: []string{"f"}}, + &StringFlag{Name: "ijk", Aliases:[]string{"i"}}, + }, + } + app := NewApp() app.Commands = []*Command{cmd} diff --git a/parse.go b/parse.go index 865accf..2c2005c 100644 --- a/parse.go +++ b/parse.go @@ -14,22 +14,17 @@ type iterativeParser interface { // iteratively catch parsing errors. This way we achieve LR parsing without // transforming any arguments. Otherwise, there is no way we can discriminate // combined short options from common arguments that should be left untouched. -func parseIter(ip iterativeParser, args []string) (*flag.FlagSet, error) { +func parseIter(set *flag.FlagSet, ip iterativeParser, args []string) error { for { - set, err := ip.newFlagSet() - if err != nil { - return nil, err - } - - err = set.Parse(args) + err := set.Parse(args) if !ip.useShortOptionHandling() || err == nil { - return set, err + return err } errStr := err.Error() trimmed := strings.TrimPrefix(errStr, "flag provided but not defined: ") if errStr == trimmed { - return nil, err + return err } // regenerate the initial args with the split short opts @@ -42,7 +37,7 @@ func parseIter(ip iterativeParser, args []string) (*flag.FlagSet, error) { shortOpts := splitShortOptions(set, trimmed) if len(shortOpts) == 1 { - return nil, err + return err } // add each short option and all remaining arguments @@ -50,6 +45,13 @@ func parseIter(ip iterativeParser, args []string) (*flag.FlagSet, error) { newArgs = append(newArgs, args[i+1:]...) args = newArgs } + + // Since custom parsing failed, replace the flag set before retrying + newSet, err := ip.newFlagSet() + if err != nil { + return err + } + *set = *newSet } }