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

Crash when switching back from the alt buffer #17709

Closed
Tracked by #17643
j4james opened this issue Aug 13, 2024 · 0 comments · Fixed by #17748
Closed
Tracked by #17643

Crash when switching back from the alt buffer #17709

j4james opened this issue Aug 13, 2024 · 0 comments · Fixed by #17748
Assignees
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, 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. Severity-Crash Crashes are real bad news.

Comments

@j4james
Copy link
Collaborator

j4james commented Aug 13, 2024

Windows Terminal version

Commit edfa3ea

Windows build number

10.0.19045.4651

Other Software

No response

Steps to reproduce

It's not easy to reproduce, but I think all you really need to trigger the crash is to switch to the alt buffer with printf "\e[?1049h", and then switch back to the main buffer with printf "\e[?1049l".

This is in Windows Terminal using a build that includes the new VT passthrough (PR #17510).

Expected Behavior

The terminal should not crash.

Actual Behavior

I got an access violation accessing the screenInfo parameter in the WriteCharsVT function here:

if (WI_IsFlagSet(screenInfo.OutputMode, DISABLE_NEWLINE_AUTO_RETURN))

What happens is that we first pass the \e[?1049l sequence through to conhost in the stateMachine.ProcessString call a couple of lines above. That ends up calling SCREEN_INFORMATION::UseMainScreenBuffer, which calls s_RemoveScreenBuffer(psiAlt), which deletes the active screenInfo instance here:

delete pScreenInfo;

That's the same screenInfo instance that was passed to our WriteCharsVT function, so when we later try to reference that, we're referencing a deleted object. Sometimes we get lucky and nothing bad happens, but sometimes it will crash.

I think maybe all we need to do to prevent the crash is to save the screenInfo.OutputMode at the start of the function, and then use that saved value instead of trying to look it up via the screenInfo object.

@j4james j4james 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 Aug 13, 2024
@carlos-zamora carlos-zamora added this to the Terminal v1.22 milestone Aug 14, 2024
@carlos-zamora carlos-zamora added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 14, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Aug 20, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, 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. Severity-Crash Crashes are real bad news.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants