4

I have a c++ code. But it is not releasing memory properly. Tell me where I am wrong, here is my code

1 void MyClass::MyFunction(void)
2 {
3    for (int i=0; i<count; i++)
4    {
5        _bstr_t xml = GetXML(i);
6        // some work
7        SysFreeString(xml);
8    }
9 }

GetXML (line 5) returns me a BSTR. At this memory of program increases. But after SysFreeString (line 7) memory does not release. What I am doing wrong here?

fhnaseer
  • 7,159
  • 16
  • 60
  • 112
  • How are you measuring memory? – mmmmmm Oct 22 '12 at 12:11
  • How can you say that the leak is not in GetXML (ie. not related to the BSTR allocation)? – user18428 Oct 22 '12 at 12:12
  • 1
    Read more on _bstr_t: http://msdn.microsoft.com/en-us/library/zthfhkd6%28v=vs.80%29.aspx, "The class manages resource allocation and deallocation...", so you should not have to call SysFreeString. – marcinj Oct 22 '12 at 12:14
  • @Mark by task manager, i am debugging my application, and whenever line 5 executes memory of application increases, – fhnaseer Oct 22 '12 at 12:17
  • 1
    @FaisalHafeez - this tells you nothing as when releasing memory in C this is not necessarily returned to the Operating System - Do more allocations and deallocs and see if the overal size does increas or use memory debuggers to show leaks like the calls to _CrtDebug in VS. – mmmmmm Oct 22 '12 at 12:20
  • @mark i havent done that before, can you give me walkthrough link of it, – fhnaseer Oct 22 '12 at 12:23
  • @FaisalHafeez read here http://msdn.microsoft.com/en-us/library/e5ewb1h3%28v=VS.80%29.aspx – marcinj Oct 22 '12 at 12:28
  • C++ processes allocate OS memory as they run, but usually deallocate upon exit only. The deallocated memory is not released from the OS, but managed as a heap by the runtime. So you won´t see the process memory consumption shrink as you deallocate memory, at least not 1:1. – TheBlastOne Oct 22 '12 at 13:14

4 Answers4

8

First: 

// This makes a copy.
// This is where the leak is. You are leaking the original string.
_bstr_t xml = GetXML();

// You want to use this, to attach the BSTR to the _bstr_t
_bstr_t xml = _bstr_t(GetXML(), false);

Second, don't do this:

SysFreeString(xml);

The _bstr_t class will do that for you.

Third, BSTR will not release the memory to the OS immediately, it caches recently used strings in order to make SysAllocString faster. You shouldn't expect to see memory usage go straight down after SysFreeString.

You can control this behaviour for debugging purposes:

Lastly, when viewing memory usage in Task Manager you need to look at the column "Commit Size" not "Working Set". Go to Menu->View->Select Columns to show the column. And also note that this really only helps over a period of time - the memory may not be released to the OS immediately, but if you have no leaks, it shouln't go up forever, over a course of hours.

Ben
  • 34,935
  • 6
  • 74
  • 113
  • Can u please elaborate what really is the difference between the two since destructor should be called in both cases? Does this apply to `_variant_t` too? I have code: `_variant_t vtVal; vtVal.bstrVal = strValue.AllocSysString();` Will this string get freed by `_variant_` destructor? – zar Oct 13 '17 at 17:25
  • 1
    @zar, the first `_bstr_t` constructor makes a copy of the `BSTR` argument (obtained from `GetXML()`). The copy *is* cleaned up by the destructor. The *original* `BSTR` is the one that leaks. `_variant_t` is another kettle of fish, but the short answer is you need to set the `vartype` as well. – Ben Oct 14 '17 at 15:02
1

I suppose you should use :

xml.Attach(GetXML(i));

operator= looks like it is actually assigning new value - which means copying it. That value returned by GetXML stays unfreed.

also there should be no need for SysFreeString(xml);

marcinj
  • 48,511
  • 9
  • 79
  • 100
  • i read somewhere that if i use _bstr_t xml(GetXML(i), false) then there will be no memory leak, but that also doesn't worked fine for me, i try this and let you know that it helped or not. – fhnaseer Oct 22 '12 at 12:20
  • Actually this should also work, from docs for fCopy: "If false, the bstr argument is attached to the new object without making a copy by calling SysAllocString.", so this is exactly the same what Attach does. – marcinj Oct 22 '12 at 12:22
0

Task Manager only provides the amount of memory allocated to the process. When C++ releases memory ( C's free) it does not necessarily return the memory to the Operating system so Task Manager will not necessarily show memory going doem until the process ends.

What Task Manager can show is if you keep allocating memory and not releasing it then the memory size of the process will keep increasing, if this happens you probably hava a memory leak.

When programming you need to use memory profilers to see if you are releasing memory. In Windows I used Rational's Purify to give me this information but it costs a lot. MS C runtime can be used to track memory. MSDN provides an overview here, read and follow th links.

As to your code and as per other comments and answers one of the points of using _bstr_t class is to do the memory and other resource management for you so you should not call SysFreeString

mmmmmm
  • 32,227
  • 27
  • 88
  • 117
0

The destructor of _bstr_t will call SysFreeString(xml), so you don't need to call SysFreeString(xml) again. An additional memory free will result in crash.

rong liu
  • 43
  • 3