Memory Leak Questions

[Hermit]

that is not what this topic is about. We are talking about a student at a very early point in the learning processs...

I don't think it's fair to assume that the OP is still in the beginning stages of his education, so the topic should have little to do with that. If this is a project for an introductory course, I'm inclined to say that the professor is totally out of his mind... in an admirable way.

I agree with your concerns regarding C++ education, but I've seen the argument in a lot of threads and this thread is no better a soap-box than the last. I don't think the chairperson of any offending institution is following our conversation.

Do i need to delete all the SettingsManager::CommandParameter *Pointers that i have on the DBInterface Header, on the Destruction of the DBInterface ???

It depends. The basic principle of memory management is that if you allocate memory, you must know exactly how it gets deallocated - including the case where an exception alters your program execution. If you expect someone else to delete it (a very bad design in most cases), you must clearly say so in the documentation. Conversely, if you are handed a piece of memory by third party code (in which case you are a victim of bad design), you should always consult their documentation.

So, the question I have to ask before I can reasonably give an answer is what allocates the memory in question and what is the expectation at that point as to how it gets deallocated?

There's a postscript to all of this, which is basically what most of the discussion in this thread has been about: You should know how to manage memory, but in practice manual memory management should be avoided at all costs in code that is not specifically designed for the purpose of memory management. A database interface is not a memory management scheme, so do not make it so. Use STL containers and smart pointers for that purpose if at all possible (but as exterminator pointed out, this is not always an option).

As a final note, unless you switch from raw pointers to containers and smart pointers (which you should), you must either implement a public/protected copy constructor and assignment operator, or declare them private and leave them unimplemented.

[dada27]

As you can see I am a true beginner in C++. I downloaded this DLL which is IAS extension. What this code does is:

I receive an authentication request from my Cisco. Into the RadiusExtensionProccess Function. i get a pointer with all the info of the call. There are a few fucntions that goes trhu the pointer an prepare the CommandParameter and send it to my DB, then the code gets the Return from my Stored Procedure and put it in a pointer and send it back to the Cisco. The DLL that i download have an XML file in where i put the parameters of the SP.

So i needed to call different SP's depending of what i received from my Cisco. so i copied the Example code and renamed the pointers. Everything is working fine, but the SVHOST.exe of the IAS. is going up around 500kb an hour. I thought that maybe the problem was that i am not deleting all the

  SettingsManager::CommandParameter   *m_RechargeCardsArray; //Add Recharge Cards

that i rename. But i dont know if i need to delete them. That was my question. Because maybe there is no memory leak, and the SVHOST goes up because i have a lot of a calls going on. i tested with around 25000 calls to the db in one hour.

Can anyone explain me what this code rally does ? g_settings is a class

// Create the thread-local DB Interface object if not done
    DBInterface *dbI = (DBInterface*) TlsGetValue(_tlsDBInterface);
    if (!dbI) {
              dbI = new DBInterface(*g_settings);
	  TlsSetValue(_tlsDBInterface,dbI);
	}

An then do you see in the code that i first sent. if i need to delete something else or if like that is ok ?


Thank you all

[exterminator]

I think you have a more complicated problem or may be no problem at all. But I can't say for sure as I don't have knowledge of the relevant code and may be you haven't posted the relevant code too.

As you can see I am a true beginner in C++. I downloaded this DLL which is IAS extension.

So, this is a downloaded DLL and you have said that you think this DLL has a leak. Well, from the amount of code you have posted till now, I don't see any allocations being done except for DBInterface that contains those pointers. But then inside DBInterface or outside I don't see much happening with those pointers. They are just being initialized to NULL in the constructor and no allocations are being done. The delete[] calls in the destructor hence seem to be deleteing nothing. Although delete call on NULL pointers is okay, but you don't have actually to free anything as the code shown doesn't allocate those arrays. Is there any code that does the allocations? Is it the DLL code that does allocations for those pointers or is it your application code that does it? If the DLL does it, then when it is done with the class' object (that time may vary on different conditions I refer to in my post further), does it free it?

What this code does is:

I receive an authentication request from my Cisco. Into the RadiusExtensionProccess Function. i get a pointer with all the info of the call. There are a few fucntions that goes trhu the pointer an prepare the CommandParameter and send it to my DB, then the code gets the Return from my Stored Procedure and put it in a pointer and send it back to the Cisco. The DLL that i download have an XML file in where i put the parameters of the SP.

