Skip to content

Commit

Permalink
Fix conhost clipboard handling bugs (#16457)
Browse files Browse the repository at this point in the history 
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.

* `ConsoleBench` (#16453) doesn't fail randomly anymore ✅

(cherry picked from commit 86c30bd)
  • Loading branch information
@lhecker
lhecker committed Jan 30, 2024
1 parent bc48eda commit faa29e8
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 108 deletions.
190 changes: 85 additions & 105 deletions src/interactivity/win32/Clipboard.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,33 +52,36 @@ contents and writing them to the console's input buffer
--*/
void Clipboard::Paste()
{
HANDLE ClipboardDataHandle;

// 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 auto clipboard = _openClipboard(ServiceLocator::LocateConsoleWindow()->GetWindowHandle());
if (!clipboard)
{
LOG_LAST_ERROR();
return;
}

ClipboardDataHandle = GetClipboardData(CF_UNICODETEXT);
if (ClipboardDataHandle == nullptr)
const auto handle = GetClipboardData(CF_UNICODETEXT);
if (!handle)
{
CloseClipboard();
return;
}

auto pwstr = (PWCHAR)GlobalLock(ClipboardDataHandle);
StringPaste(pwstr, (ULONG)GlobalSize(ClipboardDataHandle) / sizeof(WCHAR));

// WIP auditing if user is enrolled
// Clear any selection or scrolling that may be active.
Selection::Instance().ClearSelection();
Scrolling::s_ClearScroll();

GlobalUnlock(ClipboardDataHandle);
const wil::unique_hglobal_locked lock{ handle };
const auto str = static_cast<const wchar_t*>(lock.get());
if (!str)
{
return;
}

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()
Expand Down Expand Up @@ -121,6 +124,52 @@ void Clipboard::StringPaste(_In_reads_(cchData) const wchar_t* const pData,

#pragma region Private Methods

Clipboard::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 Clipboard::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
Expand Down Expand Up @@ -242,108 +291,39 @@ void Clipboard::StoreSelectionToClipboard(const bool copyFormatting)
includeCRLF = trimTrailingWhitespace = true;
}

const auto text = buffer.GetText(includeCRLF,
const auto rows = buffer.GetText(includeCRLF,
trimTrailingWhitespace,
selectionRects,
GetAttributeColors,
!selection.IsLineSelection());

CopyTextToSystemClipboard(text, copyFormatting);
}

// Routine Description:
// - Copies the text given onto the global system clipboard.
// Arguments:
// - rows - Rows of text data to copy
// - fAlsoCopyFormatting - true if the color and formatting should also be copied, false otherwise
void Clipboard::CopyTextToSystemClipboard(const TextBuffer::TextAndColor& rows, const bool fAlsoCopyFormatting)
{
std::wstring finalString;

// Concatenate strings into one giant string to put onto the clipboard.
std::wstring text;
for (const auto& str : rows.text)
{
finalString += str;
text += str;
}

// allocate the final clipboard data
const auto cchNeeded = finalString.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());

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, finalString.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 (fAlsoCopyFormatting)
{
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
const auto& fontData = gci.GetActiveOutputBuffer().GetCurrentFont();
const auto iFontHeightPoints = fontData.GetUnscaledSize().height * 72 / ServiceLocator::LocateGlobals().dpi;
const auto bgColor = gci.GetRenderSettings().GetAttributeColors({}).second;

auto HTMLToPlaceOnClip = TextBuffer::GenHTML(rows, iFontHeightPoints, fontData.GetFaceName(), bgColor);
CopyToSystemClipboard(HTMLToPlaceOnClip, L"HTML Format");
std::string htmlData, rtfData;
if (copyFormatting)
{
const auto& fontData = gci.GetActiveOutputBuffer().GetCurrentFont();
const auto iFontHeightPoints = fontData.GetUnscaledSize().height * 72 / ServiceLocator::LocateGlobals().dpi;
const auto bgColor = gci.GetRenderSettings().GetAttributeColors({}).second;

auto RTFToPlaceOnClip = TextBuffer::GenRTF(rows, iFontHeightPoints, fontData.GetFaceName(), bgColor);
CopyToSystemClipboard(RTFToPlaceOnClip, L"Rich Text Format");
}
htmlData = TextBuffer::GenHTML(rows, iFontHeightPoints, fontData.GetFaceName(), bgColor);
rtfData = TextBuffer::GenRTF(rows, iFontHeightPoints, fontData.GetFaceName(), bgColor);
}

// 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();
}
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));

// 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(std::string stringToCopy, LPCWSTR lpszFormat)
{
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());
}
}

Expand Down
8 changes: 5 additions & 3 deletions src/interactivity/win32/clipboard.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,17 @@ namespace Microsoft::Console::Interactivity::Win32
void Paste();

private:
using unique_close_clipboard_call = wil::unique_call<decltype(::CloseClipboard), &::CloseClipboard>;
static 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 TextBuffer::TextAndColor& rows, _In_ const bool copyFormatting);
void CopyToSystemClipboard(std::string stringToPlaceOnClip, LPCWSTR lpszFormat);

bool FilterCharacterOnPaste(_Inout_ WCHAR* const pwch);

#ifdef UNIT_TESTING
Expand Down

1 comment on commit faa29e8

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@check-spelling-bot Report

🔴 Please review

See the 📜action log for details.

Unrecognized words (13) 
atvis
CProc
hpj
LPCCH
NONCONST
rgi
spammy
tokeninfo
traceloggingprovider
userbase
VProc
VRaw
wcsnicmp
Previously acknowledged words that are now absent DESTINATIONNAME :arrow_right:
To accept ✔️ these unrecognized words as correct and remove the previously acknowledged and now absent words, run the following commands

... in a clone of the git@github.com:microsoft/terminal.git repository
on the dev/lhecker/fix-clipboard-bugs-1.19 branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.21/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/7711163547/attempts/1'
Pattern suggestions ✂️ (1)

You could add these patterns to .github/actions/spelling/patterns/faa29e833d08e0a20202c0dac827e73b70d85e24.txt:

# Automatically suggested patterns
# hit-count: 1 file-count: 1
# tput arguments -- https://man7.org/linux/man-pages/man5/terminfo.5.html -- technically they can be more than 5 chars long...
\btput\s+(?:(?:-[SV]|-T\s*\w+)\s+)*\w{3,5}\b
Warnings (1)

See the 📜action log for details.

ℹ️ Warnings Count
ℹ️ candidate-pattern 1

See ℹ️ Event descriptions for more information.

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. If it doesn't work for you, you can manually add (one word per line) / remove items to expect.txt and the excludes.txt files.

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

Please sign in to comment.