Skip to content

chore: prevent nil dereferences on cmd handlers #8319

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 5, 2023
Merged

chore: prevent nil dereferences on cmd handlers #8319

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b453f7
chore: detect nil cmd handlers
Emyrk Jul 5, 2023
9fa88f8
Fix typo
Emyrk Jul 5, 2023
7de7e77
Parallelize tests
Emyrk Jul 5, 2023
63073c4
Expand comment lanugage on the nil handler
Emyrk Jul 5, 2023
eaa2218
Merge remote-tracking branch 'origin/main' into stevenmasley/cmd_help
Emyrk Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Expand comment lanugage on the nil handler
  • Loading branch information
@Emyrk
Emyrk committed Jul 5, 2023
commit 63073c4e07ae9fa24ef92ebb2367c14c216fc397
7 changes: 7 additions & 0 deletions cli/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,13 @@ func (r *RootCmd) Command(subcommands []*clibase.Cmd) (*clibase.Cmd, error) {
if h == nil {
// We should never have a nil handler, but if we do, do not
// wrap it. Wrapping it just hides a nil pointer dereference.
// If a nil handler exists, this is a developer bug. If no handler
// is required for a command such as command grouping (e.g. `users'
// and 'groups'), then the handler should be set to the helper
// function.
// func(inv *clibase.Invocation) error {
// return inv.Command.HelpHandler(inv)
// }
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explains what, but not why? I'm guessing it's the call to h below, but why don't we just default to h being func(inv *clibase.Invocation) error { return inv.Command.HelpHandler(inv) } in this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mafredri I was wondering if defaulting to the help command would be a good solution.

Atm we manually invoke the help on all cmds except this one. So if I made that change it would be a new behavior that is probably fine, but being explicit is also pretty easy. Now that I have a unit test that will fail if you add a new cmd without a help, I figured I'd keep it being manual.

I'll update the comment with more explanation as to why. The unit test failure has that language near it

}
cmd.Handler = func(i *clibase.Invocation) error {
Expand Down