Fix FillConsoleOutputCharacterA crash

[mkitzan]

Summary of the Pull Request

Despite being specified as noexcept, FillConsoleOutputCharacterA emits an exception when a call to ConvetToW is made with an argument character which can't be converted. This PR fixes this throw, by wrapping ConvertToW in a try-catch_return.

References

PR Checklist

• Closes FillConsoleOutputCharacterA crashes conhost when passed an invalid character #4258
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed: thanks @miniksa
• Requires documentation to be updated
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place

Detailed Description of the Pull Request / Additional comments

Following the symantics of other FillConsoleOutputCharacter* the output param cellsModified is set to 0. The try-catch_return is also what other functions of this family perform in case of errors.

Validation Steps Performed

The repro test in the issue uses this function as it exists in the windows.h library not the one present in the WT repo (by my understanding). Because of this, I have not been able to test that this change corrects the original repro.
Original repro no longer crashes.

[zadjii-msft]

The repro test in the PR uses this function as it exists in the windows.h library not the one present in the WT repo (by my understanding). Because of this, I have not been able to test that this change corrects the original repro.

So, fortunately, you actually can test this. The functions that are in windows.h are basically just the client-side implementation of these API calls, that are serviced by conhost.exe. So when you make a change to conhost.exe and run the OpenConosle.exe that's built from this repo, you can actually check if the change in implementation fixes the bug. You should be able to run the sample repro program in an OpenConsole.exe instance and ensure that it fixes the bug.

---

Are there any other callers of ConvertToW (outside of tests) that aren't try/catching the call as well?

[mkitzan]

@zadjii-msft thanks for the info on testing, and a good thing too.

Inside ConvertToW there's something causing a crash not a throw. The line that's sticking out to me in ConvertToW is this call to MultiByteToWideChar the destination buffer is a nullptr while the return value is still being used to determine the output wstring size later. The symantics of MultiByteToWideChar don't seem to cover this usage. I'd love to hear a second opinion on this.

I haven't done a pass for other ConvertToW calls. I'll do one once the ConvertToW crash is figured out.

[mkitzan]

I'll take a closer look at the crash this weekend. School has been eating up all my time this week.

[mkitzan]

Here's the crash: string_view.data() is not inherently null terminated and we pass a string_view of a single char to ConvertToW then MultiByteToWideChar uses the pointer from data() to walk the buffer resulting in a crash.

@zadjii-msft, with the latest commit the repro no longer crashes. I did a noexcept check for other ConvertToW callers and all other calls are already wrapped in try-catch blocks.

[zadjii-msft]

Cools, thanks for the clarifications!

[miniksa]

@zadjii-msft Mike Griese FTE thanks for the info on testing, and a good thing too.

Inside ConvertToW there's something causing a crash not a throw. The line that's sticking out to me in ConvertToW is this call to MultiByteToWideChar the destination buffer is a nullptr while the return value is still being used to determine the output wstring size later. The symantics of MultiByteToWideChar don't seem to cover this usage. I'd love to hear a second opinion on this.

The important part is the final parameter, cchWideChar being 0. Setting that requests that MB2WC tell us how many wchar_ts it will need (including a null terminator) if it were to actually give us the result. This means that the destination buffer pointer lpWideCharStr doesn't matter because this call is only attempting to retrieve the count.

The MB2WC function should happily work with both counted and null-terminated (z) strings.

I haven't done a pass for other ConvertToW calls. I'll do one once the ConvertToW crash is figured out.

Here's the crash: string_view.data() is not inherently null terminated and we pass a string_view of a single char to ConvertToW then MultiByteToWideChar uses the pointer from data() to walk the buffer resulting in a crash.

See, I don't know about this. I swear it won't walk past the count that it's given for the source parameter. It will if the count was -1 per the documentation... but with an accurate count of 1, it should have no problem.

