Fix FillConsoleOutputCharacterA crash (#4309)
## 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. ## PR Checklist * [x] Closes #4258 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed: thanks @miniksa ## Detailed Description of the Pull Request / Additional comments Following the semantics 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 Original repro no longer crashes.
This commit is contained in:
parent
a13ccfd0f5
commit
65bd4e327c
|
@ -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()
|
||||
|
|
|
@ -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();
|
||||
|
|
|
@ -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()
|
||||
|
|
|
@ -25,6 +25,7 @@ public:
|
|||
static bool TestBufferSetup();
|
||||
static bool TestBufferCleanup();
|
||||
static HANDLE _hConsole;
|
||||
static bool _isV2;
|
||||
};
|
||||
|
||||
class CommonV1V2Helper
|
||||
|
|
|
@ -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");
|
||||
}
|
||||
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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;
|
||||
|
|
Loading…
Reference in a new issue