2

Please bear with me, i have been a c++ programmer for a little while.

I need to know if i am doing this wrong. It works, but i suspect it causes a memory leak. I have this function:

_bstr_t WCH2BSTR(wchar_t* st)
{
    BSTR stres = SysAllocString(st);
    return (_bstr_t)stres;
}

Let's say i were to use the result like this:

wcout << WCH2BSTR(wCharArr) << " done." << endl;

Will this cause a memory leak, or will the BSTR be deleted by a "garbage collector" like in Java?

If it is a memory leak, how can i prevent it without losing the ability to do it as a one-liner? Sometimes the results of WCH2BSTR are stored in a BSTR variable and disposed of properly, but i would like to use the same function for concatenating wchar_t to BSTR's as well in a one-liner fashion.

Thanks.

Brian
  • 414
  • 4
  • 14
  • there's no automatic garbage collection in C++. – Joseph D. Mar 08 '18 at 06:53
  • `SysFreeString` is a necessary requirement of using `SysAllocString`, otherwise you're right, you do leak memory. – acraig5075 Mar 08 '18 at 07:09
  • Firstly, since this question is windows specific rather than standard C++, so you might want to tag the question for windows or microsoft (and not C++) to increase chances of getting a useful reply. Second, have a look at MSDN, specifically https://learn.microsoft.com/en-us/cpp/atl-mfc-shared/allocating-and-releasing-memory-for-a-bstr – Peter Mar 08 '18 at 07:43
  • @codekaizer: There **is** automatic garbage collection in C++. It differs from Java's or .NET's garbage collection in that it is deterministic. Remember, in C++ *"automatic storage duration"* is called *"automatic"* for a reason. – IInspectable Mar 08 '18 at 16:56

1 Answers1

7

You have a memory leak. But it's subtle:

This line:

BSTR stres = SysAllocString(st);

Allocates a BSTR as you expect.

However, the return statement:

return (_bstr_t)stres;

Triggers a call to the _bstr_t(const wchar_t*) constructor, not the which in turn will allocate another BSTR via SysAllocString. So you've leaked a string from the initial call.

This is likely closer to what you want:

_bstr_t WCH2BSTR(const wchar_t* st)
{
    return _bstr_t(str);
}

The constructor of _bstr_t will do the SysAllocString thing for you. The destructor of _bstr_t will do the SysFreeString thing for you.

But...

Be careful of saying this:

BSTR bstr = WCH2BSTR(L"Foo");

Because that will compile! But after the assignment to a raw BSTR, the destructor of the _bstr_t returned by the helper function will get invoked and free the already returned pointer.

What you really want to do is to just avoid the helper function altogether and say this explicitly in your code:

_bstr_t bstr = L"Foo";

When the _bstr_t goes out of scope, so does the underlying BSTR that it holds.

selbie
  • 100,020
  • 15
  • 103
  • 173
  • 2
    Side note - As a result of writing this up, I think I just discovered why `std::string` doesn't explicitly allow itself to be cast to `const char*`, but instead forces callers to use the `.c_str()` method. It's because bugs like this would erupt in code. – selbie Mar 08 '18 at 07:33
  • 1
    The function body could be `return st;` – M.M Mar 08 '18 at 07:46
  • 1
    Note that `_bstr_t` has an overloaded constructor that accepts an existing `BSTR` and a `bool` to specify whether the `BSTR` is copied or not. So, if you did call `BSTR stres = SysAllocString(st);`, you can use `return _bstr_t(stres, false);` to take ownership of the `BSTR` and avoid a leak. But you are right that `return _bstr_t(st);` is better. – Remy Lebeau Mar 08 '18 at 08:01
  • Well, you would need std::wstring instead of std::string, and yeah, you could use it and call .c_str(). But you could also just use the original wchar_t pointer string directly, with std::wcout. I also think that just using the original wchar_t string directly is a better way to satisfy your statement that "What you really want to do is to just avoid the helper function altogether." See my answer. – Dan Korn Mar 09 '18 at 00:50