Advertisement Jump to content
Sign in to follow this  
DishSoap

Memory Leak Question C++

This topic is 1873 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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. 

Edited by DishSoap

Share this post


Link to post
Share on other sites
Advertisement
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.

Share this post


Link to post
Share on other sites

That code shows no memory leak.  It shows undefined behaviour.  It's allowed to syphon money from your bank account and use it to support terrorism.  Or appear to work normally.  That's the nature of undefined behaviour.  It's undefined.

Share this post


Link to post
Share on other sites

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:

 

==8448== HEAP SUMMARY:
==8448==     in use at exit: 17 bytes in 1 blocks
==8448==   total heap usage: 6 allocs, 5 frees, 141 bytes allocated
==8448==
==8448== 17 bytes in 1 blocks are definitely lost in loss record 1 of 1
==8448==    at 0x402BE94: operator new(unsigned int) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==8448==    by 0x40E9403: std::string::_Rep::_S_create(unsigned int, unsigned int, std::allocator<char> const&) (in /usr/lib/i386-linux-gnu/libstdc++.so.6.0.18)
==8448==    by 0x40E96BA: std::string::_M_mutate(unsigned int, unsigned int, unsigned int) (in /usr/lib/i386-linux-gnu/libstdc++.so.6.0.18)
==8448==    by 0x40E9F65: std::string::_M_replace_safe(unsigned int, unsigned int, char const*, unsigned int) (in /usr/lib/i386-linux-gnu/libstdc++.so.6.0.18)
==8448==    by 0x40E9FFE: std::string::assign(char const*, unsigned int) (in /usr/lib/i386-linux-gnu/libstdc++.so.6.0.18)
==8448==    by 0x40EA1DF: std::string::operator=(char const*) (in /usr/lib/i386-linux-gnu/libstdc++.so.6.0.18)
==8448==    by 0x419F934: (below main) (libc-start.c:260)
 
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.
 

 

That code shows no memory leak.  It shows undefined behaviour.  It's allowed to syphon money from your bank account and use it to support terrorism.  Or appear to work normally.  That's the nature of undefined behaviour.  It's undefined.

 

Hahaha, I liked this comment. That's where that $10.43 went...

 

So on different systems that code could result in completely different behavior? 

Share this post


Link to post
Share on other sites

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.

Edited by phil_t

Share this post


Link to post
Share on other sites

 


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;
}

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites
While clearly not the case in your code sample, in general code like this Person might have an overloaded operator& that does something other than the obvious. This is one of the reasons that highly generic code (containers and the like) need to use std::addressof instead of operator& to get the address of a value.

This is not in itself a leak. A leak is where you allocate memory and then never use it again without freeing it (this can happen even in GC'd languages if you have a large data structure that you stuff values into but then never actually use again). This is more of a dangling pointer problem. If a leak is being detected this could be to other values (like the strings) stored in Person not being properly released because they're being trampled.

Share this post


Link to post
Share on other sites

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. 

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!