locked
Possible BSTR memory leak RRS feed

  • Question

  • Hi,

    I am trying to track a memory leak which I suspect may be caused by memory allocated for CComBSTR objects not getting released.  I'll first give a broad description of the scenario.  I have a data acquisition program written in C# which makes calls to an interop COM dll to retrieve data from the outside world (data points from a programmable logic controller).  The COM dll itself is written in C++ and actually talks COM to the outside world (a Kepware OPC IO server).  In C++, once the COM dll has retrieved data from the outside world it sets up an array of BSTR's (origin of these BSTR's are CComBSTR's created on the stack) by which it conveys the data back to the client caller using IDispatch::Invoke.  The snippet of code at the bottom of my post shows how the BSTR array is set up and returned to the caller.  Note that the call to CComBSTR::Detach() sets to null the underlying string pointer so that when the CComBSTR descructor fires the underlying call to SysFreeStrinf() will not release any memory since it is passed a null pointer.  From what I have read, this is OK since in such cases it is the responsibility of the COM marshaller to release this memory (is this correct ???).  Note that we have used this COM dll on our previous platform which was written entirely in VC++, including the client which called this COM dll.  In that platform the memory leak did not show up.  Only on the new platform written in C# does it show up.

    The reason I suspect this to be the source of the memory leak is that after running performance monitor on the program to gather some memory counters which narrowed my search to the C# client's call to the COM dll and the COM dll's callback processed back in C#, I ran DebugDiag 1.1 to monitor for memory leaks and produce a core dump and then used the Advanced analysis / Memory analysis feature to analyse the dump.  The report it produced showed the COM dll as having the most outstanding memory allocations and specifically the call to SysAllocStringLen which is in the CComBSTR constructor.

    At first glance I would suspect that the COM marshaller is not releasing memory when the client caller is managed code, whereas it does when the client caller is native code.  Could this be the case?  It seems to me that given the food-chain I described above by which BSTR data is passed from the COM dll to the caller, it would be improper to release the BSTR memory in the COM dll since the associated BSTR pointers are passed to IDispatch::Invoke.  This would mean that the releasing of the memory should either occur at the marshaller level or the client level, and that the COM dll code (shown below) is OK as is.

    In pursuit of the above I also saw reverences to "turning off BSTR caching" (environment variable OANOCACHE = 1 to turn it off).  I assume that by default this caching is on.  Could this be the cause of leak in the managed code client scenario described here ? Would turning it off be proper to do?

    Any help and guidance in solving this leak is greately appreciated.  Thanks in advance.

    // code snippet

    DISPPARAMS dispparams;
    DISPID dispid;
    VARIANT VarResult = {0};

    VARIANT arg[4];

    CComBSTR value = szValue;  // szValue is LPCSTR
    arg[0].vt = VT_BSTR;
    arg[0].bstrVal = value.Detach();

    CComBSTR itemName= szItemName;  // szItemName is LPCSTR
    arg[1].vt = VT_BSTR;
    arg[1].bstrVal = itemName.Detach();

    CComBSTR groupName = szGroupName; // szGroupName is LPCSTR
    arg[2].vt = VT_BSTR;
    arg[2].bstrVal = groupName.Detach();

    CComBSTR serverName = szServerName; // szServerName is LPCSTR
    arg[3].vt = VT_BSTR;
    arg[3].bstrVal = serverName.Detach();

    dispparams.rgvarg = arg;
    dispparams.cArgs = 4;
    dispparams.cNamedArgs = 0;

    LPOLESTR szMember = OLESTR("AdviseCallback");
    hr = pOPCCallBack->GetIDsOfNames(IID_NULL, &szMember, 1, LOCALE_USER_DEFAULT,&dispid);

    if (SUCCEEDED(hr))
    {
     hr = pOPCCallBack->Invoke(dispid, IID_NULL,LOCALE_USER_DEFAULT, DISPATCH_METHOD, &dispparams, &VarResult,
      NULL, NULL);

     if (SUCCEEDED(hr))
     {
    ... and so on

    Tuesday, July 5, 2011 6:04 PM

Answers

  • Hello aram1958,

     

    1. Memory Ownership.

    1.1 >> Note that the call to CComBSTR::Detach() sets to null the underlying string pointer so that when the CComBSTR descructor fires the underlying call to SysFreeStrinf() will not release any memory since it is passed a null pointer. From what I have read, this is OK since in such cases it is the responsibility of the COM marshaller to release this memory (is this correct ???). 

    This is not correct, aram1958. It is not necessarily the responsibility of the COM marshaler to release this memory. It all depends on how the AdviseCallback() method has been declared.

    From the code snippet, the AdviseCallback() method is likely declared as somthing like :

    [id(1), helpstring("method AdviseCallback")] HRESULT AdviseCallback([in] BSTR ServerName, [in] BSTR GroupName, [in] BSTR ItemName, [in] BSTR Value);

    Notice the [in] IDL attributes applied to the parameters. If this declaration is indeed so, then the implementation of the method (the callee) must treat the incoming BSTRs as read-only. It certainly cannot free the BSTRs (via SysFreeString()).

    1.2 The onus is on the caller of the method to allocate and then free the BSTRs. It is the caller that owns the memory of the BSTRs. You can correct the code by calling VariantClear() on each of the elements of the VARIANT array "arg", after the call to Invoke(), e.g. :

     

     LPOLESTR szMember = OLESTR("AdviseCallback");
     hr = m_pIOPCCallBack->GetIDsOfNames(IID_NULL, &szMember, 1, LOCALE_USER_DEFAULT,&dispid);

     if (SUCCEEDED(hr))
     {
      hr = m_pIOPCCallBack->Invoke(dispid, IID_NULL,LOCALE_USER_DEFAULT, DISPATCH_METHOD, &dispparams, &VarResult, NULL, NULL);

      if (SUCCEEDED(hr))
      {
       //... and so on
      }
     }
     
     // Free the BSTRs contained inside the Variants.
     for (int i = 0; i < 4; i++)
     {
       VariantClear(&(arg[i]));
     }

     

    1.3 >> It seems to me that given the food-chain I described above by which BSTR data is passed from the COM dll to the caller, it would be improper to release the BSTR memory in the COM dll since the associated BSTR pointers are passed to IDispatch::Invoke. This would mean that the releasing of the memory should either occur at the marshaller level or the client level, and that the COM dll code (shown below) is OK as is.

    Negative, aram1958. This is simply not the case. The direction of the parameters (i.e. the [in], [out] or [in, out] IDL attributes) determine this. In the case of AdviseCallback(), my strong guess (albeit I could be wrong) is that the 4 BSTR params are all [in] (Please advise this). Hence the BSTRs are to be allocated and freed by the caller and the callee must only read them. Please also read point 2.2 below.

    For [out] parameters, it is the callee that allocates the memory to be returned to the caller. Once the caller receives this returned memory, it owns it and it is the caller that must free this memory.

    For [in, out] parameters, it is the responsibility of the caller to allocate memory. However, the callee may choose to free it and then re-allocate new memory. The side that is eventually responsible for freeing the memory is still the caller.

     

    2. Concerning the Memory Leakage.

    2.1 >>  In that platform the memory leak did not show up. Only on the new platform written in C# does it show up...

    >> At first glance I would suspect that the COM marshaller is not releasing memory when the client caller is managed code, whereas it does when the client caller is native code. Could this be the case?

    If the declaration of AdviseCallback() is indeed as listed in point 1.1 above, then with due respect, aram1958, I doubt this very much. There is most likely (undetected) memory leakage. The code listed in the OP is definitely leaking BSTRs since it is the caller's responsibility to free the BSTRs (and this is not happening).

    2.2 Should the call to AdviseCallback() involve marshaling (e.g. across apartments/across managed and unmanaged code), then intermediate BSTRs may be created by the COM marshaler and/or the CLR's Interop Marshaler. In these circumstances, the COM marshaler and/or the Interop Marshaler will free the intermediate BSTRs. 

    2.3 However, the original BSTRs created in the calling code must be freed by the calling code.

    2.4 There is only one very special situation in which memory is not leaked : the call to AdviseCallback() is done within the same apartment (i.e. no cross-apartment marshaling gets involved) and the code for AdviseCallback() (incorrectly) frees the BSTRs. This can only happen in unmanaged code.

    2.5 This situation will work but it is not safe because, with the same code, if cross-apartment marshaling gets involved, then AdviseCallback() would have deleted the BSTRs that are meant to be deleted by the COM marshaler. A crash can follow.

     

    3. Concerning BSTR Caching.

    3.1 BSTR caching is on by default. It is an optimization of the COM subsystem. It is generally a good feature in my opinion.

    3.2 However, turning it off will not solve your problem above if there is indeed leakage due to bugs.

    3.3 However, it can be useful for development and testing purposes for checking that all relevant BSTRs are indeed freed.

     

     

    - Bio.

     


    Please visit my blog : http://limbioliong.wordpress.com/

    • Edited by Lim Bio Liong Wednesday, July 6, 2011 10:46 PM
    • Proposed as answer by Aspen VJ Wednesday, July 13, 2011 6:18 AM
    • Marked as answer by Aspen VJ Thursday, July 14, 2011 2:28 AM
    Wednesday, July 6, 2011 4:41 PM

All replies

  • Probably is good to let people here know what is the correlation between BSTR and C# here.

    chanmm


    chanmm
    Wednesday, July 6, 2011 3:40 AM
  • When I first submitted the question, I had a dilemma about whether to submit it under a VC++/ATL forum or C#.  The reason I chose C#is that the program that calls the Interop COM dll is a program (rus as a service to be specific) written in C#.  By the way everything is on a .NET 3.5 platform.  Just to reiterate the ineraction between the two parties, the C# program calls a method in the dll (asynchronous semantics) requesting the dll to retreive up-to-date data from the outside world (as described in original question) and the dll, once it has retrieved the data, makes a callback to the C# program using IDispatch::Invoke(), to pass the newly retrieved data to the calling program.

    Maybe I should post the question on the VC++/ATL forum as well.

    Thanks,

    aram1958

    Wednesday, July 6, 2011 2:25 PM
  • Hello aram1958,

     

    1. Memory Ownership.

    1.1 >> Note that the call to CComBSTR::Detach() sets to null the underlying string pointer so that when the CComBSTR descructor fires the underlying call to SysFreeStrinf() will not release any memory since it is passed a null pointer. From what I have read, this is OK since in such cases it is the responsibility of the COM marshaller to release this memory (is this correct ???). 

    This is not correct, aram1958. It is not necessarily the responsibility of the COM marshaler to release this memory. It all depends on how the AdviseCallback() method has been declared.

    From the code snippet, the AdviseCallback() method is likely declared as somthing like :

    [id(1), helpstring("method AdviseCallback")] HRESULT AdviseCallback([in] BSTR ServerName, [in] BSTR GroupName, [in] BSTR ItemName, [in] BSTR Value);

    Notice the [in] IDL attributes applied to the parameters. If this declaration is indeed so, then the implementation of the method (the callee) must treat the incoming BSTRs as read-only. It certainly cannot free the BSTRs (via SysFreeString()).

    1.2 The onus is on the caller of the method to allocate and then free the BSTRs. It is the caller that owns the memory of the BSTRs. You can correct the code by calling VariantClear() on each of the elements of the VARIANT array "arg", after the call to Invoke(), e.g. :

     

     LPOLESTR szMember = OLESTR("AdviseCallback");
     hr = m_pIOPCCallBack->GetIDsOfNames(IID_NULL, &szMember, 1, LOCALE_USER_DEFAULT,&dispid);

     if (SUCCEEDED(hr))
     {
      hr = m_pIOPCCallBack->Invoke(dispid, IID_NULL,LOCALE_USER_DEFAULT, DISPATCH_METHOD, &dispparams, &VarResult, NULL, NULL);

      if (SUCCEEDED(hr))
      {
       //... and so on
      }
     }
     
     // Free the BSTRs contained inside the Variants.
     for (int i = 0; i < 4; i++)
     {
       VariantClear(&(arg[i]));
     }

     

    1.3 >> It seems to me that given the food-chain I described above by which BSTR data is passed from the COM dll to the caller, it would be improper to release the BSTR memory in the COM dll since the associated BSTR pointers are passed to IDispatch::Invoke. This would mean that the releasing of the memory should either occur at the marshaller level or the client level, and that the COM dll code (shown below) is OK as is.

    Negative, aram1958. This is simply not the case. The direction of the parameters (i.e. the [in], [out] or [in, out] IDL attributes) determine this. In the case of AdviseCallback(), my strong guess (albeit I could be wrong) is that the 4 BSTR params are all [in] (Please advise this). Hence the BSTRs are to be allocated and freed by the caller and the callee must only read them. Please also read point 2.2 below.

    For [out] parameters, it is the callee that allocates the memory to be returned to the caller. Once the caller receives this returned memory, it owns it and it is the caller that must free this memory.

    For [in, out] parameters, it is the responsibility of the caller to allocate memory. However, the callee may choose to free it and then re-allocate new memory. The side that is eventually responsible for freeing the memory is still the caller.

     

    2. Concerning the Memory Leakage.

    2.1 >>  In that platform the memory leak did not show up. Only on the new platform written in C# does it show up...

    >> At first glance I would suspect that the COM marshaller is not releasing memory when the client caller is managed code, whereas it does when the client caller is native code. Could this be the case?

    If the declaration of AdviseCallback() is indeed as listed in point 1.1 above, then with due respect, aram1958, I doubt this very much. There is most likely (undetected) memory leakage. The code listed in the OP is definitely leaking BSTRs since it is the caller's responsibility to free the BSTRs (and this is not happening).

    2.2 Should the call to AdviseCallback() involve marshaling (e.g. across apartments/across managed and unmanaged code), then intermediate BSTRs may be created by the COM marshaler and/or the CLR's Interop Marshaler. In these circumstances, the COM marshaler and/or the Interop Marshaler will free the intermediate BSTRs. 

    2.3 However, the original BSTRs created in the calling code must be freed by the calling code.

    2.4 There is only one very special situation in which memory is not leaked : the call to AdviseCallback() is done within the same apartment (i.e. no cross-apartment marshaling gets involved) and the code for AdviseCallback() (incorrectly) frees the BSTRs. This can only happen in unmanaged code.

    2.5 This situation will work but it is not safe because, with the same code, if cross-apartment marshaling gets involved, then AdviseCallback() would have deleted the BSTRs that are meant to be deleted by the COM marshaler. A crash can follow.

     

    3. Concerning BSTR Caching.

    3.1 BSTR caching is on by default. It is an optimization of the COM subsystem. It is generally a good feature in my opinion.

    3.2 However, turning it off will not solve your problem above if there is indeed leakage due to bugs.

    3.3 However, it can be useful for development and testing purposes for checking that all relevant BSTRs are indeed freed.

     

     

    - Bio.

     


    Please visit my blog : http://limbioliong.wordpress.com/

    • Edited by Lim Bio Liong Wednesday, July 6, 2011 10:46 PM
    • Proposed as answer by Aspen VJ Wednesday, July 13, 2011 6:18 AM
    • Marked as answer by Aspen VJ Thursday, July 14, 2011 2:28 AM
    Wednesday, July 6, 2011 4:41 PM
  • Hi Bio,

    Thank you for your very detailed and helpful response.  Some of the details in my original post may not be very clear but your hunches about them appear correct.  Specifically, the AdviseCallback() involves marshalling from unmanaged code (C++/ATL) to managed code (C#).  Since the callee is C#, the AdviseCallback() method does not appear to be defined in an IDL file, but rather is defined as a C# interface  - see code snippet below.  However, as you can see below, the parameters to AdviseCallback() in the interface definition are specified as plainly string, meaning they are read-only string parameters from the callee's perspective, which then is similar the IDL method specification that you assumed.  This being the case, your recommendation to clear the VARIANT array after the call to Invoke() makes sense.

    I will try this out now and advise you on how it works out.

    Thanks again,

    aram1958

    // code snippet

    [Guid("c982edbe-d37e-4b64-9c5d-d28cd632a106"), InterfaceType(ComInterfaceType.InterfaceIsIDispatch)]
    //    [ComVisible(true)]
    public interface IOPCCallback
    {
        [DispId(1)]
        void AdviseCallback(string serverName, string channelName, string pointAddress, string aValue);

        [DispId(2)]
        void ReportError(string errorCode, string serverName, string channelName, string pointAddress);
    }

    [Guid("a69efa4d-6f14-4b16-9445-3c9f1b0ae7c4")]
    [ComVisible(true), ClassInterface(ClassInterfaceType.None)]
    public class OPCCallback : IOPCCallback
    {
        public OPCCallback()
        {
        }

        public void AdviseCallback(string serverName, string channelName, string pointAddress, string newValue)
        {
    ...rest of implementation

    Wednesday, July 6, 2011 6:29 PM
  • Hi Bio,

    I just wanted to get back to you and let you know that the VariantClear loop which you recommended to free the BSTRs' memory after the call to IDispatch::Invoke() worked.  I have since tested the fix by running the program over a 48-hour period and then simulating some outages/recoveries by disconnecting/reconnecting network cables between the test server and IO equipment.  Throughout I ran performance monitor, logging separate counters to separately track both unmanaged and managed memory consumption.  The unmanaged consumption has remained flat throughout this test and the only fluctuation seen in managed consumption - and this, without a growing trend - appears due to garbage collection.

    Thanks again for your help,

    aram1958

    Monday, July 11, 2011 8:13 PM