Those functions are important. They seem to be doing the allocations for all CommandParameter pointer members in DBInterface. How frequently are they allocated? On every request? Are they ever cleaned up apart from the destructor for each request?

So i needed to call different SP's depending of what i received from my Cisco. so i copied the Example code and renamed the pointers. Everything is working fine, but the SVHOST.exe of the IAS. is going up around 500kb an hour. I thought that maybe the problem was that i am not deleting all the

  SettingsManager::CommandParameter   *m_RechargeCardsArray; //Add Recharge Cards

that i rename. But i dont know if i need to delete them. That was my question. Because maybe there is no memory leak, and the SVHOST goes up because i have a lot of a calls going on. i tested with around 25000 calls to the db in one hour.

May be, may be not. If the DLL is allocating for those CommandParameters upon every request and doesn't free them, then it's buggy. If it does free them (apart from in the destructor), then may you code has the leak outside of the DLL.

Can anyone explain me what this code rally does ? g_settings is a class

// Create the thread-local DB Interface object if not done
    DBInterface *dbI = (DBInterface*) TlsGetValue(_tlsDBInterface);
    if (!dbI) {
              dbI = new DBInterface(*g_settings);
	  TlsSetValue(_tlsDBInterface,dbI);
	}

Well, I ignored this part and probably everyone else did. You have threads involved which makes it more difficult. What this is doing is:

1. You have a TLS index stored in a global or static _tlsDBInterface variable.
2. Each of the thread that does through the above code does allocate a DBInterface object with g_settings as an argument and it does once when the above code is called.
3. Since the above is in a thread say A, then before thread A finishes, it has got to delete the DBInterface object (and any contained dynamically allocated objects), otherwise, repeated new threads being created would be creating that object and not freeing it and resulting in a leak.
4. If the above is being called from the same persistent thread (without the thread being destroyed), then the above object is just allocated once per-thread and after the first time, it used the TLS Index to recapture the object in its own thread local storage and use the object. At most if no more threads are destroyed and create and execute the above call, it would result in just one object being lost/leaked and you shouldn't see memory usage going up by much in due course of time your application keeps running.

So, now the questions are:

1. How frequently are new threads being created?
2. Is the thread executing above code, does ever free DBInterface object created with new before exiting? If not, you have a leak.
3. I am sure CommandParameters are being created everytime a DBInterface object is being used and prepared for save to DB. But does that happen on each request. Are they ever being freed? How frequently? Do the DBInterface object allocation or CommandParameter objects out-living the thread that creates them? Then you have a leak.

An then do you see in the code that i first sent. if i need to delete something else or if like that is ok ?

Since, you say you are using some other DLL. I suppose it is that DLL writer's responsibility to manage resources it creates. His code might be having leaks or is imperfect. In that case, if the author has support, contact them, if not then you will have to fix it on your own. Or use another vendor/another similar DLL that doesn't have such problems (remember I am not saying that this DLL does have a problem because you haven't shown us enough code allocating the resources and their free-ing and thread lifetimes when they are created etc). And yes, to prevent leaks created by dynamic allocations done by 'new' and not being deleted by 'delete' (delete for new and delete[] for new[]), you have to add delete calls to fix it. You will have to carefully understand the object lifetimes, from their construction to till when are they going to be used. They should be freed at the end of their lifetime/usage but not before that.

Overall, this looks non-trivial. You will have to analyse the code much more to get to the actual issue. Threads have made the problem more complex because you the object lifetime may vary depending upon how the threads are being managed and you should have a clear understanding of how each execution path will progress to clearly track the object lifetimes. You may need deallocations at thread exit or if threads are not being created per-request and same one is being used then atleast deallocate per request after use and before overwriting them with newly allocated CommandParameter arrays.

May be there are some tools on windows usable with VS/VC++ (if that's what you are using) that can detect leaks. I haven't see ahoodin's link but if that is helpful - try it out to find if there are leaks and then figure out how to fix them up based on some random hints I gave above.

Hope it helps.

[dada27]

exterminator Thank you ver much for your help.

I think that in here, Freeing DBInterface.

HANDLE hEventSource = NULL;
DWORD  _tlsDBInterface = 0L;
SettingsManager *g_settings = NULL;

BOOL APIENTRY DllMain( HANDLE hModule, 
                       DWORD  ul_reason_for_call, 
                       LPVOID lpReserved
					 )
{
#ifdef DEBUG
  int tmpFlag;
  char *leakBuf;
#endif
  switch (ul_reason_for_call)
  {
  case DLL_PROCESS_ATTACH:
    
#ifdef DEBUG
    tmpFlag = _CrtSetDbgFlag( _CRTDBG_REPORT_FLAG );
    tmpFlag |= _CRTDBG_CHECK_ALWAYS_DF;
    tmpFlag |= _CRTDBG_LEAK_CHECK_DF;
    _CrtSetDbgFlag( tmpFlag );
    leakBuf = (char*) malloc(128);
    strcpy(leakBuf,"Leak check working. Not a leak.");
#endif
    
    hEventSource = RegisterEventSource(NULL,_T("RadiusDB"));
    if (hEventSource == NULL)
      return FALSE;
    
    _tlsDBInterface = TlsAlloc();
    if (_tlsDBInterface == -1)
      return FALSE;
    break;
  case DLL_THREAD_ATTACH:
    break;
  case DLL_PROCESS_DETACH:
    if (hEventSource) DeregisterEventSource(hEventSource);
    {
      DBInterface *dbI = (DBInterface*) TlsGetValue(_tlsDBInterface);
      if (dbI)
	delete dbI;
    }
    if (_tlsDBInterface != -1)
      TlsFree(_tlsDBInterface);
    if (g_settings) delete g_settings;
    break;
  case DLL_THREAD_DETACH:
    {
      DBInterface *dbI = (DBInterface*) TlsGetValue(_tlsDBInterface);
      if (dbI)
	delete dbI;
    }
    break;
  }
  return TRUE;
}

If i am freeing DBInterface at the DLL_THREAD_DETACH, That will delete all of the obejcts that i created inside that class. ?

[exterminator]

The above looks good except that I don't see the DBInterface allocation in DLL_THREAD_ATTACH. (the snippet you posted earlier and asked about what it was doing).

If i am freeing DBInterface at the DLL_THREAD_DETACH, That will delete all of the obejcts that i created inside that class?

Now, that depends on how DBInterface is implemented. It will or it may not. You haven't posted how DBInterface pointers are allocated and whereall they are freed except that for in the destructor. It might be possible that you are doing allocation for them multiple times during the same DBInterface object lifetime (which is from thread entry in the DLL to its exit from there) and don't release those allocations (or may be just one of them during destruction in DLL_THREAD_DETACH) and hence resulting in a leak. If not (not multiple allocations for the same DBInterface object and if you delete all allocated memory for the pointer members in the destructor), then there isn't a leak at this place. Again, assuming your threads are exiting cleanly.

[dada27]

May be i found where the leak is, but i am not sure.

	ADODB::_ConnectionPtr connPool;
DWORD WINAPI RadiusExtensionInit ( VOID ){

	connPool.CreateInstance( __uuidof( ADODB::Connection ) );	
		connPool->CursorLocation = ADODB::adUseClient;
		connPool->CommandTimeout=5;
		connPool->Open("driver={SQL Native Client};server=;Database=tel_data;UID=;PWD=;Connection timeout=5;Connection Reset=TRUE;",empty,empty,ADODB::adConnectUnspecified);
		_variant_t iRecAffected(0L);
		connPool->Execute("SELECT 1",&iRecAffected,ADODB::adExecuteNoRecords );
		connPool->Close();


}

This function is in the same .cpp that the BOOL APIENTRY DllMain that i post before. The radiusExtensionInit gets executed only when i start the service.

I am creating a global scope ADODB::_ConnectionPtr connPool, that i open it on the radiusExtensionInit and then close it, i do this for the connection pooling to work. But i never delete that ConnPool.

I will have a new connPool Object for each new Thread ?
Do i need to delete that pointer? I cant delete right away because If i dont have any instantiated object the connection pool will be destroyed. "Qoute" From MSDN.

However, this means your application needs to keep at least one instance of a Connection object instantiated for each unique user—at all times

Could that be a cause of a leak ?

Thank You

[ahoodin]

I think it is pretty obvious where the leak is.

[dada27]

Ahoodin, Can you tell me please?

Do you think that is on the connPool connection object ? because in debug mode. i did a lot of calls, and the connPool obect was always the same, it had always the same address. So i guess that this object only gets created 1 time.

Thank you

[dada27]

Can you tell me where do you think the leak is ?


thank you

[ahoodin]

Make an attempt to add the stackwalker code to your project, read the readme file and add the 6-8 contiguous lines of code to your project.
If that doesnt work, post your project as it is. No object files (no .obj, .exe, .pch or anything in the debug or release directories). Only .vcproj, .sln,
.cpp, .h files.