Skip to content

Commit 5899343

Browse files
authored
Fix a race condition around Open/CloseClipboard (#19297)
tl;dr: Open/CloseClipboard are surprisingly not thread-safe. ## Validation Steps Performed * Copy a large amount of text (>1MB) * Run `edit.exe` * Press and hold Ctrl+Shift+V * Doesn't crash ✅
1 parent 1283c0f commit 5899343

File tree

1 file changed

+41
-2
lines changed

1 file changed

+41
-2
lines changed

src/cascadia/TerminalApp/TerminalPage.cpp

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,42 @@ namespace winrt
6060

6161
namespace clipboard
6262
{
63-
wil::unique_close_clipboard_call open(HWND hwnd)
63+
static SRWLOCK lock = SRWLOCK_INIT;
64+
65+
struct ClipboardHandle
66+
{
67+
explicit ClipboardHandle(bool open) :
68+
_open{ open }
69+
{
70+
}
71+
72+
~ClipboardHandle()
73+
{
74+
if (_open)
75+
{
76+
ReleaseSRWLockExclusive(&lock);
77+
CloseClipboard();
78+
}
79+
}
80+
81+
explicit operator bool() const noexcept
82+
{
83+
return _open;
84+
}
85+
86+
private:
87+
bool _open = false;
88+
};
89+
90+
ClipboardHandle open(HWND hwnd)
6491
{
92+
// Turns out, OpenClipboard/CloseClipboard are not thread-safe whatsoever,
93+
// and on CloseClipboard, the GetClipboardData handle may get freed.
94+
// The problem is that WinUI also uses OpenClipboard (through WinRT which uses OLE),
95+
// and so even with this mutex we can still crash randomly if you copy something via WinUI.
96+
// Makes you wonder how many Windows apps are subtly broken, huh.
97+
AcquireSRWLockExclusive(&lock);
98+
6599
bool success = false;
66100

67101
// OpenClipboard may fail to acquire the internal lock --> retry.
@@ -80,7 +114,12 @@ namespace clipboard
80114
Sleep(sleep);
81115
}
82116

83-
return wil::unique_close_clipboard_call{ success };
117+
if (!success)
118+
{
119+
ReleaseSRWLockExclusive(&lock);
120+
}
121+
122+
return ClipboardHandle{ success };
84123
}
85124

86125
void write(wil::zwstring_view text, std::string_view html, std::string_view rtf)

0 commit comments

Comments
 (0)