From cfb3ef98fc80d01391794baf7309202ebb634297 Mon Sep 17 00:00:00 2001 From: PB Date: Sun, 16 Jul 2017 15:39:26 +0200 Subject: [PATCH] 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 // ----------------------------------------------------------------------------