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.

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

Quickly invoking PowerShell menu completion multiple times crashes Terminal #17220

Closed
krzysdz opened this issue May 8, 2024 · 0 comments · Fixed by #17221
Closed

Quickly invoking PowerShell menu completion multiple times crashes Terminal #17220

krzysdz opened this issue May 8, 2024 · 0 comments · Fixed by #17221
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.

Comments

@krzysdz
Copy link
Contributor

krzysdz commented May 8, 2024

Windows Terminal version

49e4eea

Windows build number

10.0.19045.4291

Other Software

No response

Steps to reproduce

  1. Enable experimental shell completion menu ("experimental.enableShellCompletionMenu": true)
  2. Quickly invoke shell completion multiple times, so that Terminal tries to parse JSON with completion data at the same time from multiple threads in one of the following ways:

Expected Behavior

Completion menu is shown or nothing happens

Actual Behavior

Terminal crashes or throws an exception or some kind of invalid memory access error, because JSON::CharReader in Command::ParsePowerShellMenuComplete is static and shared between threads:

static std::unique_ptr<Json::CharReader> reader{ Json::CharReaderBuilder{}.newCharReader() };

Reproduction in one of the versions affected by #17204:

2024-05-08.22-04-15.mp4

Reproduction in other Terminal versions and demonstration of a possible fix (no menu is shown, because of the huge JSON, but the Terminal doesn't crash):

2024-05-08.23-51-43.mp4
@krzysdz krzysdz added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 8, 2024
@zadjii-msft zadjii-msft added In-PR This issue has a related PR Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 9, 2024
github-merge-queue bot pushed a commit that referenced this issue May 9, 2024
Fix Terminal crashing when experimental PowerShell menu completion is
very quickly invoked multiple times.

`Command::ParsePowerShellMenuComplete` can be called from multiple
threads, but it uses a `static` `Json::CharReader`, which cannot safely
parse data from multiple threads at the same time. Removing `static`
fixes the problem, since every function call gets its own `reader`.

Validation: Pressed Ctrl+Space quickly a few times with hardcoded huge
JSON as the completion payload. Also shown at the end of the second
video in #17220.

Closes #17220
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label May 9, 2024
DHowett pushed a commit that referenced this issue May 13, 2024
Fix Terminal crashing when experimental PowerShell menu completion is
very quickly invoked multiple times.

`Command::ParsePowerShellMenuComplete` can be called from multiple
threads, but it uses a `static` `Json::CharReader`, which cannot safely
parse data from multiple threads at the same time. Removing `static`
fixes the problem, since every function call gets its own `reader`.

Validation: Pressed Ctrl+Space quickly a few times with hardcoded huge
JSON as the completion payload. Also shown at the end of the second
video in #17220.

Closes #17220

(cherry picked from commit 44516ad)
Service-Card-Id: 92524217
Service-Version: 1.21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants