11

What is the correct way of doing this:

_bstr_t description;
errorInfo->GetDescription( &description.GetBSTR() );

or:

_bstr_t description;
errorInfo->GetDescription( description.GetAddress() );

Where IError:GetDescription is defined as:

HRESULT GetDescription (BSTR *pbstrDescription);

I know I could easily do this:

BSTR description= SysAllocString (L"Whateva"));
errorInfo->GetDescription (&description);
SysFreeString (description);

Thanks

bluish
  • 26,356
  • 27
  • 122
  • 180
Asim
  • 869
  • 1
  • 10
  • 17

6 Answers6

9

The BSTR is reference counted, I seriously doubt that will work right if you use GetAddress(). Sadly the source code isn't available to double-check that. I've always done it like this:

BSTR temp = 0;
HRESULT hr = p->GetDescription(&temp);
if (SUCCEEDED(hr)) {
    _bstr_t wrap(temp, FALSE);
    // etc..
}
Hans Passant
  • 922,412
  • 146
  • 1,693
  • 2,536
  • +1, the talk of the _bstr_t's BSTR being shared by other instances put me off anything that might assign directly to it. – Leo Davidson Dec 03 '10 at 15:56
  • 1
    shouldn't you use `Attach()` instead of assignment operator? – Afriza N. Arief Jan 22 '13 at 16:36
  • As shown, the function `GetDescription` allocates memory to `temp` with `SysAllocString`, and that memory is never freed. Either, you must call `SysFreeString(temp)`, or make sure that `wrap` attaches to that memory and frees it. – abelenky Jun 30 '14 at 15:59
  • not helping, if you mixing ref-counted _bstr_t still with the macro SUCCEEDED. The aim of using _bstr_t is RAII – caoanan Nov 24 '21 at 11:54
4

A late answer that may not apply to earlier (or later) versions of Visual Studio; however, VS 12.0 has the _bstr_t implementation inline, and evidently an internal Data_t instance is created with a m_RefCount of 1 when calling GetBSTR() on a virgin _bstr_t. So the _bstr_t lifecycle in your first example looks to be okay:

_bstr_t description;
errorInfo->GetDescription( &description.GetBSTR() );

But if _bstr_t is dirty, the existing internal m_wstr pointer will be overwritten, leaking the previous memory it referenced.

By using the following operator&, a dirty _bstr_t can be used given that it's first cleared via Assign(nullptr). The overload also provides the convenience of utilizing the address operator instead of GetBSTR();

BSTR *operator&(_bstr_t &b) {
    b.Assign(nullptr);
    return &b.GetBSTR();
}

So, your first example could instead look like the following:

_bstr_t description(L"naughty");
errorInfo->GetDescription(&description);

This evaluation was based on comutil.h from VS 12.0.

bvj
  • 3,294
  • 31
  • 30
4

To follow up on @Hans's answer - the appropriate way to construct the _bstr_t depends on whether GetDescription returns you a BSTR that you own, or one that references memory you don't have to free.

The goal here is to minimize the number of copies, but also avoid any manual calls to SysFreeString on the returned data. I would modify the code as shown to clarify this:

BSTR temp = 0;
HRESULT hr = p->GetDescription(&temp);
if (SUCCEEDED(hr)) {
    _bstr_t wrap(temp, false);    // do not copy returned BSTR, which
                                  // will be freed when wrap goes out of scope.
                                  // Use true if you want a copy.
    // etc..
}
Steve Townsend
  • 53,498
  • 9
  • 91
  • 140
2

_bstr_t (and its ATL sibling CComBSTR) are resource owners of BSTR. Spying from the code it seems that 'GetAddress' is specifically designed for the use case of working with BSTR output parameters where client takes ownership and expected to free the BSTR. MSDN states: 'Frees any existing string and returns the address of a newly allocated string.'.

_bstr_t bstrTemp;
HRESULT hr = p->GetDescription(bstrTemp.GetAddress());

