Don't put NUL in the buffer in VT mode #3015
Conversation
I haven't had a chance to test this fix on my machine with a CentOS VM quite yet, but this _should_ work Also adds a test
zadjii-msft
added
Product-Conhost
|
Pains me to say, but should this be in WriteChars...? |
ghost
added
Needs-Author-Feedback
|
So I don't know how this could really break backcompat. In general, we're talking about cmd.exe. For other "legacy" applications, cmd.exe is going to revert to "normal"/non-VT mode before launching the child process, so they won't be affected by this change. Apps that are using VT mode probably aren't doing this anyways, and if they're using VT mode, then they probably expect this fixed behavior, not the broken behavior. cmd.exe however was changed to be in VT mode itself, so this change will apply to cmd.exe. Things that it might affect:
At least for 2, we don't need to worry. While we can insert other C0 chars into the commandline, (e.g. ctrl+[ -> '\x1b` in a cooked read), it seems like we can't use this same input mechanism to input a NUL. ctrl+@ does nothing, and ctrl+Space just inserts a space. So nothing to worry about there. For 3, currently when you paste input with a NUL in it, we treat that NUL as the end of the string. So you can't paste a NUL into the cooked input line. This is unchanged with my delta, so nothing to worry about here. Then, as far as 1 goes, it seems like when cmd is processing a This behavior looks unchanged after my delta here, so we don't need to worry about it either. Can anyone think of any other scenarios? |
|
I can't think of any other reasonable scenarios, honestly. I'm comfortable with this. It actually touches on something I wondered before: can we use NUL to represent "unused buffer space" and translate it to space at the API boundary? It makes |
|
@msftbot make sure @miniksa signs off |
ghost
added
the
AutoMerge
|
Hello @DHowett-MSFT! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
DHowett-MSFT
removed
the
AutoMerge
This thought experiment is on my perpetual backlog for killing |
Maybe in |
|
I'm fine simply because it's scoped to the VT engine. The only null-->space problems I can envision are around weird third party apps stuffing nulls into the storage space and retrieving it via |
- Fix snapping to the cursor in "Terminal Scrolling" mode (GH-2705) fixes GH-1222 PSReadline calls SetConsoleCursorPosition on each character. We try to snap. However, we'd only ever do this with the visible viewport, the viewport defined by `SCREEN_INFORMATION::_viewport`. When there's a virtual viewport in Terminal Scrolling mode, we actually need to snap the virtual viewport, so that this behavior looks more regular. (cherry picked from commit 6f4b98a) - Passthrough BEL in conpty (GH-2990) fixes GH-2952. (cherry picked from commit 6831120) - make filling chars (and, thus, erase line/char) unset wrap (GH-2831) EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row with chars, we were setting the wrap flag. We need to specifically not do this on ANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill to the end of the line. Originally, we had a boolean `setWrap` that would mean... - **true**: if writing to the end of the row, SET the wrap value to true - **false**: if writing to the end of the row, DON'T CHANGE the wrap value ,- current wrap value | ,- fill last cell? | | ,- new wrap value | | | ,- comments |-|-|-| |1|0|0| THIS CASE WAS UNHANDLED To handle that special case (1-0-0), we need to UNSET the wrap. So now, we have ~setWrap~ `wrap` mean the following: - **true**: if writing to the end of the row, SET the wrap value to TRUE - **false**: if writing to the end of the row, SET the wrap value to FALSE - **nullopt**: leave the wrap value as it is Closes GH-1126 (cherry picked from commit 4dd9f9c) - When reverse indexing, preserve RGB/256 attributes (GH-2987) (cherry picked from commit 847d6b5) - do not allow CUU and CUD to move "across" margins. (GH-2996) If we're moving the cursor up, its vertical movement should be stopped at the top margin. Similarly, this applies to moving down and the bottom margin. Fixes GH-2929. (cherry picked from commit 0ce08af) - Prevent the v1 propsheet from zeroing colors, causing black text on black background. (GH-2651) fixes GH-2319 (cherry picked from commit b97db63) - Render the cursor state to VT (GH-2829) (cherry picked from commit a9f3849) - Don't put NUL in the buffer in VT mode (GH-3015) (cherry picked from commit a82d6b8) - Return to ground when we flush the last char (GH-2823) The InputStateMachineEngine was incorrectly not returning to the ground state after flushing the last sequence. That means that something like alt+backspace would leave us in the Escape state, not the ground state. This makes sure we return to ground. Additionally removes the "Parser.UnitTests-common.vcxproj" file, which was originally used for a theoretical time when we only open-sourced the parser. It's unnecessary now, and we can get rid of it. Also includes a small patch to bcz.cmd, to make sure bx works with projects with a space in their name. (cherry picked from commit 53c81a0) - Add support for passing through extended text attributes, like… (GH-2917) ... Related work items: #23678919, #23678920, #23731910, #23731911, #23731912, #23731913, #23731914, #23731915, #23731916, #23731917, #23731918
|
🎉 Handy links: |
ghost
mentioned this pull request
On a real VT terminal, most of the control characters that don't do anything are supposed to be filtered out, and not written to the buffer. Up to to now, though, we've only been filtering out `NUL`. This PR extends our control processing to filter the remaining characters that aren't supposed to be displayed. We introduced filtering for the `NUL` control in PR #3015. The are two special cases worth mentioning. 1. The `SUB` control's main purpose is to the cancel a control sequence that is in progress, but it also needs to output an error character (a reverse question mark) to the display. 2. The `DEL` control is typically filtered out, but when a 96-character set is designated, it can sometimes be mapped to a printable glyph that needs to be displayed. ## Validation Steps Performed I've manually tested that all the controls that are meant to be filtered out are no longer being displayed. I've also extended the existing `NUL` unit test to cover the full set of controls characters that are supposed to be filtered. Closes #10786
Summary of the Pull Request
Filter NUL from the stream when emitted in VT processing mode. Linux terminals can't really have a NUL in their "buffer" - the NUL characters are just ignored. For this particular bug,
screenwas writing a long sequence of NUL bytes as effectively a timeout, so that it could "flash" the screen.References
PR Checklist
Detailed Description of the Pull Request / Additional comments
This PR is here for discussion - I'm a tad bit worried that it might regress some scenario unintentionally.
cmd.exeuses VT processing for its own scripts and internal commands, is there something here we might be breaking? I doubt it, but I'm unsure.Validation Steps Performed
screento ensure this actually fixes the problem at hand.