STL Memory Problem

[kilgariff]

Hi, I get a segmentation fault when I try to delete a library. Could someone please have a look over this code segment and tell me if there's something obviously wrong?

There's a syntax-highlighted version with other functions that use the std::list here : http://pastebin.com/m4bf43d62

void Application::freeComponent( char* componentID ){

	Library* deadLibrary = NULL;
	//Iterate through libraries and delete the component that matches the argument
	for( std::list<Library*>::iterator i = Libraries.begin(); i != Libraries.end(); ++i )
		if( strcmp( (*i)->ComponentFactory->getComponentID(), componentID ) == 0 ){
			log << "Freeing Component: " << componentID << "...";
			
			if( (*i)->ComponentFactory->isActive ){
				log << "[ERROR] Cannot free " << componentID << ": Component in-use." << write;
				return;
			} else {
				deadLibrary = *i;
			}	
		}
	
	if( deadLibrary ){
	
		Libraries.remove( deadLibrary );
		delete deadLibrary;
		log << "Done." << write;
		return;
	}
	
	log << "Cannot free " << componentID << ":  Component isn't loaded." << write;
	return;
}

[Paul McKenzie]

Hi, I get a segmentation fault when I try to delete a library. Could someone please have a look over this code segment and tell me if there's something obviously wrong?

		if( strcmp( (*i)->ComponentFactory->getComponentID(),

How are you storing the ID? Is it a std::string or a char*? If it is a char*, then change it to std::string (and all indication shows that it is a char*). There is no need for char * routines and types if your goal is to store an entire string. Moreover, storing char* in a container (or a class) to represent a string is wrought with all sorts of potential problems.

I make it a rule to never use char* as a string in C++ if your goal is to store a string. The reasons are that it is very easy to blow up using a char* container:

#include <vector>
#include <cstring>
#include <iostream>

int main()
{
    std::vector<char *> V;
    char p[] = "This is a test";
    V.push_back(p);
    strcpy( p, "Another test");
    V.push_back(p);
    for (int i = 0; i < V.size(); ++i )
       std::cout << V[i] << "\n";
}

Output:

Another test
Another test

Note that "This is a test" no longer is in the container, since it stored pointers and not entire strings. The fix is to use a vector<string> and not a vector<char *>. If your container or whatever uses char*, then problems such as I've illustrated can cause applications to do who-knows-what.

Here is another issue:

	if( deadLibrary ){
	
		Libraries.remove( deadLibrary );

How do we know that "deadLibrary" was actually allocated?

Basically, a segmentation fault is a runtime problem. Without code that we can actually run, we can't tell you what is wrong with the code. We don't know where, when or how you're calling that function. We don't know what state your program is in at the time you get the segmentation fault, we don't know the values of any of those variables, etc.

It is very rare that anyone can determine a segmentation fault in a piece of code using static analysis of that code. Therefore, if you can post something that is complete and can run, then you will get a better answer.

Regards,

Paul McKenzie