OpenConsole crash when pasting emoji

Environment

Windows build number:  10.0.19041.630]
Windows Terminal version (if applicable):

Any other software?
tcsh (native Windows version)

# Steps to reproduce

download tcsh-x64.exe from https://github.com/amoldeshpande/tcsh/releases/tag/20210103.10 

Launch tcsh.  Doesn't have to be in terminal.  Paste any emoji into it.  It will crash conhost.exe on Windows 10 2004 and OpenConsole.exe if you launch within Windows Terminal

# Expected behavior

Should paste an emoji

# Actual behavior


Edited: repro steps with tcsh. Cannnot repro it with my test program anymore

Irrespective of whether emojis are expected to work in these conditions, it seems very wrong to me that an error condition here would cause the entire process to terminate. I'm convinced that half of the FAIL_FAST calls in the code are unintentional, and the original author was likely expecting an error to be returned, or an exception to be thrown (much like all the places we're mistakenly using gsl::narrow instead of gsl::narrow_cast).

I cannot reproduce the crash when right-click pasting 😀 (U+1F600) or 👨‍👩‍👧‍👦 (U+1F468 U+200D U+1F469 U+200D U+1F467 U+200D U+1F466) to OpenConsole.exe copied from Windows Terminal 1.4.3243.0, with the demo program built for x86, on Windows 10 version 20H2 (19042.685), where the setting "Beta: Use Unicode UTF-8 for worldwide language support" is off.

I don't see why SetFileApisToANSI would even matter here; doesn't it only affect the encoding of file names, which the demo program does not use?

Kalle Olavi Niemitalo (@KalleOlaviNiemitalo)

Kalle Olavi Niemitalo (@KalleOlaviNiemitalo) I think you need to paste more than 100 characters (the length of the INPUT_RECORD buffer that was passed to ReadConsoleInput) in order to trigger the error. I could reproduce it by pasting the following sequence:

😀💘💥👍🎅🦄🍄🌎🚖⚓🚀⌚💣😀💘💥👍🎅🦄🍄🌎🚖⚓🚀⌚💣😀💘💥👍🎅🦄🍄🌎🚖⚓🚀⌚💣😀💘💥👍🎅🦄🍄🌎🚖⚓🚀⌚💣😀💘💥👍🎅🦄🍄🌎🚖⚓🚀⌚💣

I have been using a single 😁 to crash it.

In that case I can't reproduce it either. What's the commit hash of the revision that you're testing with? And in case it makes any difference, what are the build details - x86 or x64, debug or release? Also, what keyboard locale are you using?

I just realised that the crash that I was seeing was actually in the DirectReadData::Notify method rather than the _DoGetConsoleInput function, but the code is essentially identical.

This is in DirectReadData::Notify:

terminal/src/host/readDataDirect.cpp

Lines 168 to 184 in d09fdd6

    
           for (size_t i = 0; i < _eventReadCount; ++i) 
        
           { 
        
               if (readEvents.empty()) 
        
               { 
        
                   break; 
        
               } 
        
               _outEvents.push_back(std::move(readEvents.front())); 
        
               readEvents.pop_front(); 
        
           } 
        
           // store partial event if necessary 
        
           if (!readEvents.empty()) 
        
           { 
        
               _pInputBuffer->StoreReadPartialByteSequence(std::move(readEvents.front())); 
        
               readEvents.pop_front(); 
        
               FAIL_FAST_IF(!(readEvents.empty())); 
        
           }

And this is in _DoGetConsoleInput:

terminal/src/host/directio.cpp

Lines 226 to 242 in d09fdd6

    
           for (size_t i = 0; i < eventReadCount; ++i) 
        
           { 
        
               if (readEvents.empty()) 
        
               { 
        
                   break; 
        
               } 
        
               outEvents.push_back(std::move(readEvents.front())); 
        
               readEvents.pop_front(); 
        
           } 
        
           // store partial event if necessary 
        
           if (!readEvents.empty()) 
        
           { 
        
               inputBuffer.StoreReadPartialByteSequence(std::move(readEvents.front())); 
        
               readEvents.pop_front(); 
        
               FAIL_FAST_IF(!(readEvents.empty())); 
        
           }

Either of those will crash if the number of readEvents is more than 1 greater than the _eventReadCount, and that is easy to achieve with a multibyte codepage. I can't see how the crash could be triggered with a single character, though, unless ReadConsoleInput was called with a much smaller record buffer.

If I understand correctly, InputBuffer::_ReadBuffer tries to predict how many UTF-16 input events fit in the specified number of OEM input events, but it assumes the OEM encoding uses at most two bytes per character:

terminal/src/host/inputBuffer.cpp

