diff --git a/src/host/ft_host/API_BufferTests.cpp b/src/host/ft_host/API_BufferTests.cpp index b583dbdb1..97efe3f53 100644 --- a/src/host/ft_host/API_BufferTests.cpp +++ b/src/host/ft_host/API_BufferTests.cpp @@ -18,6 +18,8 @@ class BufferTests TEST_METHOD(TestSetConsoleActiveScreenBufferInvalid); + TEST_METHOD(TestCookedReadOnNonShareableScreenBuffer); + BEGIN_TEST_METHOD(TestWritingInactiveScreenBuffer) TEST_METHOD_PROPERTY(L"Data:UseVtOutput", L"{true, false}") END_TEST_METHOD() @@ -33,6 +35,54 @@ void BufferTests::TestSetConsoleActiveScreenBufferInvalid() VERIFY_WIN32_BOOL_FAILED(SetConsoleActiveScreenBuffer(nullptr)); } +void BufferTests::TestCookedReadOnNonShareableScreenBuffer() +{ + Log::Comment(L"Get original handles"); + const auto in = GetStdInputHandle(); + const auto out = GetStdOutputHandle(); + + Log::Comment(L"Ensure cooked input is on (line input mode) and echoing to the screen."); + DWORD inMode = 0; + VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleMode(in, &inMode)); + inMode |= ENABLE_LINE_INPUT; + inMode |= ENABLE_ECHO_INPUT; + VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleMode(in, inMode)); + + Log::Comment(L"Create alternate buffer that is read/writeable but not shareable."); + const auto otherBuffer = CreateConsoleScreenBuffer(GENERIC_READ | GENERIC_WRITE, + 0, // This says non-sharable + nullptr, + CONSOLE_TEXTMODE_BUFFER, + nullptr); + VERIFY_WIN32_BOOL_SUCCEEDED(INVALID_HANDLE_VALUE != otherBuffer); + + Log::Comment(L"Set the alternate buffer as active."); + VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleActiveScreenBuffer(otherBuffer)); + + // On a cooked read with echoing, the act of reading from the buffer will cause a handle to be + // taken to the active output buffer such that the cooked/line reading handler can display + // what is being typed on the screen as it is being typed before the enter key is hit. + // This should fail because we've denied anyone sharing access with us and we hold the primary + // active handle above. + Log::Comment(L"Perform a read operation to attempt to take handle to output buffer and hopefully fail."); + char buffer[1]; + DWORD read = 0; + SetLastError(S_OK); + VERIFY_WIN32_BOOL_FAILED(ReadFile(in, buffer, sizeof(buffer), &read, nullptr)); + VERIFY_ARE_EQUAL(static_cast(ERROR_SHARING_VIOLATION), GetLastError()); + + Log::Comment(L"Put the buffer back."); + VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleActiveScreenBuffer(out)); + + Log::Comment(L"Close the alternate buffer."); + VERIFY_WIN32_BOOL_SUCCEEDED(CloseHandle(otherBuffer)); + + Sleep(2000); + + Log::Comment(L"Ensure that the console didn't die/crash"); + VERIFY_IS_TRUE(IsConsoleStillRunning()); +} + void BufferTests::TestWritingInactiveScreenBuffer() { bool useVtOutput; diff --git a/src/host/ft_host/Common.cpp b/src/host/ft_host/Common.cpp index 8716060d2..3107edaa8 100644 --- a/src/host/ft_host/Common.cpp +++ b/src/host/ft_host/Common.cpp @@ -7,6 +7,14 @@ using WEX::Logging::Log; using namespace WEX::Common; HANDLE Common::_hConsole = INVALID_HANDLE_VALUE; +extern wil::unique_process_information pi; + +bool IsConsoleStillRunning() +{ + DWORD exitCode = S_OK; + VERIFY_WIN32_BOOL_SUCCEEDED(GetExitCodeProcess(pi.hProcess, &exitCode)); + return exitCode == STILL_ACTIVE; +} void VerifySucceededGLE(BOOL bResult) { diff --git a/src/host/ft_host/Common.hpp b/src/host/ft_host/Common.hpp index d4eb89cd0..03efe90cf 100644 --- a/src/host/ft_host/Common.hpp +++ b/src/host/ft_host/Common.hpp @@ -68,3 +68,5 @@ BOOL UnadjustWindowRectEx( HANDLE GetStdInputHandle(); HANDLE GetStdOutputHandle(); + +bool IsConsoleStillRunning(); diff --git a/src/host/ft_host/InitTests.cpp b/src/host/ft_host/InitTests.cpp index 6af42e420..63d9e4dcc 100644 --- a/src/host/ft_host/InitTests.cpp +++ b/src/host/ft_host/InitTests.cpp @@ -16,6 +16,7 @@ static PCWSTR pwszForceV2ValueName = L"ForceV2"; // instead of using the Windows-default copy of console host. wil::unique_handle hJob; +wil::unique_process_information pi; static FILE* std_out = nullptr; static FILE* std_in = nullptr; @@ -135,7 +136,6 @@ MODULE_SETUP(ModuleSetup) // Setup and call create process. STARTUPINFOW si = { 0 }; si.cb = sizeof(STARTUPINFOW); - wil::unique_process_information pi; // We start suspended so we can put it in the job before it does anything // We say new console so it doesn't run in the same window as our test. diff --git a/src/host/ut_host/Host.UnitTests.vcxproj b/src/host/ut_host/Host.UnitTests.vcxproj index 5da6b9665..cd40d0c8b 100644 --- a/src/host/ut_host/Host.UnitTests.vcxproj +++ b/src/host/ut_host/Host.UnitTests.vcxproj @@ -16,6 +16,7 @@ + diff --git a/src/host/ut_host/Host.UnitTests.vcxproj.filters b/src/host/ut_host/Host.UnitTests.vcxproj.filters index 388cab428..4ead1ae1b 100644 --- a/src/host/ut_host/Host.UnitTests.vcxproj.filters +++ b/src/host/ut_host/Host.UnitTests.vcxproj.filters @@ -111,6 +111,9 @@ Source Files + + Source Files + diff --git a/src/host/ut_host/ObjectTests.cpp b/src/host/ut_host/ObjectTests.cpp new file mode 100644 index 000000000..9d0cca69c --- /dev/null +++ b/src/host/ut_host/ObjectTests.cpp @@ -0,0 +1,100 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "precomp.h" +#include "WexTestClass.h" +#include "..\..\inc\consoletaeftemplates.hpp" + +#include "CommonState.hpp" + +using namespace WEX::Common; +using namespace WEX::Logging; +using namespace WEX::TestExecution; +using Microsoft::Console::Interactivity::ServiceLocator; + +class ObjectTests +{ + CommonState* m_state; + + TEST_CLASS(ObjectTests); + + TEST_CLASS_SETUP(ClassSetup) + { + m_state = new CommonState(); + + m_state->InitEvents(); + m_state->PrepareGlobalFont(); + m_state->PrepareGlobalScreenBuffer(); + m_state->PrepareGlobalInputBuffer(); + + return true; + } + + TEST_CLASS_CLEANUP(ClassCleanup) + { + m_state->CleanupGlobalScreenBuffer(); + m_state->CleanupGlobalFont(); + m_state->CleanupGlobalInputBuffer(); + + delete m_state; + + return true; + } + + TEST_METHOD_SETUP(MethodSetup) + { + return true; + } + + TEST_METHOD_CLEANUP(MethodCleanup) + { + return true; + } + + TEST_METHOD(TestFailedHandleAllocationWhenNotShared) + { + Log::Comment(L"Create a new output buffer modeled from the default/active one."); + const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + const auto& existingOutput = gci.GetActiveOutputBuffer(); + + SCREEN_INFORMATION* newOutput; + + VERIFY_NT_SUCCESS(SCREEN_INFORMATION::CreateInstance(existingOutput.GetViewport().Dimensions(), + existingOutput.GetCurrentFont(), + existingOutput.GetBufferSize().Dimensions(), + existingOutput.GetAttributes(), + *existingOutput.GetPopupAttributes(), + existingOutput.GetTextBuffer().GetCursor().GetSize(), + &newOutput)); + + ConsoleObjectHeader* newOutputAsHeader = static_cast(newOutput); + + VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulOpenCount); + VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulReaderCount); + VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulReadShareCount); + VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulWriterCount); + VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulWriteShareCount); + + std::unique_ptr unsharedHandle; + VERIFY_SUCCEEDED(newOutput->AllocateIoHandle(ConsoleHandleData::HandleType::Output, + GENERIC_READ | GENERIC_WRITE, + 0, + unsharedHandle)); + unsharedHandle.release(); // leak the pointer because destruction will attempt count decrement + + VERIFY_ARE_EQUAL(1ul, newOutputAsHeader->_ulOpenCount); + VERIFY_ARE_EQUAL(1ul, newOutputAsHeader->_ulReaderCount); + VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulReadShareCount); + VERIFY_ARE_EQUAL(1ul, newOutputAsHeader->_ulWriterCount); + VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulWriteShareCount); + + std::unique_ptr secondHandleAttempt; + VERIFY_ARE_EQUAL(HRESULT_FROM_WIN32(ERROR_SHARING_VIOLATION), newOutput->AllocateIoHandle(ConsoleHandleData::HandleType::Output, GENERIC_READ | GENERIC_WRITE, 0, secondHandleAttempt)); + + VERIFY_ARE_EQUAL(1ul, newOutputAsHeader->_ulOpenCount); + VERIFY_ARE_EQUAL(1ul, newOutputAsHeader->_ulReaderCount); + VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulReadShareCount); + VERIFY_ARE_EQUAL(1ul, newOutputAsHeader->_ulWriterCount); + VERIFY_ARE_EQUAL(0ul, newOutputAsHeader->_ulWriteShareCount); + } +}; diff --git a/src/host/ut_host/sources b/src/host/ut_host/sources index 416c5ee6a..07f2e95e6 100644 --- a/src/host/ut_host/sources +++ b/src/host/ut_host/sources @@ -43,6 +43,7 @@ SOURCES = \ CommandNumberPopupTests.cpp \ CopyFromCharPopupTests.cpp \ CopyToCharPopupTests.cpp \ + ObjectTests.cpp \ DefaultResource.rc \ diff --git a/src/server/ObjectHandle.cpp b/src/server/ObjectHandle.cpp index eaef19caa..43382e517 100644 --- a/src/server/ObjectHandle.cpp +++ b/src/server/ObjectHandle.cpp @@ -12,20 +12,14 @@ #include "..\interactivity\inc\ServiceLocator.hpp" -ConsoleHandleData::ConsoleHandleData(const ULONG ulHandleType, - const ACCESS_MASK amAccess, - const ULONG ulShareAccess, - _In_ PVOID const pvClientPointer) : - _ulHandleType(ulHandleType), +ConsoleHandleData::ConsoleHandleData(const ACCESS_MASK amAccess, + const ULONG ulShareAccess) : + _ulHandleType(HandleType::NotReady), _amAccess(amAccess), _ulShareAccess(ulShareAccess), - _pvClientPointer(pvClientPointer), + _pvClientPointer(nullptr), _pClientInput(nullptr) { - if (_IsInput()) - { - _pClientInput = std::make_unique(); - } } // Routine Description: @@ -41,9 +35,36 @@ ConsoleHandleData::~ConsoleHandleData() { THROW_IF_FAILED(_CloseOutputHandle()); } - else +} + +// Routine Description: +// - Holds the client pointer handle for future use after we've determined that +// we have the privileges to grant it to a particular client. +// - This is separate from construction so this object can help with +// calculating the access type from the flags, but won't try to +// clean anything up until the ObjectHeader determines we have rights +// to use the object (and get it assigned here.) +// Arguments: +// - ulHandleType - specifies that the pointer given is input or output +// - pvClientPointer - pointer to the object that has an ObjectHeader +// Return Value: +// - - But will throw E_NOT_VALID_STATE (for trying to set twice) or +// E_INVALIDARG (for trying to set the NotReady handle type). +void ConsoleHandleData::Initialize(const ULONG ulHandleType, + PVOID const pvClientPointer) +{ + // This can only be used once and it's an erorr if we try to initialize after it's been done. + THROW_HR_IF(E_NOT_VALID_STATE, _ulHandleType != HandleType::NotReady); + + // We can't be initialized into the "not ready" state. Only constructed that way. + THROW_HR_IF(E_INVALIDARG, ulHandleType == HandleType::NotReady); + + _ulHandleType = ulHandleType; + _pvClientPointer = pvClientPointer; + + if (_IsInput()) { - FAIL_FAST_HR(E_UNEXPECTED); + _pClientInput = std::make_unique(); } } diff --git a/src/server/ObjectHandle.h b/src/server/ObjectHandle.h index 37b40a260..618d345e8 100644 --- a/src/server/ObjectHandle.h +++ b/src/server/ObjectHandle.h @@ -29,10 +29,11 @@ class SCREEN_INFORMATION; class ConsoleHandleData final { public: - ConsoleHandleData(const ULONG ulHandleType, - const ACCESS_MASK amAccess, - const ULONG ulShareAccess, - _In_ PVOID const pvClientPointer); + ConsoleHandleData(const ACCESS_MASK amAccess, + const ULONG ulShareAccess); + + void Initialize(const ULONG ulHandleType, + PVOID const pvClientPointer); ~ConsoleHandleData(); ConsoleHandleData(const ConsoleHandleData&) = delete; @@ -62,6 +63,7 @@ public: enum HandleType { + NotReady = 0x0, Input = 0x1, Output = 0x2 }; @@ -73,9 +75,9 @@ private: [[nodiscard]] HRESULT _CloseInputHandle(); [[nodiscard]] HRESULT _CloseOutputHandle(); - ULONG const _ulHandleType; ACCESS_MASK const _amAccess; ULONG const _ulShareAccess; + ULONG _ulHandleType; PVOID _pvClientPointer; // This will be a pointer to a SCREEN_INFORMATION or INPUT_INFORMATION object. std::unique_ptr _pClientInput; }; diff --git a/src/server/ObjectHeader.cpp b/src/server/ObjectHeader.cpp index 83f9319d0..aca88eadd 100644 --- a/src/server/ObjectHeader.cpp +++ b/src/server/ObjectHeader.cpp @@ -39,10 +39,8 @@ ConsoleObjectHeader::ConsoleObjectHeader() : try { // Allocate all necessary state. - std::unique_ptr pHandleData = std::make_unique(ulHandleType, - amDesired, - ulShareMode, - this); + std::unique_ptr pHandleData = std::make_unique(amDesired, + ulShareMode); // Check the share mode. if (((pHandleData->IsReadAllowed()) && (_ulOpenCount > _ulReadShareCount)) || @@ -50,7 +48,7 @@ ConsoleObjectHeader::ConsoleObjectHeader() : ((pHandleData->IsWriteAllowed()) && (_ulOpenCount > _ulWriteShareCount)) || ((!pHandleData->IsWriteShared()) && (_ulWriterCount > 0))) { - RETURN_WIN32(ERROR_SHARING_VIOLATION); + return HRESULT_FROM_WIN32(ERROR_SHARING_VIOLATION); } // Update share/open counts and store handle information. @@ -76,6 +74,10 @@ ConsoleObjectHeader::ConsoleObjectHeader() : _ulWriteShareCount++; } + // Commit the object into the handle now that we've determined we have the rights to use it and have counted up appropriately. + // This way, the handle will only try to cleanup and decrement its counts after we've validated rights and incremented. + pHandleData->Initialize(ulHandleType, this); + out.swap(pHandleData); } CATCH_RETURN(); diff --git a/src/server/ObjectHeader.h b/src/server/ObjectHeader.h index 842e4c294..f606c809a 100644 --- a/src/server/ObjectHeader.h +++ b/src/server/ObjectHeader.h @@ -47,4 +47,8 @@ private: ULONG _ulWriterCount; ULONG _ulReadShareCount; ULONG _ulWriteShareCount; + +#ifdef UNIT_TESTING + friend class ObjectTests; +#endif };