starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-09-25 08:24:54 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Fix conhost clipboard handling bugs (#16457)

Browse Source 
conhost has 2 bugs related to clipboard handling:
* Missing retry on `OpenClipboard`: When copying to the clipboard
  explorer.exe is very eager to open the clipboard and peek into it.
  I'm not sure why it happens, but I can see `CFSDropTarget` in the
  call stack. It uses COM RPC and so this takes ~20ms every time.
  That breaks conhost's clipboard randomly during `ConsoleBench`.
  During non-benchmarks I expect this to break during RDP.
* Missing null-terminator check during paste: `CF_UNICODETEXT` is
  documented to be a null-terminated string, which conhost v2
  failed to handle as it relied entirely on `GlobalSize`.

Additionally, this changeset simplifies the `HGLOBAL` code slightly
by adding `_copyToClipboard` to abstract it away.

## Validation Steps Performed
* `ConsoleBench` (#16453) doesn't fail randomly anymore ✅
This commit is contained in:
Leonard Hecker
2024-01-30 00:23:26 +01:00
committed by GitHub
parent 5f71cf3e94
commit 86c30bdaa2
2 changed files with 80 additions and 102 deletions
Download Patch File Download Diff File Expand all files Collapse all files

175
src/interactivity/win32/Clipboard.cpp
View File

@@ -52,33 +52,36 @@ contents and writing them to the console's input buffer
--*/
void Clipboard::Paste()
{
HANDLE ClipboardDataHandle;
const auto clipboard = _openClipboard(ServiceLocator::LocateConsoleWindow()->GetWindowHandle());
if (!clipboard)
{
LOG_LAST_ERROR();
return;
}
const auto handle = GetClipboardData(CF_UNICODETEXT);
if (!handle)
{
return;
}
// Clear any selection or scrolling that may be active.
Selection::Instance().ClearSelection();
Scrolling::s_ClearScroll();
// Get paste data from clipboard
if (!OpenClipboard(ServiceLocator::LocateConsoleWindow()->GetWindowHandle()))
const wil::unique_hglobal_locked lock{ handle };
const auto str = static_cast<const wchar_t*>(lock.get());
if (!str)
{
return;
}
ClipboardDataHandle = GetClipboardData(CF_UNICODETEXT);
if (ClipboardDataHandle == nullptr)
{
CloseClipboard();
return;
}
auto pwstr = (PWCHAR)GlobalLock(ClipboardDataHandle);
StringPaste(pwstr, (ULONG)GlobalSize(ClipboardDataHandle) / sizeof(WCHAR));
// WIP auditing if user is enrolled
GlobalUnlock(ClipboardDataHandle);
CloseClipboard();
// As per: https://learn.microsoft.com/en-us/windows/win32/dataxchg/standard-clipboard-formats
// CF_UNICODETEXT: [...] A null character signals the end of the data.
// --> Use wcsnlen() to determine the actual length.
// NOTE: Some applications don't add a trailing null character. This includes past conhost versions.
const auto maxLen = GlobalSize(handle) / sizeof(WCHAR);
StringPaste(str, wcsnlen(str, maxLen));
}
Clipboard& Clipboard::Instance()
@@ -121,6 +124,52 @@ void Clipboard::StringPaste(_In_reads_(cchData) const wchar_t* const pData,
#pragma region Private Methods
wil::unique_close_clipboard_call Clipboard::_openClipboard(HWND hwnd)
{
bool success = false;
// OpenClipboard may fail to acquire the internal lock --> retry.
for (DWORD sleep = 10;; sleep *= 2)
{
if (OpenClipboard(hwnd))
{
success = true;
break;
}
// 10 iterations
if (sleep > 10000)
{
break;
}
Sleep(sleep);
}
return wil::unique_close_clipboard_call{ success };
}
void Clipboard::_copyToClipboard(const UINT format, const void* src, const size_t bytes)
{
wil::unique_hglobal handle{ THROW_LAST_ERROR_IF_NULL(GlobalAlloc(GMEM_MOVEABLE, bytes)) };
const auto locked = GlobalLock(handle.get());
memcpy(locked, src, bytes);
GlobalUnlock(handle.get());
THROW_LAST_ERROR_IF_NULL(SetClipboardData(format, handle.get()));
handle.release();
}
void Clipboard::_copyToClipboardRegisteredFormat(const wchar_t* format, const void* src, size_t bytes)
{
const auto id = RegisterClipboardFormatW(format);
if (!id)
{
LOG_LAST_ERROR();
return;
}
_copyToClipboard(id, src, bytes);
}
// Routine Description:
// - converts a wchar_t* into a series of KeyEvents as if it was typed
// from the keyboard
@@ -260,90 +309,18 @@ void Clipboard::StoreSelectionToClipboard(const bool copyFormatting)
rtfData = buffer.GenRTF(req, fontSizePt, fontName, bgColor, isIntenseBold, GetAttributeColors);
}
CopyTextToSystemClipboard(text, htmlData, rtfData);
}
const auto clipboard = _openClipboard(ServiceLocator::LocateConsoleWindow()->GetWindowHandle());
// Routine Description:
// - Copies the text given onto the global system clipboard.
// Arguments:
// - text - plain-text data
// - htmlData - HTML copy data
// - rtfData - RTF copy data
void Clipboard::CopyTextToSystemClipboard(const std::wstring& text, const std::string& htmlData, const std::string& rtfData) const
{
// allocate the final clipboard data
const auto cchNeeded = text.size() + 1;
const auto cbNeeded = sizeof(wchar_t) * cchNeeded;
wil::unique_hglobal globalHandle(GlobalAlloc(GMEM_MOVEABLE | GMEM_DDESHARE, cbNeeded));
THROW_LAST_ERROR_IF_NULL(globalHandle.get());
EmptyClipboard();
// As per: https://learn.microsoft.com/en-us/windows/win32/dataxchg/standard-clipboard-formats
// CF_UNICODETEXT: [...] A null character signals the end of the data.
// --> We add +1 to the length. This works because .c_str() is null-terminated.
_copyToClipboard(CF_UNICODETEXT, text.c_str(), (text.size() + 1) * sizeof(wchar_t));
auto pwszClipboard = (PWSTR)GlobalLock(globalHandle.get());
THROW_LAST_ERROR_IF_NULL(pwszClipboard);
// The pattern gets a bit strange here because there's no good wil built-in for global lock of this type.
// Try to copy then immediately unlock. Don't throw until after (so the hglobal won't be freed until we unlock).
const auto hr = StringCchCopyW(pwszClipboard, cchNeeded, text.data());
GlobalUnlock(globalHandle.get());
THROW_IF_FAILED(hr);
// Set global data to clipboard
THROW_LAST_ERROR_IF(!OpenClipboard(ServiceLocator::LocateConsoleWindow()->GetWindowHandle()));
{ // Clipboard Scope
auto clipboardCloser = wil::scope_exit([]() {
THROW_LAST_ERROR_IF(!CloseClipboard());
});
THROW_LAST_ERROR_IF(!EmptyClipboard());
THROW_LAST_ERROR_IF_NULL(SetClipboardData(CF_UNICODETEXT, globalHandle.get()));
if (!htmlData.empty())
{
CopyToSystemClipboard(htmlData, L"HTML Format");
}
if (!rtfData.empty())
{
CopyToSystemClipboard(rtfData, L"Rich Text Format");
}
}
// only free if we failed.
// the memory has to remain allocated if we successfully placed it on the clipboard.
// Releasing the smart pointer will leave it allocated as we exit scope.
globalHandle.release();
}
// Routine Description:
// - Copies the given string onto the global system clipboard in the specified format
// Arguments:
// - stringToCopy - The string to copy
// - lpszFormat - the name of the format
void Clipboard::CopyToSystemClipboard(const std::string& stringToCopy, LPCWSTR lpszFormat) const
{
const auto cbData = stringToCopy.size() + 1; // +1 for '\0'
if (cbData)
if (copyFormatting)
{
wil::unique_hglobal globalHandleData(GlobalAlloc(GMEM_MOVEABLE | GMEM_DDESHARE, cbData));
THROW_LAST_ERROR_IF_NULL(globalHandleData.get());
auto pszClipboardHTML = (PSTR)GlobalLock(globalHandleData.get());
THROW_LAST_ERROR_IF_NULL(pszClipboardHTML);
// The pattern gets a bit strange here because there's no good wil built-in for global lock of this type.
// Try to copy then immediately unlock. Don't throw until after (so the hglobal won't be freed until we unlock).
const auto hr2 = StringCchCopyA(pszClipboardHTML, cbData, stringToCopy.data());
GlobalUnlock(globalHandleData.get());
THROW_IF_FAILED(hr2);
const auto CF_FORMAT = RegisterClipboardFormatW(lpszFormat);
THROW_LAST_ERROR_IF(0 == CF_FORMAT);
THROW_LAST_ERROR_IF_NULL(SetClipboardData(CF_FORMAT, globalHandleData.get()));
// only free if we failed.
// the memory has to remain allocated if we successfully placed it on the clipboard.
// Releasing the smart pointer will leave it allocated as we exit scope.
globalHandleData.release();
_copyToClipboardRegisteredFormat(L"HTML Format", htmlData.data(), htmlData.size());
_copyToClipboardRegisteredFormat(L"Rich Text Format", rtfData.data(), rtfData.size());
}
}

7
src/interactivity/win32/clipboard.hpp
View File

@@ -35,15 +35,16 @@ namespace Microsoft::Console::Interactivity::Win32
void Paste();
private:
static wil::unique_close_clipboard_call _openClipboard(HWND hwnd);
static void _copyToClipboard(UINT format, const void* src, size_t bytes);
static void _copyToClipboardRegisteredFormat(const wchar_t* format, const void* src, size_t bytes);
InputEventQueue TextToKeyEvents(_In_reads_(cchData) const wchar_t* const pData,
const size_t cchData,
const bool bracketedPaste = false);
void StoreSelectionToClipboard(_In_ const bool fAlsoCopyFormatting);
void CopyTextToSystemClipboard(const std::wstring& text, const std::string& htmlData, const std::string& rtfData) const;
void CopyToSystemClipboard(const std::string& stringToPlaceOnClip, LPCWSTR lpszFormat) const;
bool FilterCharacterOnPaste(_Inout_ WCHAR* const pwch);
#ifdef UNIT_TESTING