Fix FillConsoleOutputCharacterA crash

src/host/_output.cpp

@@ -303,7 +303,11 @@ void WriteToScreen(SCREEN_INFORMATION& screenInfo, const Viewport& region)
                                                                    const size_t lengthToWrite,
                                                                    const COORD startingCoordinate,
                                                                    size_t& cellsModified) noexcept
+try
 {
+    // In case ConvertToW throws causing an early return, set modified cells to 0.
+    cellsModified = 0;
+
     const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
 
     // convert to wide chars and call W version
@@ -313,3 +317,4 @@ void WriteToScreen(SCREEN_INFORMATION& screenInfo, const Viewport& region)
 
     return FillConsoleOutputCharacterWImpl(OutContext, wchs.at(0), lengthToWrite, startingCoordinate, cellsModified);
 }
+CATCH_RETURN()

src/host/ft_host/API_FillOutputTests.cpp

@@ -10,6 +10,39 @@ class FillOutputTests
     BEGIN_TEST_CLASS(FillOutputTests)
     END_TEST_CLASS()
 
+    // Adapted from repro in GH#4258
+    TEST_METHOD(FillWithInvalidCharacterA)
+    {
+        BEGIN_TEST_METHOD_PROPERTIES()
+            TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method") // Don't pollute other tests by isolating our codepage change to this test.
+        END_TEST_METHOD_PROPERTIES()
+
+        VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleOutputCP(50220));
+        auto handle = GetStdOutputHandle();
+        const COORD pos{ 0, 0 };
+        DWORD written = 0;
+        const char originalCh = 14;
+
+        WEX::Logging::Log::Comment(L"This test diverges between V1 and V2 consoles.");
+        if (Common::_isV2)
+        {
+            VERIFY_WIN32_BOOL_FAILED(FillConsoleOutputCharacterA(handle, originalCh, 1, pos, &written));
+            VERIFY_ARE_EQUAL(gsl::narrow_cast<DWORD>(HRESULT_CODE(E_UNEXPECTED)), ::GetLastError());
+        }
+        else
+        {
+            VERIFY_WIN32_BOOL_SUCCEEDED(FillConsoleOutputCharacterA(handle, originalCh, 1, pos, &written));
+            VERIFY_ARE_EQUAL(1u, written);
+
+            char readCh = 42; // don't use null (the expected) or 14 (the actual) to ensure that it is read out.
+            DWORD read = 0;
+
+            VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterA(handle, &readCh, 1, pos, &read));
+            VERIFY_ARE_EQUAL(1u, read);
+            VERIFY_ARE_EQUAL(0, readCh, L"Null should be read back as the conversion from the invalid original character.");
+        }
+    }
+
     TEST_METHOD(WriteNarrowGlyphAscii)
     {
         HANDLE hConsole = GetStdOutputHandle();

src/host/ft_host/Common.cpp

@@ -7,6 +7,7 @@ using WEX::Logging::Log;
 using namespace WEX::Common;
 
 HANDLE Common::_hConsole = INVALID_HANDLE_VALUE;
+bool Common::_isV2 = true;
 extern wil::unique_process_information pi;
 
 bool IsConsoleStillRunning()

src/host/ft_host/Common.hpp

@@ -25,6 +25,7 @@ class Common
     static bool TestBufferSetup();
     static bool TestBufferCleanup();
     static HANDLE _hConsole;
+    static bool _isV2;
 };
 
 class CommonV1V2Helper

src/host/ft_host/InitTests.cpp

@@ -91,10 +91,12 @@ MODULE_SETUP(ModuleSetup)
     if (testAsV1)
     {
         v2ModeHelper.reset(new CommonV1V2Helper(CommonV1V2Helper::ForceV2States::V1));
+        Common::_isV2 = false;
     }
     else
     {
         v2ModeHelper.reset(new CommonV1V2Helper(CommonV1V2Helper::ForceV2States::V2));
+        Common::_isV2 = true;
     }
 
     // Retrieve location of directory that the test was deployed to.
@@ -106,11 +108,13 @@ MODULE_SETUP(ModuleSetup)
     // The OS will auto-start the inbox conhost to host this process.
     if (insideWindows || testAsV1)
     {
+        WEX::Logging::Log::Comment(L"Launching with inbox conhost.exe");
         value = value.Append(L"Nihilist.exe");
     }
     else
     {
         // If we're outside or testing V2, let's use the open console binary we built.
+        WEX::Logging::Log::Comment(L"Launching with OpenConsole.exe");
         value = value.Append(L"OpenConsole.exe Nihilist.exe");
     }
 

src/inc/til.h

@@ -34,3 +34,23 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
         LOG_CAUGHT_EXCEPTION();             \
         return false;                       \
     }
+
+// MultiByteToWideChar has a bug in it where it can return 0 and then not set last error.
+// WIL has a fit if the last error is 0 when a bool false is returned.
+// This macro doesn't have a fit. It just reports E_UNEXPECTED instead.
+#define THROW_LAST_ERROR_IF_AND_IGNORE_BAD_GLE(condition) \
+    do                                                    \
+    {                                                     \
+        if (condition)                                    \
+        {                                                 \
+            const auto gle = ::GetLastError();            \
+            if (gle)                                      \
+            {                                             \
+                THROW_WIN32(gle);                         \
+            }                                             \
+            else                                          \
+            {                                             \
+                THROW_HR(E_UNEXPECTED);                   \
+            }                                             \
+        }                                                 \
+    } while (0, 0)

src/types/convert.cpp

@@ -39,7 +39,7 @@ static const WORD leftShiftScanCode = 0x2A;
 
     // Ask how much space we will need.
     int const iTarget = MultiByteToWideChar(codePage, 0, source.data(), iSource, nullptr, 0);
-    THROW_LAST_ERROR_IF(0 == iTarget);
+    THROW_LAST_ERROR_IF_AND_IGNORE_BAD_GLE(0 == iTarget);
 
     size_t cchNeeded;
     THROW_IF_FAILED(IntToSizeT(iTarget, &cchNeeded));
@@ -49,7 +49,7 @@ static const WORD leftShiftScanCode = 0x2A;
     out.resize(cchNeeded);
 
     // Attempt conversion for real.
-    THROW_LAST_ERROR_IF(0 == MultiByteToWideChar(codePage, 0, source.data(), iSource, out.data(), iTarget));
+    THROW_LAST_ERROR_IF_AND_IGNORE_BAD_GLE(0 == MultiByteToWideChar(codePage, 0, source.data(), iSource, out.data(), iTarget));
 
     // Return as a string
     return out;