@zadjii-msft Mike Griese FTE, with the latest commit the repro no longer crashes. I did a noexcept check for other ConvertToW callers and all other calls are already wrapped in try-catch blocks.

I think adding the try-catch block here is definitely warranted. It would probably harm nothing to just function-level TRY the whole method instead of having to declare the string outside the try and then use fill it later.

Finally, I think we're going to need a test here that confirms the error code coming back from an invalid/unconvertible input character is the same error code that the v1 console gave. I'd want to see that in feature tests.

Given that this is a bit weird in that you are seeing a crash that I cannot believe exists, I'd like to try to repro this myself. If I can get some time...

[miniksa]

OK I adapted the original repro scenario from #4258 into a feature test.

I ran it against conhost v1 and this was the behavior:

• Set the codepage to 50220 with SetConsoleOutputCP.
• Fill with the invalid character (0x14) via FillConsoleOutputCharacterA --> function call succeeds, written = 1.
• Read back the same position via ReadConsoleOutputCharacterA --> function succeeds, read = 1, value of character read = 0x0.

I also reproduced the crash against v2. We should likely match the v1 behavior, beyond just fixing the crash, if it isn't matching. Will continue looking.

[mkitzan]

Interesting that there's this discrepancy between v1 and v2. I'll take some time this weekend to have another look. Thanks for the v1 repro scenario, @miniksa, I'll try and get the behavior to match it.

[miniksa]

@mkitzan, I'll keep looking now. And upload the test. We can merge our two together to fix it.

[miniksa]

f31a3c1

EDIT:

Run by:

1. Open cmd
2. Go to git project root
3. .\tools\razzle.cmd
4. Build the project (x64, debug or whatever)
5. %TAEF% .\bin\x64\debug\conhost.feature.tests.dll /name:FillOutputTests::FillWithInvalidCharacterA /p:TestAsV1=true
6. Switch it to false too.

[miniksa]

Part of the problem here also seems to be that MultiByteToWideChar on ConvertToW:41 seems to return 0, which is supposed to be a failure code here: https://docs.microsoft.com/en-us/windows/win32/api/stringapiset/nf-stringapiset-multibytetowidechar.

So then we call THROW_LAST_ERROR_IF but that itself throws because the last error is set to 0 which is the "OK" error code.

[miniksa]

I'm going to try to debug the v1 host to see how it is coming to a different translation conclusion than the v2 one.

[miniksa]

OK. So the v1 host was awful here.

It would do...

WCHAR ConversionFunction(UINT codepage, char ch)
{
	WCHAR wch = 0;
	MultiByteToWideChar(codepage, 0, &ch, 1, &wch, 1);
	return wch;
}

That's right. It didn't check if there was an error. It just tried it and then plowed ahead with the 0 value if it didn't work.

That feels terribly bad.

I think what we do here is update the test I just wrote to document the difference here, fix this so it doesn't crash (and returns an acceptable error), and then check it in and roll from there.

[mkitzan]

That's right. It didn't check if there was an error. It just tried it and then plowed ahead with the 0 value if it didn't work.

Well that's something, eh? Sorry for being AWOL on this one recently: it's midterm season.

[miniksa]

That's right. It didn't check if there was an error. It just tried it and then plowed ahead with the 0 value if it didn't work.

Well that's something, eh? Sorry for being AWOL on this one recently: it's midterm season.

Not a problem. As long as you don't mind me partying on your branch.

[mkitzan]

Not a problem. As long as you don't mind me partying on your branch.

Always up for a party, thanks for the help!

[miniksa]

OK I cannot comprehend why the test got back a 0x2 error code instead of the 0xffff I was expecting, but then I went to go figure out why all tests are being bad right now in #4490. So I'll get back to this.

[ghost]

🎉Windows Terminal Preview v0.9.433.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download

[DHowett-MSFT]

🎉 Once again, thanks for the contribution!

This pull request was included in a set of conhost changes that was just
released with Windows Insider Build 19603.