Note that using 'GetAddress()' is not equivalent to using '&GetBSTR()' in case the _bstr_t already owns a BSTR. 'GetBSTR'' only returns the store location without freeing an existing one.

Caveat: this specific use case of 'GetAddress' is not stated in the documentation; it is my deduction from looking at the source code and experience with its ATL counter part CComBSTR.

Since user 'caoanan' questioned this solution, I paste here the source code Microsoft:

inline BSTR* _bstr_t::GetAddress()
{
    Attach(0);
    return &m_Data->GetWString();
}

inline wchar_t*& _bstr_t::Data_t::GetWString() throw()
{
    return m_wstr;
}

inline void _bstr_t::Attach(BSTR s)
{
    _Free();

    m_Data = new Data_t(s, FALSE);
    if (m_Data == NULL) {
        _com_issue_error(E_OUTOFMEMORY);
    }
}

inline _bstr_t::Data_t::Data_t(BSTR bstr, bool fCopy)
    : m_str(NULL), m_RefCount(1)
{
    if (fCopy && bstr != NULL) {
        m_wstr = ::SysAllocStringByteLen(reinterpret_cast<char*>(bstr),
                                         ::SysStringByteLen(bstr));

        if (m_wstr == NULL) {
            _com_issue_error(E_OUTOFMEMORY);
        }
    }
    else {
        m_wstr = bstr;
    }
}
gast128
  • 1,223
  • 12
  • 22
  • @caoanan: could you please post an example? – gast128 Nov 24 '21 at 18:48
  • see the answer from @steve-townsend https://stackoverflow.com/a/4347754/3353857, should use GetBSTR(), not GetAddress() – caoanan Nov 29 '21 at 11:40
  • No because using &GetBSTR does lead to a memory leak. It returns the wrapped m_wstr member and this gets overwritten by the newly BSTR (without releasing the old one). In contrast GetAddress first frees the BSTR before returning the address. – gast128 Nov 29 '21 at 12:33
  • can't agree. see MSDN's example of _bstr_t::Assign, https://learn.microsoft.com/en-us/cpp/cpp/bstr-t-assign – caoanan Dec 01 '21 at 04:17
  • @caoanan: I know that example from MSDN. To me it doesn't counter argument my and Microsoft's statement that it releases the embedded BSTR before returning the address. I have pasted Microsoft's code so you can have a look at yourself. Please point the location in the source where I am wrong and I will retract this contribution. – gast128 Dec 01 '21 at 07:54
  • I like your latest edit of the answer. "Using 'GetAddress()' is not equivalent to using '&GetBSTR()' in case the _bstr_t already owns a BSTR." – caoanan Jan 14 '22 at 03:00
  • OP's question was all about virgin _bstr_t, you guys make it grow. To avoid confusion, it's better to clarify the "casese", answer OP's question first, then extend the discussion. – caoanan Jan 14 '22 at 03:09
-1

Thanks to the answers by bvj and gast128, it is clear that you can pass an empty bstr_t via GetAddress() to a function with the signature GetDescription(BSTR* BS), which will create a new BSTR, but you must never pass a bstr_t containing a value in this way to a function intended to use the wrapped BSTR, since GetAddress() will free the wrapped BSTR. I present a complete example.

#include <iostream>
#include <comdef.h>

void CreateBSTR(BSTR* bstr)
{
  *bstr = SysAllocString(L"Read the manual!");
}

void UseBSTR(BSTR* bstr)
{
  std::wcout << L"Content: " << *bstr << std::endl;
}

int main()
{
  bstr_t bs_t;
  CreateBSTR(bs_t.GetAddress());
  std::wcout << L"Description: " << bs_t << std::endl;

  BSTR BS = bs_t.Detach();
  UseBSTR(&BS);
  bs_t.Attach(BS);
  //Don't: UseBSTR(bs_t.GetAddress());
}
-1
    int GetDataStr(_bstr_t & str) override {
    BSTR data = str.Detach();
    int res = m_connection->GetDataStr( &data );
    str.Attach(data);
    return res;
}