Hi!
While testing fixes for #2302 I noticed a Win32-specific issue during large or slow paste operations.
ScintillaWin::Paste keeps the clipboard open while the document is modified. With a large rectangular paste, especially one that takes a long time, this makes the clipboard unavailable to the whole system until the paste operation finishes.
I attached a Win32 patch for this.
The patch copies CF_UNICODETEXT into a local std::string while the clipboard is open, then closes the clipboard before starting the undo group, clearing the selection, and inserting the paste text. This also seems to be closer to the way the other platform implementations work, where the paste data is copied before InsertPasteShape is called.
The paste flags are still read before closing the clipboard.
The text read is now also bounded by GlobalSize() instead of relying on the NUL-terminated read from the clipboard memory.
This does not change the final inserted document content. It only changes when the clipboard is released.
I also attached a separate Win32 drop patch. It adds the same bounds for reading CF_UNICODETEXT in ScintillaWin::Drop by limiting the scan to GlobalSize() / sizeof(wchar_t). It also moves ReleaseStgMedium(&medium) before the early return for empty dropped text, so the medium is released in that path too. Just in case you find it helpful.
--- scintilla/win32/ScintillaWin.cxx
+++ scintilla/win32/ScintillaWin.cxx
@@ -3074,36 +3074,46 @@ bool SupportedFormat(const FORMATETC *pFE) noexcept {
}
void ScintillaWin::Paste() {
- Clipboard clipboard(MainHWND());
- if (!clipboard) {
- return;
- }
- UndoGroup ug(pdoc);
- const bool isLine = SelectionEmpty() &&
- (::IsClipboardFormatAvailable(cfLineSelect) || ::IsClipboardFormatAvailable(cfVSLineTag));
- ClearSelection(multiPasteMode == MultiPaste::Each);
- bool isRectangular = (::IsClipboardFormatAvailable(cfColumnSelect) != 0);
-
- if (!isRectangular) {
- // Evaluate "Borland IDE Block Type" explicitly
- GlobalMemory memBorlandSelection(::GetClipboardData(cfBorlandIDEBlockType));
- if (memBorlandSelection) {
- isRectangular = (memBorlandSelection.Size() == 1) && (static_cast<BYTE *>(memBorlandSelection.ptr)[0] == 0x02);
- memBorlandSelection.Unlock();
+ bool isLine = false;
+ bool isRectangular = false;
+ bool hasUnicodeText = false;
+ std::string putf;
+
+ {
+ Clipboard clipboard(MainHWND());
+ if (!clipboard) {
+ return;
}
- }
- const PasteShape pasteShape = isRectangular ? PasteShape::rectangular : (isLine ? PasteShape::line : PasteShape::stream);
+
+ isLine = SelectionEmpty() &&
+ (::IsClipboardFormatAvailable(cfLineSelect) || ::IsClipboardFormatAvailable(cfVSLineTag));
+ isRectangular = (::IsClipboardFormatAvailable(cfColumnSelect) != 0);
+
+ if (!isRectangular) {
+ // Evaluate "Borland IDE Block Type" explicitly
+ GlobalMemory memBorlandSelection(::GetClipboardData(cfBorlandIDEBlockType));
+ if (memBorlandSelection) {
+ isRectangular = (memBorlandSelection.Size() == 1) && (static_cast<BYTE *>(memBorlandSelection.ptr)[0] == 0x02);
+ memBorlandSelection.Unlock();
+ }
+ }
- // Use CF_UNICODETEXT if available
- GlobalMemory memUSelection(::GetClipboardData(CF_UNICODETEXT));
- if (const wchar_t *uptr = static_cast<const wchar_t *>(memUSelection.ptr)) {
- const std::string putf = EncodeWString(uptr);
+ // Use CF_UNICODETEXT if available
+ GlobalMemory memUSelection(::GetClipboardData(CF_UNICODETEXT));
+ if (const wchar_t *uptr = static_cast<const wchar_t *>(memUSelection.ptr)) {
+ hasUnicodeText = true;
+ const size_t uchars = memUSelection.Size() / sizeof(wchar_t);
+ const wchar_t *const uptrEnd = std::find(uptr, uptr + uchars, L'\0');
+ putf = EncodeWString(std::wstring_view(uptr, uptrEnd - uptr));
+ memUSelection.Unlock();
+ }
+ }
+
+ if (hasUnicodeText) {
+ UndoGroup ug(pdoc);
+ const PasteShape pasteShape = isRectangular ? PasteShape::rectangular : (isLine ? PasteShape::line : PasteShape::stream);
+ ClearSelection(multiPasteMode == MultiPaste::Each);
InsertPasteShape(putf.c_str(), putf.length(), pasteShape);
- memUSelection.Unlock();
}
Redraw();
}
--- scintilla/win32/ScintillaWin.cxx
+++ scintilla/win32/ScintillaWin.cxx
@@ -3815,14 +3815,17 @@ STDMETHODIMP ScintillaWin::Drop(LPDATAOBJECT pIDataSource, DWORD grfKeyState,
if (medium.hGlobal) {
GlobalMemory memUDrop(medium.hGlobal);
if (const wchar_t *uptr = static_cast<const wchar_t *>(memUDrop.ptr)) {
- putf = EncodeWString(uptr);
+ const size_t uchars = memUDrop.Size() / sizeof(wchar_t);
+ const wchar_t *const uptrEnd = std::find(uptr, uptr + uchars, L'\0');
+ putf = EncodeWString(std::wstring_view(uptr, uptrEnd - uptr));
}
memUDrop.Unlock();
}
+ ::ReleaseStgMedium(&medium);
if (putf.empty()) {
return S_OK;
@@ -3835,9 +3838,6 @@ STDMETHODIMP ScintillaWin::Drop(LPDATAOBJECT pIDataSource, DWORD grfKeyState,
DropAt(movePos, putf.c_str(), putf.size(), *pdwEffect == DROPEFFECT_MOVE, isRectangular);
-
- // Free data
- ::ReleaseStgMedium(&medium);
return S_OK;
This seems unnecessary. The relevant documentation for
CF_UNICODETEXTdefines the presence of a terminating NUL character.https://learn.microsoft.com/en-us/windows/win32/dataxchg/standard-clipboard-formats
If there is a problem with non-NUL terminated blocks that should be an independent issue and fix. Changing multiple elements in a single issue / patch make it more difficult to understand for both the reviewer and future maintainers.
Sorry for that additional noise. Got carried away.
Here are the two cleaned patches: