Fix conhost clipboard handling bugs

src/interactivity/win32/Clipboard.cpp

@@ -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()
@@ -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(L"HTML 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());
-
-    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()));
+    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));
 
-        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());
     }
 }
 

src/interactivity/win32/clipboard.hpp

@@ -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