Lines 394 to 405 in d09fdd6

    
           ++virtualReadCount; 
        
           if (!unicode) 
        
           { 
        
               if (readEvents.back()->EventType() == InputEventType::KeyEvent) 
        
               { 
        
                   const KeyEvent* const pKeyEvent = static_cast<const KeyEvent* const>(readEvents.back().get()); 
        
                   if (IsGlyphFullWidth(pKeyEvent->GetCharData())) 
        
                   { 
        
                       ++virtualReadCount; 
        
                   } 
        
               } 
        
           }

According to #7589 (comment), the estimated number of bytes per character even depends on the font.

Then, SplitToOem translates the UTF-16 input events to OEM input events. If the configured code page is UTF-8, then it uses CESU-8, which may need three bytes per UTF-16 code unit. _DoGetConsoleInput cannot return the excess events to its caller and does not know how to put them back to the input buffer, either.

If InputBuffer::_ReadBuffer supported UTF-8, then that would avoid the crash, I think. However, the resulting input events would still have CESU-8 rather than UTF-8, unless that is converted in some other layer that I cannot find now.

Also, I wonder if the StoreReadPartialByteSequence call should perhaps be skipped if IsPeek is true.

dangit, I can't reproduce it any more with my simple piece of code.

The more involved repro consists of running code in this branch https://github.com/amoldeshpande/tcsh/tree/unicode under Windows Terminal or OpenConsole.exe and pasting the emoji. This is 100% still repro-able with font of "MesloLGM NF" . Have not tried any other fonts IIRC with this shell.

(Nerd Fonts from here https://github.com/ryanoasis/nerd-fonts/releases)

In answer to a previous question, I used an x64 debug build with standard US-EN keyboard layout.

I will keep trying to make my simple repro crash again meanwhile.

My guess is that there was a mode change involved somewhere (i.e. SetConsoleMode) that persisted from another test case or app. Then when you restarted OpenConsole, the modes went back to the defaults, so the error couldn't be reproduced.


Editing to update repro steps:

download tcsh-x64.exe from https://github.com/amoldeshpande/tcsh/releases/tag/20210103.10 

Launch tcsh.  Doesn't have to be in terminal.  Paste any emoji into it.  It will crash conhost.exe on Windows 10 2004 and OpenConsole.exe if you launch within Windows Terminal

Hmmmmmm. So it sounds like we kinda need to do a SplitToOem in InputBuffer::Read, before we even move the records to the Notify call. Or something like a pre-emptive scan in Read, like checking how many events each event will be translated into. I wonder how easy this would be to write a test for...

Okay so I think InputBuffer::Read is only called in two places:

directio.cpp:189
readDataDirect.cpp:135

Both do the same thing. If called with !unicode, then they will call SplitToOem on the events read from the buffer, to expand them out to chars. So I think it's okay to pre-emptively trim the buffer in the !unicode case inside of IB::Read, because we know the caller will be expanding them.

Wow those two functions are really basically the same code aren't they...

I think this is a valid test for #8663

Navigation Menu

Repository navigation

OpenConsole crash when pasting emoji #8663

Environment

20 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	for (size_t i = 0; i < _eventReadCount; ++i)
	{
	if (readEvents.empty())
	{
	break;
	}
	_outEvents.push_back(std::move(readEvents.front()));
	readEvents.pop_front();
	}

	// store partial event if necessary
	if (!readEvents.empty())
	{
	_pInputBuffer->StoreReadPartialByteSequence(std::move(readEvents.front()));
	readEvents.pop_front();
	FAIL_FAST_IF(!(readEvents.empty()));
	}

	for (size_t i = 0; i < eventReadCount; ++i)
	{
	if (readEvents.empty())
	{
	break;
	}
	outEvents.push_back(std::move(readEvents.front()));
	readEvents.pop_front();
	}

	// store partial event if necessary
	if (!readEvents.empty())
	{
	inputBuffer.StoreReadPartialByteSequence(std::move(readEvents.front()));
	readEvents.pop_front();
	FAIL_FAST_IF(!(readEvents.empty()));
	}

	++virtualReadCount;
	if (!unicode)
	{
	if (readEvents.back()->EventType() == InputEventType::KeyEvent)
	{
	const KeyEvent* const pKeyEvent = static_cast<const KeyEvent* const>(readEvents.back().get());
	if (IsGlyphFullWidth(pKeyEvent->GetCharData()))
	{
	++virtualReadCount;
	}
	}
	}

OpenConsole crash when pasting emoji #8663

Description

Environment

Activity

j4james commented on Dec 27, 2020

KalleOlaviNiemitalo commented on Dec 27, 2020

j4james commented on Dec 27, 2020

amoldeshpande commented on Dec 27, 2020

j4james commented on Dec 27, 2020

j4james commented on Dec 27, 2020

KalleOlaviNiemitalo commented on Dec 27, 2020

amoldeshpande commented on Dec 27, 2020

j4james commented on Dec 27, 2020

amoldeshpande commented on Dec 27, 2020

20 remaining items

zadjii-msft commented on Feb 2, 2022

zadjii-msft commented on Feb 2, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions