Getting CrtIsValidHeapPointer Error when trying to release memory [C++]

Question

I have an exercise and I need to create an Engine class and a Car class.

Those are parts of my code:

Car.cpp

Car::Car()
{
    _engine = new Engine();
    _manufacturer = new char[10];
    _prod_year = 0;
    _color = new char[10];
}

Car::~Car()
{ 
    // Releasing allocated memory
    delete[] _manufacturer;
    delete[] _color;
    delete _engine;
}

void Car::print(void) const
{
    (*_engine).print();
    cout << "\n" << endl;
    cout << "Manufacturer:" << _manufacturer << "\n" << endl;
    cout << "Production Year:" << _prod_year << "\n" << endl;
    cout << "Color:" << _color << endl;
}

Car.h

class Car
{

    Engine * _engine;
    char * _manufacturer;
    int _prod_year;
    char * _color;

    public:

    /*
    * Creates a Car with the default set of attributes.
    */

    Car();
    virtual ~Car();
    inline Engine * GetEngine() { return _engine; }
    void SetEngine(Engine * engine) { _engine = engine; }
    inline char * GetManufacturer(){ return _manufacturer; }
    void SetManufacturer(char * manufacturer){_manufacturer = manufacturer;}
    inline int GetProdYear() { return _prod_year; }
    void SetProdYear(int prod_year) { _prod_year = prod_year; }
    inline char * GetColor() { return _color; }
    void SetColor(char * color) { _color = color; }
    void print(void) const;
};

Engine.h

class Engine
{
    /*  
        Represents an Engine with the following attributes:
        * int Hourse Power
        * char * Manufacturer
        * int Production Year
    */

    int _horse_power;
    char * _manufacturer;
    int _prod_year;

public:
    Engine();
    virtual ~Engine();
    int GetHorsePower() { return _horse_power; }
    void SetHorsePower(int horse_power) { _horse_power = horse_power; }
    char * GetManufacturer(){ return _manufacturer; }
    void SetManufacturer(char * manufacturer){_manufacturer = manufacturer;}
    int GetProdYear() { return _prod_year; }
    void SetProdYear(int prod_year) { _prod_year = prod_year; }
    void print() const;

};

Exc.cpp

Engine engine;
engine.SetHorsePower(150);
cout << "Current Horse Power: " <<engine.GetHorsePower()<<endl;

char * manufacturer = new char[strlen("VP") +1];
strcpy(manufacturer, "VP");
engine.SetManufacturer(manufacturer);
cout << "Current Manufacturer: " << engine.GetManufacturer() << endl;

engine.SetProdYear(1995);
cout << "Current Production Year: " << engine.GetProdYear() << endl;

cout << "\n ------------- Printing engine details -------------\n" << endl;
engine.print();
Car car;
car.SetEngine(&engine);
cout << "Current Engine: " << endl;
(*car.GetEngine()).print();

char * car_manufacturer = new char[strlen("Toyota") + 1];
car_manufacturer = { "Toyota" };
car.SetManufacturer(car_manufacturer);
cout << "Current Car Manufacturer: " << car.GetManufacturer() << endl;

car.SetProdYear(2010);
cout << "Current Car Production Year: " << car.GetProdYear() << endl;

char * color = new char[strlen("Yellow") + 1];
color = { "Yellow" };
car.SetColor(color);
cout << "Current Car Color: " << car.GetColor() << endl;

cout << "\n ------------- Printing car details -------------\n" << endl;

car.print();

My program is doing ok until it gets to ~Car and then I get "CrtIsValidHeapPointer".

Can someone tell me what I did wrong?

Thx.

---

SOLUTION

Engine * engine = new Engine;
engine->SetHorsePower(150);
cout << "Current Horse Power: " << engine->GetHorsePower()<<endl;

char * manufacturer = new char[strlen("VP") +1];
strcpy(manufacturer, "VP");
engine->SetManufacturer(manufacturer);
cout << "Current Manufacturer: " << engine->GetManufacturer() << endl;

engine->SetProdYear(1995);
cout << "Current Production Year: " << engine->GetProdYear() << endl;

cout << "\n ------------- Printing engine details -------------\n" << endl;
engine->print();

Car car;
car.SetEngine(engine);
cout << "Current Engine: " << endl;
car.GetEngine()->print();

char * car_manufacturer = new char[strlen("Toyota") + 1];
strcpy(car_manufacturer, "Toyota");
car.SetManufacturer(car_manufacturer);
cout << "Current Car Manufacturer: " << car.GetManufacturer() << endl;

car.SetProdYear(2010);
cout << "Current Car Production Year: " << car.GetProdYear() << endl;

char * color = new char[strlen("Yellow") + 1];
strcpy(color, "Yellow");
car.SetColor(color);
cout << "Current Car Color: " << car.GetColor() << endl;

cout << "\n ------------- Printing car details -------------\n" << endl;

car.print();

return 0;

Answer 1

In the Car constructor you dynamically allocate an Engine and assign to _engine.

Then in your main program you do

car.SetEngine(&engine);

thereby changing _engine to point to an object you haven't allocated dynamically with new, and losing the original pointer and giving you a memory leak.

So in the Car destructor you try to delete a pointer you haven't allocated with new leading to undefined behavior.

There are also problems with the strings, where you do thing like

char * car_manufacturer = new char[strlen("Toyota") + 1];
car_manufacturer = { "Toyota" };

That first allocates memory, and you assign a pointer to that memory to car_manufacturer. Then directly afterward you make car_manufacturer point somewhere else again losing the pointer to the memory you have allocated and a new memory leak. Besides you then make set the pointer in the car object to the string literal "Toyota" which again is a pointer to memory you have not allocated with new[] so doing delete[] again leads to undefined behavior.

---

Solving the problem with the strings are easy, even if you don't want to use std::string (which is the recommended solution), and that is to copy the string instead of reassigning the pointer:

char * car_manufacturer = new char[strlen("Toyota") + 1];
strcpy(car_manufacturer, "Toyota");

To solve the first problem with the engine, you need to allocate the new engine dynamically too, if you have to call SetEngine.

In your Set function you should also free the memory already allocated, so you don't have memory leaks.