From aadfaada7a41513ea6ab8ad00c34f597f3ff9c6e Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 22 Aug 2020 19:24:11 +0200 Subject: [PATCH 1/4] Fix crash when using custom DnD formats under Wine Using ::HeapSize() on a global pointer is wrong, and even though it somehow still works under "genuine" MSW, it crashes under Wine. Fix this by using ::GlobalSize() instead, which is the right function to use with this kind of pointer. Thanks to Damjan Jovanovic for the analysis of the problem in Wine bugzilla (see https://bugs.winehq.org/show_bug.cgi?id=38924#c10). Closes #18887. --- src/msw/ole/dataobj.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/msw/ole/dataobj.cpp b/src/msw/ole/dataobj.cpp index e584ced903..9d3a89c16b 100644 --- a/src/msw/ole/dataobj.cpp +++ b/src/msw/ole/dataobj.cpp @@ -975,14 +975,10 @@ const void *wxDataObject::GetSizeFromBuffer(const void *buffer, size_t *size, const wxDataFormat& WXUNUSED(format)) { - // hack: the third parameter is declared non-const in Wine's headers so - // cast away the const - const size_t realsz = ::HeapSize(::GetProcessHeap(), 0, - const_cast(buffer)); - if ( realsz == (size_t)-1 ) + const size_t realsz = ::GlobalSize(::GlobalHandle(buffer)); + if ( !realsz ) { - // note that HeapSize() does not set last error - wxLogApiError(wxT("HeapSize"), 0); + wxLogLastError(wxT("GlobalSize")); return NULL; } From 0d7be7c1892bc09c796a673197c4c89b462fee9f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 22 Aug 2020 19:30:03 +0200 Subject: [PATCH 2/4] Use GlobalPtrLock instead of manual Global{Lock,Unlock}() calls Also add GlobalPtrLock::GetSize() and use it instead of calling GetSizeFromBuffer() as it's more direct and doesn't require the use of ::GlobalHandle(). --- include/wx/msw/private.h | 9 +++++++++ src/msw/ole/dataobj.cpp | 23 +++++++---------------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/include/wx/msw/private.h b/include/wx/msw/private.h index eec028852b..66fef4d9ae 100644 --- a/include/wx/msw/private.h +++ b/include/wx/msw/private.h @@ -773,6 +773,15 @@ public: void *Get() const { return m_ptr; } operator void *() const { return m_ptr; } + size_t GetSize() const + { + const size_t size = ::GlobalSize(m_hGlobal); + if ( !size ) + wxLogLastError(wxT("GlobalSize")); + + return size; + } + private: HGLOBAL m_hGlobal; void *m_ptr; diff --git a/src/msw/ole/dataobj.cpp b/src/msw/ole/dataobj.cpp index 9d3a89c16b..0531fcdaaf 100644 --- a/src/msw/ole/dataobj.cpp +++ b/src/msw/ole/dataobj.cpp @@ -713,12 +713,9 @@ STDMETHODIMP wxIDataObject::SetData(FORMATETC *pformatetc, } // copy data - const void *pBuf = GlobalLock(pmedium->hGlobal); - if ( pBuf == NULL ) { - wxLogLastError(wxT("GlobalLock")); - + GlobalPtrLock ptr(pmedium->hGlobal); + if ( !ptr ) return E_OUTOFMEMORY; - } // we've got a problem with SetData() here because the base // class version requires the size parameter which we don't @@ -731,14 +728,14 @@ STDMETHODIMP wxIDataObject::SetData(FORMATETC *pformatetc, case wxDF_HTML: case CF_TEXT: case CF_OEMTEXT: - size = strlen((const char *)pBuf); + size = strlen((const char *)ptr.Get()); break; #if !(defined(__BORLANDC__) && (__BORLANDC__ < 0x500)) case CF_UNICODETEXT: #if ( defined(__BORLANDC__) && (__BORLANDC__ > 0x530) ) - size = std::wcslen((const wchar_t *)pBuf) * sizeof(wchar_t); + size = std::wcslen((const wchar_t *)ptr.Get()) * sizeof(wchar_t); #else - size = wxWcslen((const wchar_t *)pBuf) * sizeof(wchar_t); + size = wxWcslen((const wchar_t *)ptr.Get()) * sizeof(wchar_t); #endif break; #endif @@ -759,18 +756,12 @@ STDMETHODIMP wxIDataObject::SetData(FORMATETC *pformatetc, size = sizeof(METAFILEPICT); break; default: - pBuf = m_pDataObject-> - GetSizeFromBuffer(pBuf, &size, format); + size = ptr.GetSize(); size -= m_pDataObject->GetBufferOffset(format); } - bool ok = m_pDataObject->SetData(format, size, pBuf); - - GlobalUnlock(pmedium->hGlobal); - - if ( !ok ) { + if ( !m_pDataObject->SetData(format, size, ptr.Get()) ) return E_UNEXPECTED; - } } break; From 75ad798d8b4183b427007787b6c976626a964c7e Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 22 Aug 2020 19:44:10 +0200 Subject: [PATCH 3/4] Use GlobalPtrLock instead of manual GlobalUnlock() calls Make the code safer and shorter by using RAII wrapper instead of manual Global{Lock,Unlock}() calls. No real changes. --- src/msw/clipbrd.cpp | 67 ++++++++++++++++++----------------------- src/msw/dcprint.cpp | 15 +++++---- src/msw/ole/dataobj.cpp | 26 +++++----------- src/msw/printdlg.cpp | 6 ++-- 4 files changed, 49 insertions(+), 65 deletions(-) diff --git a/src/msw/clipbrd.cpp b/src/msw/clipbrd.cpp index ea77bd4182..357e0c5674 100644 --- a/src/msw/clipbrd.cpp +++ b/src/msw/clipbrd.cpp @@ -245,9 +245,12 @@ bool wxSetClipboardData(wxDataFormat dataFormat, abs(ds.dsBmih.biHeight); HANDLE hMem; hMem = ::GlobalAlloc(GHND, ds.dsBmih.biSize + numColors*sizeof(RGBQUAD) + bmpSize); - if ( hMem ) + if ( !hMem ) + break; + { - char* pDst = (char*)::GlobalLock(hMem); + GlobalPtrLock ptr(hMem); + char* pDst = (char*)ptr.Get(); memcpy(pDst, &ds.dsBmih, ds.dsBmih.biSize); pDst += ds.dsBmih.biSize; if ( numColors > 0 ) @@ -259,9 +262,9 @@ bool wxSetClipboardData(wxDataFormat dataFormat, pDst += numColors*sizeof(RGBQUAD); } memcpy(pDst, dib.GetData(), bmpSize); - ::GlobalUnlock(hMem); - handle = ::SetClipboardData(CF_DIB, hMem); - } + } // unlock hMem + + handle = ::SetClipboardData(CF_DIB, hMem); } } break; @@ -276,14 +279,16 @@ bool wxSetClipboardData(wxDataFormat dataFormat, { wxMetafile *wxMF = (wxMetafile *)data; HANDLE data = GlobalAlloc(GHND, sizeof(METAFILEPICT) + 1); - METAFILEPICT *mf = (METAFILEPICT *)GlobalLock(data); + { + GlobalPtrLock ptr(data); + METAFILEPICT *mf = (METAFILEPICT *)data.Get(); - mf->mm = wxMF->GetWindowsMappingMode(); - mf->xExt = width; - mf->yExt = height; - mf->hMF = (HMETAFILE) wxMF->GetHMETAFILE(); - GlobalUnlock(data); - wxMF->SetHMETAFILE((WXHANDLE) NULL); + mf->mm = wxMF->GetWindowsMappingMode(); + mf->xExt = width; + mf->yExt = height; + mf->hMF = (HMETAFILE) wxMF->GetHMETAFILE(); + wxMF->SetHMETAFILE((WXHANDLE) NULL); + } // unlock data handle = SetClipboardData(CF_METAFILEPICT, data); break; @@ -326,11 +331,7 @@ bool wxSetClipboardData(wxDataFormat dataFormat, HANDLE hGlobalMemory = GlobalAlloc(GHND, l); if ( hGlobalMemory ) { - LPSTR lpGlobalMemory = (LPSTR)GlobalLock(hGlobalMemory); - - memcpy(lpGlobalMemory, s, l); - - GlobalUnlock(hGlobalMemory); + memcpy(GlobalPtrLock(hGlobalMemory), s, l); } handle = SetClipboardData(dataFormat, hGlobalMemory); @@ -344,9 +345,7 @@ bool wxSetClipboardData(wxDataFormat dataFormat, HANDLE hGlobalMemory = ::GlobalAlloc(GHND, size); if ( hGlobalMemory ) { - LPWSTR lpGlobalMemory = (LPWSTR)::GlobalLock(hGlobalMemory); - memcpy(lpGlobalMemory, s, size); - ::GlobalUnlock(hGlobalMemory); + memcpy(GlobalPtrLock(hGlobalMemory), s, size); } handle = ::SetClipboardData(CF_UNICODETEXT, hGlobalMemory); @@ -407,9 +406,7 @@ bool wxSetClipboardData(wxDataFormat dataFormat, HGLOBAL hText = GlobalAlloc(GMEM_MOVEABLE |GMEM_DDESHARE, strlen(buf)+4); // Put your string in the global memory... - ptr = (char *)GlobalLock(hText); - strcpy(ptr, buf); - GlobalUnlock(hText); + strcpy((char*)GlobalPtrLock(hText).Get(), buf); handle = ::SetClipboardData(gs_htmlcfid, hText); @@ -861,11 +858,9 @@ bool wxClipboard::GetData( wxDataObject& data ) if ( hMem ) { wxTextDataObject& textDataObject = (wxTextDataObject &)data; - const void* buf = ::GlobalLock(hMem); - DWORD size = ::GlobalSize(hMem); - bool ok = textDataObject.SetData(size, buf); - ::GlobalUnlock(hMem); - return ok; + + GlobalPtrLock ptr(hMem); + return textDataObject.SetData(ptr.GetSize(), ptr); } } break; @@ -885,11 +880,9 @@ bool wxClipboard::GetData( wxDataObject& data ) if ( hMem ) { wxBitmapDataObject& bitmapDataObject = (wxBitmapDataObject &)data; - const void* buf = ::GlobalLock(hMem); - DWORD size = ::GlobalSize(hMem); - bool ok = bitmapDataObject.SetData(size, buf); - ::GlobalUnlock(hMem); - return ok; + + GlobalPtrLock ptr(hMem); + return bitmapDataObject.SetData(ptr.GetSize(), ptr); } } break; @@ -901,11 +894,9 @@ bool wxClipboard::GetData( wxDataObject& data ) if ( hMem ) { wxMetafileDataObject& metaFileDataObject = (wxMetafileDataObject &)data; - const void* buf = ::GlobalLock(hMem); - DWORD size = ::GlobalSize(hMem); - bool ok = metaFileDataObject.SetData(wxDF_METAFILE, size, buf); - ::GlobalUnlock(hMem); - return ok; + + GlobalPtrLock ptr(hMem); + return metaFileDataObject.SetData(wxDF_METAFILE, ptr.GetSize(), ptr); } } break; diff --git a/src/msw/dcprint.cpp b/src/msw/dcprint.cpp index 2bcbea8e2b..977ac9d00e 100644 --- a/src/msw/dcprint.cpp +++ b/src/msw/dcprint.cpp @@ -266,14 +266,17 @@ static bool wxGetDefaultDeviceName(wxString& deviceName, wxString& portName) if (pd.hDevNames) { - lpDevNames = (LPDEVNAMES)GlobalLock(pd.hDevNames); - lpszDeviceName = (LPTSTR)lpDevNames + lpDevNames->wDeviceOffset; - lpszPortName = (LPTSTR)lpDevNames + lpDevNames->wOutputOffset; + { + GlobalPtrLock ptr(pd.hDevNames); - deviceName = lpszDeviceName; - portName = lpszPortName; + lpDevNames = (LPDEVNAMES)ptr.Get(); + lpszDeviceName = (LPTSTR)lpDevNames + lpDevNames->wDeviceOffset; + lpszPortName = (LPTSTR)lpDevNames + lpDevNames->wOutputOffset; + + deviceName = lpszDeviceName; + portName = lpszPortName; + } // unlock pd.hDevNames - GlobalUnlock(pd.hDevNames); GlobalFree(pd.hDevNames); pd.hDevNames=NULL; } diff --git a/src/msw/ole/dataobj.cpp b/src/msw/ole/dataobj.cpp index 0531fcdaaf..5eea992ac7 100644 --- a/src/msw/ole/dataobj.cpp +++ b/src/msw/ole/dataobj.cpp @@ -95,16 +95,15 @@ HGLOBAL wxGlobalClone(HGLOBAL hglobIn) { HGLOBAL hglobOut = NULL; - LPVOID pvIn = GlobalLock(hglobIn); - if (pvIn) + GlobalPtrLock ptrIn(hglobIn); + if (ptrIn) { - SIZE_T cb = GlobalSize(hglobIn); + SIZE_T cb = ptrIn.GetSize(); hglobOut = GlobalAlloc(GMEM_FIXED, cb); if (hglobOut) { - CopyMemory(hglobOut, pvIn, cb); + CopyMemory(hglobOut, ptrIn, cb); } - GlobalUnlock(hglobIn); } return hglobOut; @@ -637,27 +636,18 @@ STDMETHODIMP wxIDataObject::GetDataHere(FORMATETC *pformatetc, case TYMED_HGLOBAL: { // copy data - HGLOBAL hGlobal = pmedium->hGlobal; - void *pBuf = GlobalLock(hGlobal); - if ( pBuf == NULL ) { - wxLogLastError(wxT("GlobalLock")); + GlobalPtrLock ptr(pmedium->hGlobal); + if ( !ptr ) return E_OUTOFMEMORY; - } wxDataFormat format = pformatetc->cfFormat; // possibly put the size in the beginning of the buffer - pBuf = m_pDataObject->SetSizeInBuffer - ( - pBuf, - ::GlobalSize(hGlobal), - format - ); + void* const pBuf = + m_pDataObject->SetSizeInBuffer(ptr, ptr.GetSize(), format); if ( !m_pDataObject->GetDataHere(format, pBuf) ) return E_UNEXPECTED; - - GlobalUnlock(hGlobal); } break; diff --git a/src/msw/printdlg.cpp b/src/msw/printdlg.cpp index 5074092790..b6c3f8eec8 100644 --- a/src/msw/printdlg.cpp +++ b/src/msw/printdlg.cpp @@ -147,7 +147,9 @@ wxCreateDevNames(const wxString& driverName, ( driverName.length() + 1 + printerName.length() + 1 + portName.length()+1 ) * sizeof(wxChar) ); - LPDEVNAMES lpDev = (LPDEVNAMES)GlobalLock(hDev); + + GlobalPtrLock ptr(hDev); + LPDEVNAMES lpDev = (LPDEVNAMES)ptr.Get(); lpDev->wDriverOffset = sizeof(WORD) * 4 / sizeof(wxChar); wxStrcpy((wxChar*)lpDev + lpDev->wDriverOffset, driverName); @@ -160,8 +162,6 @@ wxCreateDevNames(const wxString& driverName, wxStrcpy((wxChar*)lpDev + lpDev->wOutputOffset, portName); lpDev->wDefault = 0; - - GlobalUnlock(hDev); } return hDev; From 1bf76ee5945451939894dfb805cee47c9f7d8705 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 22 Aug 2020 23:48:58 +0200 Subject: [PATCH 4/4] Add a safety check for the buffer size in wxIDataObject Avoid size overflow if the offset value is greater than it. --- src/msw/ole/dataobj.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/msw/ole/dataobj.cpp b/src/msw/ole/dataobj.cpp index 5eea992ac7..6dbeae8088 100644 --- a/src/msw/ole/dataobj.cpp +++ b/src/msw/ole/dataobj.cpp @@ -745,9 +745,20 @@ STDMETHODIMP wxIDataObject::SetData(FORMATETC *pformatetc, case CF_METAFILEPICT: size = sizeof(METAFILEPICT); break; + default: size = ptr.GetSize(); - size -= m_pDataObject->GetBufferOffset(format); + + // Account for the possible offset. + const size_t + ofs = m_pDataObject->GetBufferOffset(format); + + // Check that it has a reasonable value to avoid + // overflow. + if ( ofs > size ) + return E_UNEXPECTED; + + size -= ofs; } if ( !m_pDataObject->SetData(format, size, ptr.Get()) )