Freeing a BSTR using ::SysFreeString(). More Platform Dependant?

Question

I am writing a COM Server which have a plenty of Interfaces and methods. And most of the methods have the BSTR as the parameters and as local parameters used for the return. A snippet looks like

Update 5:

The real code. This fetches from bunch of Data based on a specific condition the DB to populate an array of Object.

STDMETHODIMP CApplication::GetAllAddressByName(BSTR bstrParamName, VARIANT *vAdddresses)
{
    AFX_MANAGE_STATE(AfxGetStaticModuleState())

//check the Database server connection

COleSafeArray saAddress;
HRESULT hr;

// Prepare the SQL Strings dan Query the DB

long lRecCount = table.GetRecordCount();

 if (lRecCount > 0)
 {
    //create one dimension safe array for putting  details
    saAddress.CreateOneDim(VT_DISPATCH,lRecCount);

    IAddress *pIAddress = NULL; 
    //retrieve details 
    for(long iRet = table.MoveFirst(),iCount=0; !iRet; iRet = table.MoveNext(),iCount++)
    {
        CComObject<CAddress> *pAddress;
        hr = CComObject<CAddress>::CreateInstance(&pAddress);
        if (SUCCEEDED(hr))
        {   
            BSTR bstrStreet = ::SysAllocString(table.m_pRecordData->Street);
            pAddress->put_StreetName(bstrStreet);

            BSTR bstrCity = ::SysAllocString(table.m_pRecordData->City);
            pAddress->put_CityName(bstrCity);
        }
        hr = pAddress->QueryInterface(IID_IAddress, (void**)&pIAddress);
        if(SUCCEEDED(hr)) 
        {
            saAddress.PutElement(&iCount,pIAddress); 
        }
    }
    *vAdddresses=saAddress.Detach(); 
}
table.Close(); 
return S_OK;
}


STDMETHODIMP CAddress::put_CityName(BSTR bstrCityName)
{
    AFX_MANAGE_STATE(AfxGetStaticModuleState())
    // m_sCityName is of CComBSTR Type
    m_sCityName.Empty();//free the old string 
    m_sCityName = ::SysAllocString(bstrCityName);//create the memory for the new string
    return S_OK;
}

The problem lies in the Memory Freeing part. The code works very fine in any Win XP machines, but when comes to WIN2K8 R2 and WIN7 the code crashes and pointing to the ::SysFreeString() as the culprit. The MSDN is not adequate to the solution.

Can anyone please help in finding the right solution?

Thanks a lot in advance :)

Update 1:

I have tried using the CComBSTR as per the suggestion in the place of raw BSTR, initialized using direct CString's and excluded the SysFreeString(). But for my trouble, on getting out of scope the system is calling the SysFreeString() which again causes the crash :(

Update 2: With the same CComBSTR i tried to allocate using the SysAllocString() , the problem remains same :(

Update 3: I am tired of all the options and in peace I am having only question in mind

Is it necessary to free the BSTR through SysFreeString() which was allocated using SysAllocString()/string.AllocSysString()?

Update 4: I missed to provide the information about the crash. When I tried to debug the COM server crashed with a error saying

"Possible Heap Corruption"

. Please help me out of here.. :(

IMO, the allocator should be the freer. That is, the client calling FooMethod should do the allocation of bstrName. And it should free it as well. Plus, your code is confusing, you have bstrname, and bstrName. Maybe obj.Name = bstrName; should really be obj.Name = bstrname; ? — Todd Murray, Feb 27 '12 at 14:54
Shouldn't the code be similar to `obj.Name = ::SysAllocString(someString);`. Also, consider using `CComBSTR` or `_bstr_t` instead of raw BSTRs, would save you a lot of headaches. — Bhargav, Feb 27 '12 at 15:14
Yes it is necessary to release the allocated memory block. Please, provide us the full code which uses the 'obj' variable. Otherwise, it is impossible to help you to find the problem — Igor Chornous, Feb 29 '12 at 19:51
@jmaniac, I have updated my answer in order to cover your updates — Igor Chornous, Mar 01 '12 at 06:47

Igor Chornous · Answer 1 · 2012-03-01T08:19:00.797

// Now All Things are packed in to the Object
obj.Name = bstrName;
obj.Name2 = bstrname2;

I don't quite understand what do you mean by saying that things are packed since you're just copying pointers to the strings, and at the moment when you call SysFreeString obj.Name and obj.Name2 will point to an invalid block of memory. Although this code is not safe, it looks like if the source of your problem is class CFoo. You should show us more details of your code

I suggest you to use a CComBSTR class which will take a responsibility for releasing the memory.

UPDATE

#include <atlbase.h>
using namespace ATL;
...
{
    CComBSTR bstrname(_T("Some Name")); 
    CComBSTR bstrname2(_T("Another Name"));
    // Here one may work with these variables if needed
    ...
    // Copy the local values to the Obj's member Variable 
    bstrname.Copy(&obj.Name); 
    bstrname2.Copy(&obj.Name2);
}

UPDATE2 First of all one should free bstrCity and bstrStreetName with SysFreeString or use CComBSTR instead within this block:

if (SUCCEEDED(hr))
{   
    BSTR bstrStreet = ::SysAllocString(table.m_pRecordData->Street);
    pAddress->put_StreetName(bstrStreet);

    BSTR bstrCity = ::SysAllocString(table.m_pRecordData->City);
    pAddress->put_CityName(bstrCity);

    // SysFreeString(bstrStreet)
    // SysFreeString(bstrCity)
}

Consider to amplify the loop's condition !iRet with iCount < lRecCount.

for(...; !iRet /* && (iCount < lRecCount) */; ...)

Also, here:

m_sCityName = ::SysAllocString(bstrCityName);

you allocate memory but never release it since internally CComBSTR& operator = (OLESTR ..) allocates a new storage itself. One should rewrite is as follows:

m_sCityName = bstrCityName;

Everything else, looks good for me

UPDATE3 Well, Heap corruption is often a consequence of writing some values outside of the allocated memory block. Say you allocate an array of length 5 and put some value to the 6th position

Thanks Kids-fox for the Answer. As you mentioned the packaging is filling the Class' member variable as that is going to be returned. For my puzzle, this didn't crashed even once in the XP machines, only on the modern OS's. — Sathish Guru V, Feb 28 '12 at 04:07
It may not crash only due to a coincidence. There is a general rule of thumb in COM: a function should allocate the memory for its returning values and for all the output values. Thus FooMethod should not release the memory. Look at the snippet which I've added to my answer — Igor Chornous, Feb 28 '12 at 07:01
You are returning pointers to freed memory. The behavior is undefined. It is not surprising that undefined behavior varies from system to system. — Raymond Chen, Feb 29 '12 at 18:12
@RaymondChen, I bet you wanted to answer jmaniac. Cause I'm stating the very same idea — Igor Chornous, Feb 29 '12 at 19:25

score 2 · Accepted Answer · answered Jul 26 '13 at 12:22

Finally I have found the real reason for the Heap Corruption that happened in the code.

The put_StreetName/put_CityName of the IAddress/CAddress is designed in a following way.

STDMETHODIMP CAddress::put_CityName(BSTR bstrCityName)
{
    AFX_MANAGE_STATE(AfxGetStaticModuleState())

    m_sCityName.Empty();
    TrimBSTR(bstrCityName);
    m_sCityName = ::SysAllocString(bstrCityName);

    return S_OK;
}

BSTR CAddress::TrimBSTR(BSTR bstrString)
{
    CString sTmpStr(bstrString);
    sTmpStr.TrimLeft();
    sTmpStr.TrimRight();
    SysReAllocString(&bstrString,sTmpStr);  // The Devilish Line
}

The Devilish Line of code is the real culprit that caused the Memory to go hell.

What caused the trouble?

In this line of code, the BSTR string passed as a parameter is from another application and the real memory is in another realm. So the system is trying to reallocate teh string. Either succeed or not, the same is tried to cleared off from the memory in original application/realm, thus causing a crash.

What still unsolved?

Why the same piece of code doesn't crashed a single time in Win XP and older systems? :(

Thanks for all who took their time to answer and solve my problem :)

Your analysis is off by about a mile. `BSTR`s can be marshaled across apartment boundaries by the system without any problem. Your issue is that `SysReAllocString` updates a **temporary** (`&bstrString`). When `TrimBSTR` returns `bstrCityName` points to a string that potentially is no more. To fix your bug either change the signature of `TrimBSTR` to take a `BSTR*` or return the updated value of `bstrString` from `TrimBSTR` (how did that even compile?). — IInspectable, Apr 22 '14 at 09:19

Freeing a BSTR using ::SysFreeString(). More Platform Dependant?

2 Answers2

Linked