I was working on a project with a friend the other day and he had code that looked similar to this:

std::unordered_map<std::string, Person* person> map;

for(int i = 0; i < N; i++)
{
    Person person_to_add;
    /* create members for person */
    map[person_to_add.name] = &person_to_add;
}

map["Tom"].email = "something";

This is funny, because to me, this code is completely wrong. Person_to_add goes out of scope after each iteration of the loop so it's address should be pointing to some location on the stack, no?

Anyway, what is curious, and why we had a hard time finding the bug was that the project will go on working as normal this way, it will just result in a memory leak. What am I missing here? Does the c++ container dynamically allocate 'Person' at some point and never free it? Or is there something else going on?

Thanks.

Yes, the code is wrong. At the best case, accessing any Persons inside map stomp on memory that doesn't belong to the Person. At the worst case, you won't notice memory corruption until much later.

Does the memory leak report include where the memory is leaking? I do not see any memory leaks here as the Person*s weren't allocated on the heap.

Also, your friend should consider using a smart pointer (such as a shared_pointer) if map owns the Persons it contains. This way, the Persons are automatically deleted when the map expires.

I suggested that. He is in a beginning C++ class that has yet to cover smart pointers, so he is unsure if he would credit deducted for using them. The memory leak looks as such:

This is the report on a side project I made right now to reproduce the issue where I am using a vector instead of a map, as it repeats with any collection.

Show us the exact code. Also, the implementation of Person.

Sure thing, as I said I don't have his exact implementation but I reproduced it with this:

#include <iostream>
#include <vector>

class Person
{
public:
    std::string email;
    std::string name;
};

int main(int argc, char** argv)
{
    std::vector<Person*> some_list;

    for(int i = 0; i < 10; i++)
    {
        Person some_person;
        some_list.push_back(&some_person);
    }

    map[1]->email = "blah";
    std::cout << map[1]->email << std::endl;
    return 0;
}

I understand the code is wrong.

I am more interested in why it's a memory leak. I think summing it up to undefined behavior is accurate though.

Here's what's going on, and the explanation for your leak:

- You create a Person object, and put a pointer to it in some_list.

- When some_person goes out of scope, Person is destructed. At this point, the two strings inside it (email and name) will also be destructed. And as you know, you still have a pointer to this (now garbage/undefined) memory that used to contain a valid Person.

- When you make this assignment:

map[1]->email = "blah";

it will call std::string's assignment operator. What that does is allocate new memory, and copy "blah" into that new memory. Technically this is the undefined behavior, since you shouldn't be calling methods on objects that have been destructed.

And that's it. There's your leak. You invoked std::string's assignment operator, which makes an allocation. But that std::string was already destructed, meaning its destructor will never be called. And thus the destructor for the mail and name strings will never be called.

Thank you!

The fact that I didn't understand 'why' it was a leak made me feel like I was lacking some basic knowledge, which I was. So thank you for spelling it out for me.