Destructor called on std::vector invalidating for_each

Question

I've been doing some reading about delete[], new[], what-is-the-rule-of-three, deallocating-objects-stored-in-a-vector, delete-vector-class-member and encountering an

Exception thrown: read access violation.

I have defined a base class

class Person
{
private:
    std::string name;
    int age;
    static int cur_id;
public:
    Person() {}
    Person(std::string new_name, int new_age) : name(new_name), age(new_age) { cur_id += 1; }
    virtual ~Person() {}
    void setname(std::string name) { this->name = name; }
    std::string getname() { return name; }
    int getage() { return age; }
    void setage(int age) { this->age = age; }
    int get_id() { return cur_id; }

    virtual void getdata(void) = 0;
    virtual void putdata(void) = 0;
};

Derived class

class Professor : private Person
{
private:
    int publications;
    Professor* prof;
public:
    Professor() {}
    Professor(std::string new_name, int new_age, int new_pubs) : Person(new_name, new_age) { this->publications = new_pubs; }
    ~Professor() override { delete prof; }
    void getdata() override 
    {
        std::vector<std::string> prof_input = get_input();
        std::cout << prof_input[0] << "\n";
        std::cout << std::stoi(prof_input[1]) << "\n";
        std::cout << std::stoi(prof_input[2]) << "\n";
        prof = new Professor(prof_input[0], std::stoi(prof_input[1]), std::stoi(prof_input[2]));
    }
    void putdata() override 
    {
        std::cout << prof->getname() << " " << prof->getage() << " " << prof->publications << " " << prof->get_id();
    }
};

and have defined main

int main()
{
    int size; 

    std::cin >> size;

    std::cin.ignore();

    Professor* prof = new Professor();

    std::vector<Professor> prof_vec;

    for (int i = 0; i < size; i++)
    {
        int which;
        std::cin >> which;
        std::cin.ignore();

        switch (which)
        {
        case 1: 
        {
            prof->getdata();
            prof_vec.push_back(*prof);
        }
        break;
        case 2:
        {
            // something else
        }
        break;
        }
    }

    for (auto prof : prof_vec)
    {
        prof.putdata();
    }  <<< ------------------- Things so south here

    delete prof;

    fgetc(stdin);

    return 0;
}

What I figured was happening is (assuming only one element in the vector), once the call to putdata() exited the block, the destructor call delete prof occurs followed by

this->prof was 0xCDCDCDCD.

I'm attempting to delete an object that doesn't exist. If I want to be sure to properly delete, what should I be doing to ensure no memory leaks even with such a simple example?

If you really want to ensure no memory leaks in a modern C++ way, use a [smart pointer](https://stackoverflow.com/questions/106508/what-is-a-smart-pointer-and-when-should-i-use-one) (link pretty old, c++11's smart pointer is named `std::unique_ptr`). — spectras, Oct 18 '17 at 00:17
@spectras I entirely agree with you. My example is contrived for learning proper memory management but I would use `std::unique_ptr`, `std::shared_ptr` with `std::make_unique()` or `std::make_shared()` in a hearbeat — Mushy, Oct 18 '17 at 00:20
Okay. Well in your program, `for (auto prof : prof_vec)` makes a temporary copy of each professor while iterating. It's possible because you didn't disable copy constructor and your class has a default constructor and only trivial non-static members. Except the compiler-generated copy constructor simply copies the pointer, which is bound to go wrong. Use `for (auto & prof : prof_vec)` if you don't want to copy them. And disable that copy constructor you'll also catch the push_back() copy. — spectras, Oct 18 '17 at 00:24
Please read what a [mcve]. The vast majority of this code is completely irrelevant to the problem. — Barry, Oct 18 '17 at 00:27
@Barry Will do and thank you. I get dinged for showing minimal examples using contrived constructs and people end up asking for more code so I tend be a bit more on the verbose side. — Mushy, Oct 18 '17 at 01:10
You should not have `Professor* prof;` as a class member of Professor, that makes no sense at all . Instead the `getdata()` function should initialize the current instance. — M.M, Oct 18 '17 at 01:42

score 4 · Answer 1 · answered Oct 18 '17 at 00:25

You're violating the rule of 3/5/0:

class Professor : private Person
{
private:
    Professor* prof;
public:
    // ...
    ~Professor() override { delete prof; }
};

If you provide one of the special member functions, you should provide all of the special member functions. In this case, your copy constructor just member-wise copies the Professor* and now you have two different objects trying to delete the same pointer.

Prefer the rule of 0:

class Professor : private Person
{
private:
    std::unique_ptr<Professor> prof;
public:
    // ...
    ~Professor() = default;
};

@M.M Yeah probably, didn't even notice that they were the same type! Just saw the lack of special members and my inner siren went off. — Barry, Oct 18 '17 at 02:26

score 2 · Answer 2 · edited Oct 18 '17 at 01:52

2

The member Professor* prof is not initialized in the constructor Professor(), and is only initialized in getdata();

When the code goes to case 2, if there is not any call to getdata(), then the prof is left uninitialized.

The 0xCDCDCDCD indicates an uninitialized value in debug mode.

edited Oct 18 '17 at 01:52

Remy Lebeau

454,445
28
366
620

answered Oct 18 '17 at 00:46

James Dong

384
2
7

Destructor called on std::vector invalidating for_each

2 Answers2