From cfb3ef98fc80d01391794baf7309202ebb634297 Mon Sep 17 00:00:00 2001 From: PB Date: Sun, 16 Jul 2017 15:39:26 +0200 Subject: [PATCH 1/3] Make wxBasicString as simple and safe to use as possible Don't try to write a general purpose class, but provide just the methods that we need in our code. This fixes the bug added in 294436c8bbef3e74780e32fb327b19e253b7c10b which resulted in a crash because a literal string, not a BSTR, was passed to wxBasicString ctor. --- include/wx/msw/ole/oleutils.h | 44 ++++++++++++++--------------- src/msw/ole/automtn.cpp | 6 ++-- src/msw/ole/oleutils.cpp | 52 ++++------------------------------- 3 files changed, 29 insertions(+), 73 deletions(-) diff --git a/include/wx/msw/ole/oleutils.h b/include/wx/msw/ole/oleutils.h index d207eeaf78..f334a7a289 100644 --- a/include/wx/msw/ole/oleutils.h +++ b/include/wx/msw/ole/oleutils.h @@ -65,47 +65,43 @@ inline void wxOleUninitialize() class WXDLLIMPEXP_CORE wxBasicString { -public: - // Takes over the ownership of bstr - explicit wxBasicString(BSTR bstr = NULL); +public: + // Constructs with the owned BSTR set to NULL + wxBasicString() : m_bstrBuf(NULL) {} - // Constructs the BSTR from wxString - wxBasicString(const wxString& str); + // Constructs with the owned BSTR created from a wxString + wxBasicString(const wxString& str) + : m_bstrBuf(SysAllocString(str.wc_str(*wxConvCurrent))) {} - // Creates a copy of the BSTR owned by bstr - wxBasicString(const wxBasicString& bstr); + // Constructs with the owned BSTR as a copy of the BSTR owned by bstr + wxBasicString(const wxBasicString& bstr) : m_bstrBuf(bstr.Copy()) {} // Frees the owned BSTR - ~wxBasicString(); + ~wxBasicString() { SysFreeString(m_bstrBuf); } - // Returns the owned BSTR and gives up its ownership + // Creates the owned BSTR from a wxString + void AssignFromString(const wxString& str); + + // Returns the owned BSTR and gives up its ownership, + // the caller is responsible for freeing it BSTR Detach(); - // Frees the owned BSTR - void Free(); - - // Returns a copy of the owned BSTR, - // the caller is responsible for freeing it + // Returns a copy of the owned BSTR, + // the caller is responsible for freeing it BSTR Copy() const { return SysAllocString(m_bstrBuf); } - // Returns the address of the owned BSTR, not to be called - // when wxBasicString already contains a non-NULL BSTR + // Returns the address of the owned BSTR, not to be called + // when wxBasicString already contains a non-NULL BSTR BSTR* ByRef(); // Sets its BSTR to a copy of the BSTR owned by bstr wxBasicString& operator=(const wxBasicString& bstr); - // Creates its BSTR from wxString - wxBasicString& operator=(const wxString& str); - - // Takes over the ownership of bstr - wxBasicString& operator=(BSTR bstr); - - /// Returns the owned BSTR while keeping its ownership + /// Returns the owned BSTR while keeping its ownership operator BSTR() const { return m_bstrBuf; } // retrieve a copy of our string - caller must SysFreeString() it later! - wxDEPRECATED_MSG("use Copy() instead") + wxDEPRECATED_MSG("use Copy() instead") BSTR Get() const { return Copy(); } private: // actual string diff --git a/src/msw/ole/automtn.cpp b/src/msw/ole/automtn.cpp index fdbd9f4d4b..4d091c5285 100644 --- a/src/msw/ole/automtn.cpp +++ b/src/msw/ole/automtn.cpp @@ -130,8 +130,8 @@ bool wxAutomationObject::Invoke(const wxString& member, int action, } int namedArgStringCount = namedArgCount + 1; - wxVector argNames(namedArgStringCount, wxString()); - argNames[0] = member; + wxVector argNames(namedArgStringCount); + argNames[0].AssignFromString(member); // Note that arguments are specified in reverse order // (all totally logical; hey, we're dealing with OLE here.) @@ -141,7 +141,7 @@ bool wxAutomationObject::Invoke(const wxString& member, int action, { if ( !INVOKEARG(i).GetName().empty() ) { - argNames[(namedArgCount-j)] = INVOKEARG(i).GetName(); + argNames[(namedArgCount-j)].AssignFromString(INVOKEARG(i).GetName()); j ++; } } diff --git a/src/msw/ole/oleutils.cpp b/src/msw/ole/oleutils.cpp index d1378fdecf..5bce2f3062 100644 --- a/src/msw/ole/oleutils.cpp +++ b/src/msw/ole/oleutils.cpp @@ -70,27 +70,12 @@ WXDLLEXPORT wxString wxConvertStringFromOle(BSTR bStr) // ---------------------------------------------------------------------------- // wxBasicString // ---------------------------------------------------------------------------- - -wxBasicString::wxBasicString(BSTR bstr) -{ - m_bstrBuf = bstr; -} - -wxBasicString::wxBasicString(const wxString& str) -{ - m_bstrBuf = SysAllocString(str.wc_str(*wxConvCurrent)); -} - -wxBasicString::wxBasicString(const wxBasicString& bstr) -{ - m_bstrBuf = bstr.Copy(); -} - -wxBasicString::~wxBasicString() -{ - SysFreeString(m_bstrBuf); -} - +void wxBasicString::AssignFromString(const wxString& str) +{ + SysFreeString(m_bstrBuf); + m_bstrBuf = SysAllocString(str.wc_str(*wxConvCurrent)); +} + BSTR wxBasicString::Detach() { BSTR bstr = m_bstrBuf; @@ -100,12 +85,6 @@ BSTR wxBasicString::Detach() return bstr; } -void wxBasicString::Free() -{ - SysFreeString(m_bstrBuf); - m_bstrBuf = NULL; -} - BSTR* wxBasicString::ByRef() { wxASSERT_MSG(!m_bstrBuf, @@ -126,25 +105,6 @@ wxBasicString& wxBasicString::operator=(const wxBasicString& src) return *this; } -wxBasicString& wxBasicString::operator=(const wxString& str) -{ - SysFreeString(m_bstrBuf); - m_bstrBuf = SysAllocString(str.wc_str(*wxConvCurrent)); - - return *this; -} - -wxBasicString& wxBasicString::operator=(BSTR bstr) -{ - wxCHECK_MSG(m_bstrBuf == NULL || m_bstrBuf != bstr, - *this, wxS("Attempting to assign already owned BSTR")); - - SysFreeString(m_bstrBuf); - m_bstrBuf = bstr; - - return *this; -} - // ---------------------------------------------------------------------------- // Convert variants // ---------------------------------------------------------------------------- From 4ebef67e3e28ceb658b06eb1dc0748abca34a82e Mon Sep 17 00:00:00 2001 From: PB Date: Sun, 16 Jul 2017 15:39:26 +0200 Subject: [PATCH 2/3] Update update constructing wxBasicString from literal strings No real changes, just replace wxT() with wxS() as wxBasicString ctor now that it takes wxString and not "const wchar_t*" and remove the now unnecessary explicit "wxString()". --- src/msw/mediactrl_wmp10.cpp | 6 +++--- src/msw/webview_ie.cpp | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/msw/mediactrl_wmp10.cpp b/src/msw/mediactrl_wmp10.cpp index da014b8644..581d7076de 100644 --- a/src/msw/mediactrl_wmp10.cpp +++ b/src/msw/mediactrl_wmp10.cpp @@ -1061,12 +1061,12 @@ bool wxWMP10MediaBackend::ShowPlayerControls(wxMediaCtrlPlayerControls flags) if(!flags) { m_pWMPPlayer->put_enabled(VARIANT_FALSE); - m_pWMPPlayer->put_uiMode(wxBasicString(wxT("none"))); + m_pWMPPlayer->put_uiMode(wxBasicString(wxS("none"))); } else { // TODO: use "custom"? (note that CE only supports none/full) - m_pWMPPlayer->put_uiMode(wxBasicString(wxT("full"))); + m_pWMPPlayer->put_uiMode(wxBasicString(wxS("full"))); m_pWMPPlayer->put_enabled(VARIANT_TRUE); } @@ -1358,7 +1358,7 @@ wxLongLong wxWMP10MediaBackend::GetDownloadTotal() if(m_pWMPPlayer->get_currentMedia(&pWMPMedia) == 0) { BSTR bsOut; - pWMPMedia->getItemInfo(wxBasicString(wxT("FileSize")), + pWMPMedia->getItemInfo(wxBasicString(wxS("FileSize")), &bsOut); wxString sFileSize = wxConvertStringFromOle(bsOut); diff --git a/src/msw/webview_ie.cpp b/src/msw/webview_ie.cpp index 79ea18b78f..848068d59d 100644 --- a/src/msw/webview_ie.cpp +++ b/src/msw/webview_ie.cpp @@ -1009,8 +1009,8 @@ void wxWebViewIE::FindInternal(const wxString& text, int flags, int internal_fla if(SUCCEEDED(document->QueryInterface(wxIID_IMarkupContainer, (void **)&pIMC))) { wxCOMPtr ptrBegin, ptrEnd; - wxBasicString attr_bstr(wxString("style=\"background-color:#ffff00\"")); - wxBasicString text_bstr(text.wc_str()); + wxBasicString attr_bstr(wxS("style=\"background-color:#ffff00\"")); + wxBasicString text_bstr(text); pIMS->CreateMarkupPointer(&ptrBegin); pIMS->CreateMarkupPointer(&ptrEnd); From 10f7d3569447b8aaf2c61ee84bc19f0edef12728 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 16 Jul 2017 16:18:40 +0200 Subject: [PATCH 3/3] Remove unwanted CR characters from MSW OLE source file Somehow recent changes to this file added CRs (^M) characters to the ends of some lines, remove them to avoid having a mix of Unix and DOS EOLs in the same file. No real changes. --- src/msw/ole/oleutils.cpp | 50 ++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/src/msw/ole/oleutils.cpp b/src/msw/ole/oleutils.cpp index 5bce2f3062..21bf29fb17 100644 --- a/src/msw/ole/oleutils.cpp +++ b/src/msw/ole/oleutils.cpp @@ -70,38 +70,38 @@ WXDLLEXPORT wxString wxConvertStringFromOle(BSTR bStr) // ---------------------------------------------------------------------------- // wxBasicString // ---------------------------------------------------------------------------- -void wxBasicString::AssignFromString(const wxString& str) -{ - SysFreeString(m_bstrBuf); - m_bstrBuf = SysAllocString(str.wc_str(*wxConvCurrent)); -} - -BSTR wxBasicString::Detach() -{ - BSTR bstr = m_bstrBuf; - - m_bstrBuf = NULL; - - return bstr; +void wxBasicString::AssignFromString(const wxString& str) +{ + SysFreeString(m_bstrBuf); + m_bstrBuf = SysAllocString(str.wc_str(*wxConvCurrent)); } -BSTR* wxBasicString::ByRef() -{ +BSTR wxBasicString::Detach() +{ + BSTR bstr = m_bstrBuf; + + m_bstrBuf = NULL; + + return bstr; +} + +BSTR* wxBasicString::ByRef() +{ wxASSERT_MSG(!m_bstrBuf, - wxS("Can't get direct access to initialized BSTR")); - return &m_bstrBuf; + wxS("Can't get direct access to initialized BSTR")); + return &m_bstrBuf; } wxBasicString& wxBasicString::operator=(const wxBasicString& src) { - if ( this != &src ) - { - wxCHECK_MSG(m_bstrBuf == NULL || m_bstrBuf != src.m_bstrBuf, - *this, wxS("Attempting to assign already owned BSTR")); - SysFreeString(m_bstrBuf); - m_bstrBuf = src.Copy(); - } - + if ( this != &src ) + { + wxCHECK_MSG(m_bstrBuf == NULL || m_bstrBuf != src.m_bstrBuf, + *this, wxS("Attempting to assign already owned BSTR")); + SysFreeString(m_bstrBuf); + m_bstrBuf = src.Copy(); + } + return *this; }