-
Notifications
You must be signed in to change notification settings - Fork 6.6k
Fix stack overflow in AddJSONFlags #4614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
pkg/cmdutil/json_flags.go
Outdated
| @@ -77,7 +77,7 @@ func AddJSONFlags(cmd *cobra.Command, exportTarget *Exporter, fields []string) { | |||
| sort.Strings(fields) | |||
| return JSONFlagError{fmt.Errorf("Specify one or more comma-separated fields for `--json`:\n %s", strings.Join(fields, "\n "))} | |||
| } | |||
| return c.Parent().FlagErrorFunc()(c, e) | |||
| return cmd.Parent().FlagErrorFunc()(c, e) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a workaround, but there is still a problem of calling the parent FlagErrorFunc but passing it the wrong *cobra.Command reference as argument.
The core of the problem is that FlagErrorFunc is inherited by subcommands, but that up to now we've never used AddJSONFlags on a command that has subcommands. This FlagErrorFunc here is only supposed to be applied on the command with the --json flag, but accidentally applies to subcommands. Furthermore, when this custom FlagErrorFunc tried to invoke the parent FlagErrorFunc, it passed in the wrong argument to it.
I think a proper fix would be twofold:
- Only ever raise the JSONFlagError if
c == cmd(in addition to the already existing condition); - When calling
FlagErrorFunconc.Parent(), you should pass the same parent as argument to the func.
Finally, for good measure, you could also check if c.Parent() even exists (root commands will not have a parent) and only call its FlagErrorFunc if it does. We don't yet use AddJSONFlags on any root command, but in case we do, it would be nice if it doesn't panic.
Does all this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially had the passed argument set to c.Parent() but then it seemed to me that this is rather meant to be the command on which the error occured.
This might also make more sense for 1. where the json error should only be raised on the command where the flag was registered as it isn't persistent and this way the fields are also correct.
Parent check makes sense (and is also done in the default func).
So i would agree on both the checks, but i am not so sure about the passed argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but then it seemed to me that this is rather meant to be the command on which the error occured.
Ah! You might be right. Although, then I really don't know how Cobra expects us to override the FlagErrorFunc down the line while still respecting potentially defined FlagErrorFunc handlers of parent commands 🤔 But since we don't use SetFlagErrorFunc heavily across the codebase, I think your approach is actually fine and makes more sense than what I suggested.
67065ef to
7c443ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/cmdutil/json_flags.go
fix #4612