"invalid bounds arguments passed to std::clamp" when resizing window to zero height

[Predelnik]

Windows Terminal version

1.12.10733.0

Windows build number

10.0.22000

Other Software

No response

Steps to reproduce

Launch OpenConsole.exe compiled in debug mode, drag bottom of the window to the very top.

Expected Behavior

No assertion

Actual Behavior

Assertion from from STL "invalid bounds arguments passed to std::clamp" on the line

clampMe.Top = std::clamp(clampMe.Top, Top(), BottomInclusive());

in Viewport Viewport::Clamp

Call stack

 	ucrtbased.dll!00007ff9248a7fbc()	Unknown
 	ucrtbased.dll!00007ff9248a7ef4()	Unknown
 	ucrtbased.dll!00007ff9248a7d90()	Unknown
>	OpenConsole.exe!std::clamp>(const short & _Val, const short & _Min_val, const short & _Max_val, std::less _Pred) Line 10188	C++
 	OpenConsole.exe!std::clamp(const short & _Val, const short & _Min_val, const short & _Max_val) Line 10208	C++
 	OpenConsole.exe!Microsoft::Console::Types::Viewport::Clamp(const Microsoft::Console::Types::Viewport & other) Line 245	C++
 	OpenConsole.exe!Microsoft::Console::Render::Renderer::TriggerRedrawCursor(const _COORD * const pcoord) Line 284	C++
 	OpenConsole.exe!TextBuffer::TriggerRedrawCursor(const _COORD position) Line 992	C++
 	OpenConsole.exe!Cursor::_RedrawCursorAlways() Line 190	C++
 	OpenConsole.exe!Cursor::SetIsOn(const bool fIsOn) Line 103	C++
 	OpenConsole.exe!Microsoft::Console::CursorBlinker::TimerRoutine(SCREEN_INFORMATION & ScreenInfo) Line 153	C++
 	OpenConsole.exe!CursorTimerRoutineWrapper(_TP_CALLBACK_INSTANCE * __formal, void * __formal, _TP_TIMER * __formal) Line 24	C++
 	ntdll.dll!TppTimerpExecuteCallback()	Unknown
 	ntdll.dll!TppWorkerThread()	Unknown
 	kernel32.dll!00007ff9feee54e0()	Unknown
 	ntdll.dll!RtlUserThreadStart�()	Unknown

Looks like it happens because SCREEN_INFORMATION::_InternalSetViewportSize increases viewport's Top in srNewViewport.Top -= DeltaY; (DeltaY is negative) line without any check to prevent Top becoming more than Bottom which sadly can happen. Clamp cannot work when Top is more than Bottom

[zadjii-msft]

For my own reference, which commit are you building off of? I just wanna know how far back we'll have to service this fix 😅

[Predelnik]

I'm building recent main (v1.10.1933.0-605-g41ef5554f) and haven't tried to figure out when exactly it appeared unfortunately.

[zadjii-msft]

uh, https://github.com/microsoft/terminal/releases/tag/v1.10.1933.0 is like, 10 months old... If this goes that far back, well, we're in some serious trouble

edit: just guesses

• https://github.com/microsoft/terminal/blame/41ef5554f5cb946c4a48e2b8db791265c7217fce/src/renderer/base/renderer.cpp#L266 - Add support for double-width/double-height lines in conhost #8664 added the Clamp call to Renderer::TriggerRedrawCursor. There wasn't one there before that.

[j4james]

The more I think about, the more I think that call to view.Clamp is fundamentally wrong anyway. The intention was to clip the cursor rect to the viewport, so you'd only be drawing the portion of the cursor that was visible, but that's not what Viewport::Clamp does. If the cursor is completely off screen, it's basically going to pull it back into view, forcing the renderer to redraw an area of the screen that doesn't actually need refreshing. That's probably not the end of the world, but it's not right.

For the same reason, I don't think this bounds error is the end of the world either. In a release build, the clamp is just going to fail to do anything, but that's no worse than the off screen clamping. It's just invalidating an area of the screen that doesn't need redrawing.

So while we definitely should be fixing this, I don't think you need to panic about backporting the fix, unless I'm missing something.

[zadjii-msft]

Oh oh oh maybe this is why #12739

Maybe we're clamping the cursor, drawing it there, and then not invalidating it at the clamped place. Idk. Just a thought while fixing this. EDIT: I was wrong. that one still repros.

[j4james]

@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:

terminal/src/terminal/adapter/adaptDispatch.cpp

Line 219 in 4d27a05

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.

[zadjii-msft]

Thanks for the write up @j4james. I'm gonna just promote that to another thread to try and keep the history a bit cleaner.