why my heap is corrupted?

Question

i'm getting an error - heap corruption, can't figure out why.

my base :

h:

class Base
{
public :
    Base(char* baseName, char* cityName);
    virtual ~Base();

    list<Vehicle*>::const_iterator GetEndList();
    void PrintAllVehicles(ofstream &ResultFile) const;
    char* GetBaseName() const;
    char* GetLocation() const;
    void InsertNewVehicleToBase(Vehicle* newVehicle);
    list<Vehicle*>::const_iterator FindVehicle(char* id);
    void RemoveVehicle (list<Vehicle*>::const_iterator beg);



 private:
    char* m_name;
    char* m_location;
    list<Vehicle*> m_baseVehicles;

};  

cpp :

Base::Base(char* baseName, char* cityName)
{
    m_name = new char [strlen(baseName)+1];
    strcpy(m_name, baseName);
    m_location = new char [strlen(cityName)+1];
    strcpy(m_location, cityName);
}

Base::~Base()
{
    delete [] m_name;
    delete [] m_location;
    //m_baseVehicles.clear();
}

army distructor :

Army::~Army()
{
    list<Base*>::iterator baseIter = m_basesList.begin();
    for (baseIter ; baseIter != m_basesList.end() ; ++baseIter)
        delete (*baseIter);
    m_basesList.clear();
 }  

what am i doing wrong? thanks.

Answer 1

I don't see any problem. Just as Matt Kane said, how does it get populated?

Answer 2

Heap corruptions come from copying more data into an allocated heap cell than the memory allocated to the cell. The beginning and end of heap cells contain data than when overwritten will be reported as a heap corruption.

You have not posted all the code and I cannot see a problem in the code you have posted but I would advise using a memory tool like Valgrind to help you diagnose the problem.

Answer 3

Visible problems with this code:

• use of char* not std::string requires manual memory management
• Use of raw pointers in STL containers overcomplicates cleanup code
• use of CRT for string manipulation is a C++ 'code smell'

Answer 4

There's nothing obviously wrong with the code you've shown, so chances are that the error is in code you haven't shown.

The most immediately suspicious thing to me is that the Base class owns two pointers and does not have a copy constructor or assignment operator defined. This means that should you ever copy a Base object, you will end up with two Base objects pointing to the same data, and when they destruct, they will delete it twice, causing heap corruption.

The Army class may also have this problem as well (since it owns several Base pointers), but you don't show the definition of the class, so it's not obvious whether it has a copy constructor and assignment operator or not.

Finally, you haven't shown where the Base objects are being allocated. Is it possible that they are being passed into an Army object and also deleted somewhere outside the Army object? Or perhaps the Base* contained by the Army object are referring to objects on the stack that should not be deleted?

Answer 5

You didn't post the part of the code that has the problem, because all that looks pretty normal. However, it's got a lot of const-correctness problems:

Base(char* baseName, char* cityName);

Strings should be passed as const char* unless they are being modified.

virtual ~Base();

No idea if this needs to be virtual; can't see what its subclasses are.

list<Vehicle*>::const_iterator GetEndList();

Should be a const method, since it's a const_iterator: list<Vehicle*>::const_iterator GetEndList() const;

char* GetBaseName() const;
char* GetLocation() const;

These should return const char*, since your code is not set up to handle the name and location being changed.

list<Vehicle*>::const_iterator FindVehicle(char* id);

Again, should be a const method: list<Vehicle*>::const_iterator FindVehicle(char* id) const;

Base::~Base()
{
    delete [] m_name;
    delete [] m_location;
    //m_baseVehicles.clear();
}

You don't need m_baseVehicles.clear(); because it happens anyway right after the destructor. However, you need to delete the vehicles if they aren't referenced elsewhere, or you'll get a leak.

army distructor :

"destructor". And where is the rest of Army?

Army::~Army()
{
    list<Base*>::iterator baseIter = m_basesList.begin();
    for (baseIter ; baseIter != m_basesList.end() ; ++baseIter)
        delete (*baseIter);
    m_basesList.clear();
 }  

Again, you don't need m_basesList.clear();.

Answer 6

Nothing wrong with the piece of code given. But with this kind of code, there is a high possiblity of mutiple deletion, I mean deletin a memory block twice which leads to heap corruption.