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

Neofetch in wsl2(debian) crashes the terminal #4254

Closed
Nithsua opened this issue Jan 16, 2020 · 35 comments
Closed

Neofetch in wsl2(debian) crashes the terminal #4254

Nithsua opened this issue Jan 16, 2020 · 35 comments
Assignees
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Resolution-Fix-Available It's available in an Insiders build or a release
Milestone

Comments

@Nithsua
Copy link

Nithsua commented Jan 16, 2020

Environment

Windows build number: 19041.21
Windows Terminal version (if applicable):  0.8.10091.0

Any other software?
Debian in WSL2

Steps to reproduce

  1. Open terminal (has wsl2 as default shell)
  2. run neofetch

Expected behavior

To show the output of neofetch

Actual behavior

Terminal crashes without any output

@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 Jan 16, 2020
@aoahonen
Copy link

I'm having this same issue with crashing Windows Terminal when running Neofetch in WSL (Ubuntu 16.04)

Event Viewer log's

Faulting application name: WindowsTerminal.exe, version: 0.8.2001.9001, time stamp: 0x5e17d9ef
Faulting module name: ucrtbase.dll, version: 10.0.18362.387, time stamp: 0x4361b720
Exception code: 0xc0000409
Fault offset: 0x000000000006db8e
Faulting process id: 0x4294
Faulting application start time: 0x01d5cc4225433c98
Faulting application path: C:\Program Files\WindowsApps\Microsoft.WindowsTerminal_0.8.10091.0_x64__8wekyb3d8bbwe\WindowsTerminal.exe
Faulting module path: C:\WINDOWS\System32\ucrtbase.dll
Report Id: c83e735f-d600-4a91-8279-8c22e29d0ccc
Faulting package full name: Microsoft.WindowsTerminal_0.8.10091.0_x64__8wekyb3d8bbwe
Faulting package-relative application ID: App

WSL starts as expected (with neofetch) when launching from start menu, or when started with "wsl" or "bash" commands from cmd.exe ("legacy" command prompt)

@nizmow
Copy link

nizmow commented Jan 16, 2020

Happening to me also, on two separate machines (machine A: WSL2/Ubuntu 19.10, machine B: WSL1/Ubuntu 18.04). On the machine A, I get what looks like the first row of characters in the Ubuntu logo before Terminal freezes for a second and crashes. On machine B, it seems to crash immediately.

More details:

Machine A: Terminal installed from MS Store.
Machine B: Terminal built from source and deployed locally.

EDIT: for the record, I just installed neofetch natively on Windows via scoop and ran it in Powershell -- it works perfectly.

@skyline75489
Copy link
Collaborator

skyline75489 commented Jan 16, 2020

I can't seem to reproduce it on my openSUSE distro (Yeah I'm more of a SUSE guy now). It's kind of surprising if this is actually related to distro, though.

EDIT: Ignore the distro thing. This is a regression. Last Store build 0.7.3451.0 is good. Current master 6d6fb7f is bad

@zadjii-msft
Copy link
Member

Wow yea that just crashed the Terminal for me too, pretty immediately, with Ubuntu.

Maybe this is related to the #1360/#4150 class of issues, @miniksa could you verify for me?

@armak
Copy link

armak commented Jan 16, 2020

I encountered this issue too with the latest release version, Ubuntu 18.04 WSL (1) (Microsoft Windows [Version 10.0.18362.356]).

@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Jan 16, 2020
@DHowett-MSFT
Copy link
Contributor

@nizmow since you’ve built it from source, you should be able to produce far better debugging information than somebody who installed it from the store. Would you mind?

@ntnlabs
Copy link

ntnlabs commented Jan 16, 2020

I have the same issue when I connect from my Win terminal with ssh to another box (pure linux). After entering a correct pass the app crashes. Reading the comments I may add that after I log into another box I get a neofetch info...

@ZhaoMJ
Copy link
Contributor

ZhaoMJ commented Jan 16, 2020

I just bisected the commits and it seems that #4125 caused this regression.
The error happens at TerminalDispatch::CursorBackward at TerminalDispatch.cpp Line 63. Specifically, narrow_cast() throws since it's unable to cast distance, which has a weird value (9999999) at this point, into short.

@j4james
Copy link
Collaborator

j4james commented Jan 16, 2020

This is the result of a CUB escape sequence with a distance of 9999999. I've mentioned this as an issue before - it used to be clamped in the state machine, but that was changed in PR #3956. And the result is it throws an exception in the narrow cast in the TerminalDispatch::CursorBackward handler here:

