Skip to content
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

Jump to bottom

Windows Terminal crashes when nested command does not have command field #9448

Closed
waf opened this issue Mar 11, 2021 · 4 comments · Fixed by #9495
Closed

Windows Terminal crashes when nested command does not have command field #9448

waf opened this issue Mar 11, 2021 · 4 comments · Fixed by #9495
Labels
Area-CmdPal Command Palette issues and features Area-Settings Issues related to settings and customizability, for console or terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Milestone
Terminal v2.0

Comments

@waf
Copy link
Contributor

waf commented Mar 11, 2021

Crash happens in the latest release of Windows Terminal 1.6 and Preview 1.7.

Environment

Windows build number: Microsoft Windows NT 10.0.19042.0
Windows Terminal version (if applicable):
  Version: 1.6.10571.0
  Version: 1.7.572.0

Steps to reproduce

  1. Add the following command to the actions array:
{
    "name": "Cause a crash",
    "commands": [
        { "name": "hello" },
        { "name": "world" }
    ]
}
  1. Try to run "Cause a crash" from the Command Palette
  2. Windows Terminal crashes

From some experimentation, the crash is caused by the missing "command" field in the nested commands. While the example JSON above is not real-world, I originally found this when I typo'd the "command" field name, which could also happen to other users.

It might be related to #8422, but it's still happening in the latest release of Windows Terminal (both 1.6 and 1.7 preview).

I've submitted a Feedback Hub reproduction with the exact same title as this GitHub issue, but I wasn't signed in, so it appears I didn't get a hyperlink or ID. Let me know if you'd like me to submit another one (signed in this time).
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Mar 11, 2021
@zadjii-msft
Copy link
Member

Welp that's a pretty trivial repro. We'll make sure to add it to test cases so that this doesn't regress in the future. Thanks!

@zadjii-msft zadjii-msft added Area-CmdPal Command Palette issues and features Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. labels Mar 11, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Mar 11, 2021
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Mar 11, 2021
@zadjii-msft zadjii-msft added the Help Wanted We encourage anyone to jump in on these. label Mar 11, 2021
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Mar 13, 2021
@Don-Vito
Copy link
Contributor

Don-Vito commented Mar 14, 2021 •
edited
Loading

It doesn't seem related to #8422, as that PR modified only the CmdPal (fixed an underflow there) and here it is a bug in the ShortcutActionDispatch that doesn't verify the parameters.

I will add protection in ShortcutActionDispatch and also improve the parsing of the commands.

@Don-Vito Don-Vito mentioned this issue Mar 14, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Mar 14, 2021
@ghost ghost closed this as completed in #9495 Mar 15, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Mar 15, 2021
ghost pushed a commit that referenced this issue Mar 15, 2021
@Don-Vito
Invalidate nested command with no valid subcommands (#9495)
28da6c4 
Currently, when loading command with sub-commands that fail to parse,
we result with command that:
* Is not considered nested (has no sub-commands)
* Has no action of its own

The commit contains a few changes:
1. Protection in the dispatch that will prevent NPE
2. Change in the command parsing that will no load
a command if all its sub-commands failed to parse
3. We will add a warning in this case (the solution is somewhat
hacky, due to the hack that was there previously)

When such command is passed to a dispatch we crash with NPE.

Closes #9448
DHowett pushed a commit that referenced this issue Mar 15, 2021
@Don-Vito @DHowett
Invalidate nested command with no valid subcommands (#9495)
0026b67 
Currently, when loading command with sub-commands that fail to parse,
we result with command that:
* Is not considered nested (has no sub-commands)
* Has no action of its own

The commit contains a few changes:
1. Protection in the dispatch that will prevent NPE
2. Change in the command parsing that will no load
a command if all its sub-commands failed to parse
3. We will add a warning in this case (the solution is somewhat
hacky, due to the hack that was there previously)

When such command is passed to a dispatch we crash with NPE.

Closes #9448

(cherry picked from commit 28da6c4)
DHowett pushed a commit that referenced this issue Mar 15, 2021
@Don-Vito @DHowett
Invalidate nested command with no valid subcommands (#9495)
a6af9a5 
Currently, when loading command with sub-commands that fail to parse,
we result with command that:
* Is not considered nested (has no sub-commands)
* Has no action of its own

The commit contains a few changes:
1. Protection in the dispatch that will prevent NPE
2. Change in the command parsing that will no load
a command if all its sub-commands failed to parse
3. We will add a warning in this case (the solution is somewhat
hacky, due to the hack that was there previously)

When such command is passed to a dispatch we crash with NPE.

Closes #9448

(cherry picked from commit 28da6c4)
@ghost
Copy link

ghost commented Apr 14, 2021

🎉This issue was addressed in #9495, which has now been successfully released as Windows Terminal v1.7.1033.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Apr 14, 2021

🎉This issue was addressed in #9495, which has now been successfully released as Windows Terminal Preview v1.8.1032.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-CmdPal Command Palette issues and features Area-Settings Issues related to settings and customizability, for console or terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Projects
None yet
Milestone
Terminal v2.0
Development

Successfully merging a pull request may close this issue.

Invalidate nested command with no valid subcommands Don-Vito/terminal
4 participants
@waf @DHowett @Don-Vito @zadjii-msft