Terminal: cmd.exe always hang if calling SetConsoleScreenBufferSize at bottom line

[mattn]

Environment

Microsoft Windows [Version 10.0.18362.239]

Steps to reproduce

Sorry, I don't figure out clearly but this step always make cmd.exe hang. At present, the method that can be reliably reproduced is to start vim.exe at the bottom line of cmd.exe.

Expected behavior

Do not hang.

Actual behavior

Hang.

FYI, I tried to run this code, but not reproduce.

#include <windows.h>

int
main(int argc, char* argv[]) {
    HANDLE h = GetStdHandle(STD_OUTPUT_HANDLE);
    COORD coord;
    coord.X = 80;
    coord.Y = 300;
    SetConsoleScreenBufferSize(h, coord);
    return 0;
}

[mattn]

I sent PR to vim/vim. https://github.com/vim/vim/pull/4679

I cound avoid hang with moving cursor to top of window before calling SetConsoleScreenBufferSize.

[mattn]

Now, I could reproduce this with the code above. But seems to depend on some conditions.

[mattn]

[mattn]

I attached conhost.exe with gdb.

[Thread 17920.0x3d04 exited with code 0]
warning: onecore\windows\core\console\open\src\buffer\out\textbuffercelliterator
.cpp(49)\conhost.exe!00007FF7AA12FB34: (caller: 00007FF7AA10CACE) Exception(1) t
id(3e34) 80070057 p[^[ｪﾔﾁﾄ｢ﾜｷB
warning: onecore\windows\core\console\open\src\buffer\out\cursor.cpp(198)\conhos
t.exe!00007FF7AA125B92: (caller: 00007FF7AA109DAD) LogHr(2) tid(3e34) 80070057 p
[^[ｪﾔﾁﾄ｢ﾜｷB
    Msg:[onecore\windows\core\console\open\src\buffer\out\textbuffercelliterator
.cpp(49)\conhost.exe!00007FF7AA12FB34: (caller: 00007FF7AA10CACE) Exception(1) t
id(3e34) 80070057 p[^[ｪﾔﾁﾄ｢ﾜｷB
]
warning: onecore\windows\core\console\open\src\host\_stream.cpp(233)\conhost.exe
!00007FF7AA12DB11: (caller: 00007FF7AA10E281) FailFast(1) tid(3e34) 8000FFFF vｽI
ﾈG[ﾅｷB
gdb: unknown target exception 0xc0000409 at 0x7ff7aa12db11

Thread 1 received signal ?, Unknown signal.
[Switching to Thread 17920.0x3e34]
0x00007ffd53dcf5ef in RaiseFailFastException ()
   from C:\WINDOWS\System32\KernelBase.dll
(gdb)

[mattn]

https://github.com/microsoft/terminal/blob/9b92986b49bed8cc41fde4d6ef080921c41e6d9e/src/buffer/out/textBufferCellIterator.cpp#L46

This is a line crashed.

[mattn]

Probably, https://github.com/microsoft/terminal/issues/1795 https://github.com/microsoft/terminal/issues/1206 https://github.com/microsoft/terminal/issues/1238 are related issues.

[mattn]

I guess that the cause of this bug is related on cursor location. If the cursor position is out of bounds of RESIZED size, cmdhost.exe crash.

[mattn]

@DHowett-MSFT Did you get enough information to fix this issue?

[miniksa]

@mattn, I've tried to reproduce this today and cannot get it to happen. Does it still happen for you? Can you provide a crash dump from the conhost on your machine that is crashing?

[mattn]

This is dump file of conhost.exe

conhost.zip

[miniksa]

This is dump file of conhost.exe

conhost.zip

.  0  Id: 10fc.1110 Suspend: 0 Teb: 000000c5`6b7cb000 Unfrozen
 # Child-SP          RetAddr           Call Site
00 000000c5`6ba7f598 00007ffc`6a8657d2 ntdll!ZwWaitForSingleObject+0x14 [minkernel\ntdll\daytona\objfre\amd64\usrstubs.asm @ 211] 
01 000000c5`6ba7f5a0 00007ffc`6cc263e0 KERNELBASE!DeviceIoControl+0x82 [minkernel\kernelbase\filehops.c @ 3526] 
02 000000c5`6ba7f610 00007ff7`4726f17f kernel32!DeviceIoControlImplementation+0x80 [base\win32\client\filehops.c @ 168] 
03 (Inline Function) --------`-------- conhost!DeviceComm::_CallIoctl+0x43 [onecore\windows\core\console\open\src\server\devicecomm.cpp @ 153] 
04 (Inline Function) --------`-------- conhost!DeviceComm::ReadIo+0x43 [onecore\windows\core\console\open\src\server\devicecomm.cpp @ 50] 
05 000000c5`6ba7f660 00007ffc`6cc27bd4 conhost!ConsoleIoThread+0xcf [onecore\windows\core\console\open\src\host\srvinit.cpp @ 655] 
06 000000c5`6ba7f7e0 00007ffc`6d34cee1 kernel32!BaseThreadInitThunk+0x14 [base\win32\client\thread.c @ 64] 
07 000000c5`6ba7f810 00000000`00000000 ntdll!RtlUserThreadStart+0x21 [minkernel\ntdll\rtlstrt.c @ 1153] 

   1  Id: 10fc.1154 Suspend: 0 Teb: 000000c5`6b7d3000 Unfrozen
 # Child-SP          RetAddr           Call Site
00 000000c5`6b9ffe18 00007ffc`6a878ba3 ntdll!ZwWaitForSingleObject+0x14 [minkernel\ntdll\daytona\objfre\amd64\usrstubs.asm @ 211] 
01 000000c5`6b9ffe20 00007ff7`4727bced KERNELBASE!WaitForSingleObjectEx+0x93 [minkernel\kernelbase\synch.c @ 1328] 
02 000000c5`6b9ffec0 00007ffc`6cc27bd4 conhost!Microsoft::Console::Render::RenderThread::_ThreadProc+0x29 [onecore\windows\core\console\open\src\renderer\base\thread.cpp @ 169] 
03 000000c5`6b9ffef0 00007ffc`6d34cee1 kernel32!BaseThreadInitThunk+0x14 [base\win32\client\thread.c @ 64] 
04 000000c5`6b9fff20 00000000`00000000 ntdll!RtlUserThreadStart+0x21 [minkernel\ntdll\rtlstrt.c @ 1153] 

This dump file shows two normal-state threads inside conhost.exe.
Thread 0 is awaiting an API call to service.
Thread 1 is waiting for a frame to paint to the screen.

What's odd here is that there's no input thread. I would expect at least 3. I would suspect that perhaps you've grabbed a dump of the wrong conhost.exe if there were multiple.

Given it's a crash issue, I might be able to find your crash report if you submit feedback with the feedback hub. I'll trigger the bot to give you directions for that.

[miniksa]

/feedback

[msftbot[bot]]

Hi there!

Can you please send us feedback with the Feedback Hub with this issue and paste the link here so we can more easily find your crash information on the back end?

Thanks!

[mattn]

Sorry, I mistaken. When this bug occur, conhost.exe is always exited so I can not get dump. But when I used gdb, I could get stacktrace above.

[j4james]

I can reproduce this fairly easily in the debugger (or at least a very similar issue). The trick, in my case, is to have the _Wrap test output on resize_ option unchecked in the _Layout_ properties. Then the easiest way to trigger it is with a printf in a bash shell:

printf "\e[1;80H\e[8;25;40t"; sleep 5

The sleep isn't always required, but it seems to help in some situations, assumedly because it needs time to trigger before the prompt is displayed and the cursor repositions itself.

In the debugger I can see that it's crashing when trying to paint the cursor while out of bounds (the buffer width is 40, while the y pos is at 80). Here's the key part of my call stack (on commit 4b439cf290d74caf61041803846df81a75f28919):

OpenConsole.exe!TextBufferCellIterator::TextBufferCellIterator(const TextBuffer & buffer, _COORD pos, const Microsoft::Console::Types::Viewport limits) Line 46
OpenConsole.exe!TextBufferCellIterator::{ctor}(const TextBuffer & pos, _COORD) Line 23
OpenConsole.exe!SCREEN_INFORMATION::CursorIsDoubleWidth() Line 2852
OpenConsole.exe!RenderData::IsCursorDoubleWidth() Line 269
OpenConsole.exe!Microsoft::Console::Render::Renderer::_PaintCursor(Microsoft::Console::Render::IRenderEngine * const pEngine) Line 733
OpenConsole.exe!Microsoft::Console::Render::Renderer::_PaintFrameForEngine(Microsoft::Console::Render::IRenderEngine * const pEngine) Line 118
OpenConsole.exe!Microsoft::Console::Render::Renderer::PaintFrame() Line 65
OpenConsole.exe!Microsoft::Console::Render::RenderThread::_ThreadProc() Line 165
OpenConsole.exe!Microsoft::Console::Render::RenderThread::s_ThreadProc(void * lpParameter) Line 148

I'm almost certain I've had this crash with a manual resize as well, although I haven't now been able to reproduce that.

[j4james]

I found another easier to reproduce crash, which I think is different to the one I gave above, but possibly closer the OPs crash. It doesn't depend on the _Wrap test output on resize_ option being unchecked, but it does seem necessary to fill the screen buffer to trigger it.

Steps to reproduce:

1. Open a bash shell in conhost
2. Make sure the screen height is 25.
3. Also make sure the screen buffer height isn't too big, so it's easy to fill.
4. Do a couple of directory listings or something until you've reached the bottom of the buffer and it has started to cycle.
5. Execute an escape sequence that increases the screen height, e.g. printf "\e[8;30;80t"

This crashes for me every time in the current master build (32ea419c3d7b64de70d9007c9b6c119ece850535), although not in the system conhost (ver 10.0.18362.535), so this may be a regression.

[miniksa]

Alright, @j4james, thanks. I'll get back around to this at some point.

[magiblot]

As @mattn hinted, the issue can be worked around by setting the cursor position to (0, 0) before resizing the buffer. It still crashes if the console is resized to the minimum allowed, but that is less likely to happen.

Another workaround is to FreeConsole() and then AllocConsole() every time the console dies (which can be detected by checking if GetNumberOfConsoleInputEvents fails with error ERROR_PIPE_NOT_CONNECTED). This will pop out a new console where your application can continue running.

[j4james]

I've been doing some digging into this, and found there were two underlying issues: 1) the cursor position being out of range, and 2) the viewport being out of range. Issue 1 has largely been addressed by PR #4901, which added bounds checking on the cursor position. That fixed my first test case.

Another partial "fix" came in commit eb480b6bbbd83a2aafbe62992d360838e0ab9da5, which added exception checking on the _PaintFrameForEngine method, and that has stopped my second test case from crashing. It's still technically failing, though, so that's not ideal, but at least it doesn't crash.

It's also worth mentioning that this exception check doesn't catch all exceptions under the _PaintFromEngine method. In particular, an exception in the IsCursorDoubleWidth method will not be caught because it's incorrectly marked as noexcept. So that's the first thing I'd recommend fixing.

The main outstanding problem, though, is the viewport being out of range. That can be triggered from a number of different places: the SetConsoleScreenBufferSize API (which may have been the source of this particular issue), the SetConsoleScreenBufferInfoEx (which I suspect was the source of issue #256), and the SetConsoleWindowInfo API (which was the source of the crash in my second test case).

The last of these is fairly easy to fix. The error is in the SCREEN_INFORMATION::SetViewport method, which tries to make sure the viewport coordinates are in range. There are off-by-one errors in those calculations, though, so it never successfully corrects out-of-range values.

The other two only seem to fail when viewport wrapping is disabled. The problem comes from resizing the buffer in such a way that that viewport extends past the bottom or right. When the wrapping is enabled, the ResizeWithReflow method adjust the viewport so that's not a problem. When disabled, though, the ResizeTraditional method leaves the viewport in a broken state, and the next repaint or cursor movement will likely crash.

Now I don't want to mess with the ResizeScreenBuffer method, since that's called from a bunch of places, most of which have their own way of adjusting the viewport. So my suggestion is we add some additional checks in the SetConsoleScreenBufferSizeImpl and SetConsoleScreenBufferInfoExImpl functions to test if the viewport is out of range, and then adjust the origin to move it back into range.

I can't be certain this will fix every possible resize exception, but it should at least be a major improvement on the current state of affairs.

[j4james]

After testing this some more, I found that fixing the viewport wasn't enough by itself. We also need to make sure the cursor position is clamped to the new buffer dimensions, otherwise there are still situations in which conhost can crash. I had hoped we were clamping the cursor to the viewport in enough places to make that unnecessary, but it seems not.

So in addition to the changes discussed above, I'm also recommending we clamp the cursor position in SetConsoleScreenBufferSize and SetConsoleScreenBufferSizeImpl. I don't think it's necessary in SetConsoleWindowInfo though.

I've already starting putting together a PR with these changes, but there's not a huge amount of work involved, so if you think this is the wrong approach, it's not a big deal to drop it and try something else.

[miniksa]

I'm alright with your proposals, @j4james. I think it's just a bunch of little things that slipped through the cracks here, so I'm happy to take the fixes.

If you did want to tackle the Resize* methods though ever... they're not sacred nor original. I did rewrite them at some point in time, so if it's time to patch them up further, don't feel like you must avoid them unless they just look too crazy to touch.

My long term goal has always been to try to move everything to standard rectangle math (bottom and right are exclusive to match what everything else does instead of the crazypantsness that Console did forever with SMALL_RECTs being inclusive bottom/right) and I think this particular area is suffering from the mismatch pretty heavily. My first attempt was Viewport... which obviously failed as it left a lot of bugs. til::rectangle and friends are my newest attempt, but I haven't rolled them out broadly yet.