Memory Leak issue with my code, I am newbie

Question

I keep getting a Memory Leak which seems to occur because the new initialized objects passed as pointers to add(…) are not actually delete. But somehow, I cannot make this work properly. I tried adding a for-loop to loop every object in array within class Monsters to delete each object when the call of the destructor is invoked. And then to do delete[] monsters; and monsters = nullptr.

https://repl.it/@xxxxxx10/TatteredTroubledTasks

---

#include <iostream>
#include <string>

class Monster{
public:
  Monster(){};
  Monster(std::string name) : name(name){}
private:
  std::string name = "";
};

class MonstersList {
public:
  MonstersList(int max):max(max){
    monsters = new Monster[max];
  }
  
  virtual ~MonstersList() {
      delete[] monsters;
  }

  void add(const Monster* m){
    monsters[total++] = *m;
  }

 Monster* begin() const { return monsters; }
 Monster* end() const { return monsters + total; }

private:
  Monster* monsters = nullptr;
  int max = 0;
  int total = 0;
};

MonstersList* InitListOfMonsters()
{
    MonstersList* monsters = new MonstersList(2);
    monsters->add(new Monster("Sally"));
    monsters->add(new Monster("Rich"));
    
    return monsters;
}

int main()
{
  MonstersList* monsters = InitListOfMonsters();
  
  // Do something here...

  return 0;
} // <----- Memory leak!

How can this be prevented from happening, recall, I tried to for loop and delete each monster pointer object in the array.

EDIT: When I try for-loop to delete each object, then an error sort of occurs, breaks at delete_scalar.cpp <-- Microsoft C++ file.

_CRT_SECURITYCRITICAL_ATTRIBUTE
void __CRTDECL operator delete(void* const block) noexcept
{
    #ifdef _DEBUG
    _free_dbg(block, _UNKNOWN_BLOCK);
    #else
    free(block);
    #endif
}

Occurs when I run this code in destructor:

if (monsters != nullptr) {
    for (Monster* m = begin(); m != end(); m++) {
        delete m;
    }

    delete[] monsters;
    monsters = nullptr;
}

Answer 1

There are two places, where the code leaks memory

MonstersList* InitListOfMonsters()
{
    // ...
    monsters->add(new Monster("Sally"));
    // ...
    
    return monsters;
}

Each of the news alloctes memory, which is never freed. Since you don't store pointers, but Monster objects, you should not pass pointers, but Monsters.

monsters->add(Monster("Sally"));

and modify add() as

void add(Monster const &m) {
    monsters[total++] = m;
}

The second one I see, is in main

MonstersList* monsters = InitListOfMonsters();

there's no corresponding

delete monsters;

The destructor is not executed until the object is destroyed. This happens, when you call delete monsters.

---

Update for the EDIT

This code has two issues

if (monsters != nullptr) {
    for (Monster* m = begin(); m != end(); m++) {
        delete m;
    }

    delete[] monsters;
    monsters = nullptr;
}

the loop is equivalent to

for (Monster* m = monsters; m != (monsters + total); m++) {
    delete m;
}

The first delete is delete monsters, which deletes the whole array and causes a double delete with the later delete[] monsters. Furthermore does it delete the following elements, which are not allocated separately, but as part of the whole array (which was already deleted in the first iteration).

The second issue is delete vs delete[]. A simple delete is for single objects, whereas delete[] is for dynamically allocated arrays. In this case the first delete m inside for deletes an array with the non-array delete.

---

Finally, heed the suggestions and warnings in the comments. When you use C++, especially C++11 and newer, do it the C++ way. Use vector and unique_ptr, if pointers are really necessary.

Answer 2

Well here's my old school effort. See comments at various points in the code for the things you were missing.

References

What is The Rule of Three?

What is the copy-and-swap idiom?

It is good to know this stuff, although that doesn't mean you should be old school. Modern C++ is better.

#include <iostream>
#include <algorithm>
#include <string>

class Monster
{
public:
    Monster() {};
    Monster(std::string name) : name(name) {}
private:
    std::string name = "";
};

class MonstersList
{
public:
    MonstersList(int max) : monsters(new Monster[max]), max(max), total(0)
    {
    }

    // copy constructor, essential if your destructor deletes, see rule of three
    MonstersList(const MonstersList& rhs) : monsters(new Monster[rhs.max]), max(rhs.max), total(rhs.total)
    {
        std::copy(rhs.begin(), rhs.end(), begin());
    }

    // assignment operator, again essential with a deleting destructor, 
    // this implementation uses the copy and swap idiom
    MonstersList& operator=(MonstersList rhs)
    {
        swap(rhs);
        return *this;
    }

    virtual ~MonstersList()
    {
        delete[] monsters;
    }

    void swap(MonstersList& rhs)
    {
        std::swap(monsters, rhs.monsters);
        std::swap(max, rhs.max);
        std::swap(total, rhs.total);
    }

    void add(const Monster& m) // pass by reference
    {
        monsters[total++] = m;
    }

    Monster* begin() const { return monsters; }
    Monster* end() const { return monsters + total; }

private:
    Monster* monsters;
    int max;
    int total;
};

MonstersList* InitListOfMonsters()
{
    // allocate a list of monsters, but no need to allocate the monsters themselves
    MonstersList* monsters = new MonstersList(2);
    monsters->add(Monster("Sally"));
    monsters->add(Monster("Rich"));

    return monsters;
}

int main()
{
    MonstersList* monsters = InitListOfMonsters();
    // Do something here...

    // don't forget to delete the monster list
    delete monsters;
    return 0;
}