Skip to content

Fix stack overflow in AddJSONFlags#4614

Merged
mislav merged 2 commits intocli:trunkfrom
rsteube:fix-json-flags-loop
Oct 27, 2021
Merged

Fix stack overflow in AddJSONFlags#4614
mislav merged 2 commits intocli:trunkfrom
rsteube:fix-json-flags-loop

Conversation

@rsteube
Copy link
Contributor

@rsteube rsteube commented Oct 26, 2021

fix #4612

@rsteube rsteube requested a review from a team as a code owner October 26, 2021 08:32
@rsteube rsteube requested review from samcoe and removed request for a team October 26, 2021 08:32
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)
Copy link
Contributor

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:

  1. Only ever raise the JSONFlagError if c == cmd (in addition to the already existing condition);
  2. When calling FlagErrorFunc on c.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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@rsteube rsteube force-pushed the fix-json-flags-loop branch from 67065ef to 7c443ab Compare October 26, 2021 13:28
@mislav mislav enabled auto-merge (squash) October 26, 2021 14:12
@mislav mislav merged commit d794a92 into cli:trunk Oct 27, 2021
@rsteube rsteube deleted the fix-json-flags-loop branch October 27, 2021 12:57
Copy link

@Txward Txward left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stack overflow caused by AddJSONFlags in v2.2.0

3 participants