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 resizing openconsole to 0 rows (was: std::clamping the cursor, 2023 edition) #14665

Open
zadjii-msft opened this issue Jan 11, 2023 · 0 comments
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news.
Milestone

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented Jan 11, 2023

@zadjii-msft I think we may want to reopen this issue. I was just testing PR #14661, which kind of reverts your fix for this, and I wanted to make sure I hadn't reintroduced the bug. What I discovered was that the issue was never fully resolved - while we don't get a clamp assertion in the cursor code, we now just get it somewhere else instead! For me it occurred here:

row = std::clamp(row + rowOffset.Value, viewport.top, viewport.bottom - 1);

And I double checked with the commit from when PR #13001 was merged (7990436), and it was already failing then. It may not be obvious in some shells, but in a WSL bash shell it crashes straight away. In a cmd shell I had to type some command (e.g. CLS) while the window had a zero size in order to trigger the crash.

It's not a major problem, since it only happens in a debug build, but if we think it needs fixing we should probably reopen this, or at least open another issue if you prefer that.

As for how we fix it, I would have liked it if we could put a minimum size on the viewport, but I suspect that's not possible in conhost for legacy reasons. So failing that, a quick fix would be to replace the clamp calls with a min/max combo which won't assert. But my concern is that the VT architecture is kind of dependent on a non-zero screen size, so there's a fair chance this issue might just surface elsewhere at some point in the future. Maybe we just have to accept that.

Originally posted by @j4james in #12917 (comment)

@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 11, 2023
@zadjii-msft zadjii-msft added this to the Terminal v1.18 milestone Jan 11, 2023
@zadjii-msft zadjii-msft changed the title std::clamping the cursor, 2023 edition std::clamping the cursor, 2023 edition Jan 11, 2023
@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. labels Jan 16, 2023
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 16, 2023
@zadjii-msft zadjii-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, 2023
@zadjii-msft zadjii-msft added the Severity-Crash Crashes are real bad news. label Apr 4, 2023
@zadjii-msft zadjii-msft changed the title std::clamping the cursor, 2023 edition Crash when resizing openconsole to 0 rows (was: std::clamping the cursor, 2023 edition) Apr 4, 2023
@zadjii-msft zadjii-msft modified the milestones: Terminal v1.18, Backlog Apr 4, 2023
@zadjii-msft zadjii-msft added the Help Wanted We encourage anyone to jump in on these. label Apr 4, 2023
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.) Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news.
Projects
None yet
Development

No branches or pull requests

1 participant