const COORD newCursorPos{ cursorPos.X - gsl::narrow<short>(distance), cursorPos.Y };

In theory that exception should have been caught, which would still be wrong, but at least not crashing. However, it seems that GSL_TERMINATE_ON_CONTRACT_VIOLATION is set, so the throw_exception call actually just kills the app.

@j4james
Copy link
Collaborator

j4james commented Jan 16, 2020

I just bisected the commits and it seems that #4125 caused this regression.

Just to be clear, that's not really the cause. That just fixed an error in the passthrough implementation that was hiding the problem.

@nizmow
Copy link

nizmow commented Jan 16, 2020

Looks like debugging might not be necessary, but when I get in front of a machine next I’ll give it a shot if still required. Figured it must be an escape sequence.

@miniksa
Copy link
Member

miniksa commented Jan 16, 2020

This is the result of a CUB escape sequence with a distance of 9999999. I've mentioned this as an issue before - it used to be clamped in the state machine, but that was changed in PR #3956. And the result is it throws an exception in the narrow cast in the TerminalDispatch::CursorBackward handler here:

const COORD newCursorPos{ cursorPos.X - gsl::narrow<short>(distance), cursorPos.Y };

In theory that exception should have been caught, which would still be wrong, but at least not crashing. However, it seems that GSL_TERMINATE_ON_CONTRACT_VIOLATION is set, so the throw_exception call actually just kills the app.

Welp, looks like we need #4144 in so we can get going on #4153 and apply the saturating math here.

@j4james
Copy link
Collaborator

j4james commented Jan 16, 2020

As another example of this problem, you can kill conhost in a similar way with the IL and DL sequences (assuming you're using a recent build). For example, in a bash shell do:

printf "\e[9999999L"

I know #4144/#4153 should fix many of these issues eventually, but I think it's a serious enough problem that it's worth reverting the state machine clamping ASAP as a quick fix.

Either way, I think we should be defining GSL_THROW_ON_CONTRACT_VIOLATION instead of using the default GSL_TERMINATE_ON_CONTRACT_VIOLATION. I can't believe we'd actually want the app to just crash if there is an unexpected overflow.

@miniksa
Copy link
Member

miniksa commented Jan 16, 2020

As another example of this problem, you can kill conhost in a similar way with the IL and DL sequences (assuming you're using a recent build). For example, in a bash shell do:

printf "\e[9999999L"

I know #4144/#4153 should fix many of these issues eventually, but I think it's a serious enough problem that it's worth reverting the state machine clamping ASAP as a quick fix.

Either way, I think we should be defining GSL_THROW_ON_CONTRACT_VIOLATION instead of using the default GSL_TERMINATE_ON_CONTRACT_VIOLATION. I can't believe we'd actually want the app to just crash if there is an unexpected overflow.

I didn't know that was an option. Let's file an issue to consider changing GSL Terminate to GSL Throw.

Also, I just merged #4144 now. We could fix this one particular crash by using the saturating math without waiting for all locations to be fixed as a part of #4153.

@j4james
Copy link
Collaborator

j4james commented Jan 16, 2020

Also, I just merged #4144 now. We could fix this one particular crash by using the saturating math without waiting for all locations to be fixed as a part of #4153.

I'm not sure what you had in mind, but if you are just planning to fix it in TerminalDispatch, be aware that that just fixes the crash - the functionality will still be broken. It's only getting as far as TerminalDispatch because it had already failed in conhost. Fixing it in the one but not the other will just leave the two systems out of sync.

@miniksa
Copy link
Member

miniksa commented Jan 16, 2020

I don't have a specific fix in mind. You just said that it used to be clamped and now it isn't. So I supposed we would use clamping math to fix it. Even if that has to be on both sides of the interface.

@j4james
Copy link
Collaborator

j4james commented Jan 16, 2020

I'm not objecting to the use of the new clamping math. I was just suggesting we handle that clamping at the state machine level since that would be the quickest short term solution. If you think it'll be easy to fix everywhere else instead, that'd be great, since that is ultimately what we want - I just thought that would be a lot of work.

@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Priority-1 A description (P1) Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news. labels Jan 16, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 16, 2020
@zadjii-msft zadjii-msft added this to the Terminal v0.9 milestone Jan 16, 2020
@dollproxy
Copy link

dollproxy commented Jan 16, 2020

Same thing happened to me while trying neofetch with the Terminal (preview) from Chocolatey, latest version.

But yeah it seems independent of the distro you run WSL with

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 16, 2020
@miniksa miniksa self-assigned this Jan 16, 2020
@miniksa miniksa modified the milestones: Terminal v0.9, Terminal v1.0 Feb 5, 2020
@cinnamon-msft cinnamon-msft added Priority-2 A description (P2) v1-Scrubbed and removed Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. labels Feb 28, 2020
ghost pushed a commit that referenced this issue Apr 1, 2020
## Summary of the Pull Request

This PR clamps the parameter values in the VT `StateMachine` parser to 32767, which was the initial limit prior to PR #3956. This fixes a number of overflow bugs (some of which could cause the app to crash), since much of the code is not prepared to handle values outside the range of a `short`.

## References

#3956 - the PR where the cap was changed to the range of `size_t`
#4254 - one example of a crash caused by the higher range

## PR Checklist
* [x] Closes #5160
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Detailed Description of the Pull Request / Additional comments

The DEC STD 070 reference recommends supporting up to at least 16384 for parameter values, so 32767 should be more than enough for any standard VT sequence. It might be nice to increase the limit to 65535 at some point, since that is the cap used by both XTerm and VTE. However, that is not essential, since there are very few situations where you'd even notice the difference. For now, 32767 is the safest choice for us, since anything greater than that has the potential to overflow and crash the app in a number of places.

## Validation Steps Performed

I had to make a couple of modifications to the range checks in the `OutputEngineTest`, more or less reverting to the pre-#3956 behavior, but after that all of the unit tests passed as expected.

I manually confirmed that this fixes the hanging test case from #5160, as well as overflow issues in the cursor operations, and crashes in `IL` and `DL` (see #4254 (comment)).
donno2048 added a commit to donno2048/terminal that referenced this issue Sep 28, 2020
## Summary of the Pull Request

This PR clamps the parameter values in the VT `StateMachine` parser to 32767, which was the initial limit prior to PR #3956. This fixes a number of overflow bugs (some of which could cause the app to crash), since much of the code is not prepared to handle values outside the range of a `short`.

## References

#3956 - the PR where the cap was changed to the range of `size_t`
#4254 - one example of a crash caused by the higher range

## PR Checklist
* [x] Closes #5160
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Detailed Description of the Pull Request / Additional comments

The DEC STD 070 reference recommends supporting up to at least 16384 for parameter values, so 32767 should be more than enough for any standard VT sequence. It might be nice to increase the limit to 65535 at some point, since that is the cap used by both XTerm and VTE. However, that is not essential, since there are very few situations where you'd even notice the difference. For now, 32767 is the safest choice for us, since anything greater than that has the potential to overflow and crash the app in a number of places.

## Validation Steps Performed

I had to make a couple of modifications to the range checks in the `OutputEngineTest`, more or less reverting to the pre-#3956 behavior, but after that all of the unit tests passed as expected.

I manually confirmed that this fixes the hanging test case from #5160, as well as overflow issues in the cursor operations, and crashes in `IL` and `DL` (see microsoft/terminal#4254 (comment)).
@mikelyons
Copy link

Does anyone have the procedure for removing neofetch so that I don't have to reconfigure everything?

@DHowett
Copy link
Member

DHowett commented Oct 15, 2020

@mikelyons you're commenting on a bug with a crash that was fixed 9 months ago. What are you seeing?

@mikelyons
Copy link

@DHowett I'm just still seeing WSL terminal crashing immediately on launch after a windows update, latest windows updates don't fix it.

@DHowett
Copy link
Member

DHowett commented Dec 4, 2020

Please file a new bug. It would also help if you follow the feedback hub steps that (I think) are in the bug template.

@microsoft microsoft locked as resolved and limited conversation to collaborators Dec 4, 2020
@microsoft microsoft unlocked this conversation Jul 13, 2021
@zadjii-msft zadjii-msft self-assigned this Jul 13, 2021
@zadjii-msft
Copy link
Member

You know, there was discussion of some other related crashes (namely in printf "\e[999999999L") in this thread, but those look like they've been fixed in the meantime as well. Unclear when exactly. But the OP, and the follow-up crashes have been fixed, so I'm gonna close this one out.

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Jul 13, 2021
@zadjii-msft zadjii-msft added Resolution-Fix-Available It's available in an Insiders build or a release and removed Needs-Tag-Fix Doesn't match tag requirements labels Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Resolution-Fix-Available It's available in an Insiders build or a release
Projects
None yet
Development

No branches or pull requests