vector::push_back ( A * ) creating leaks?

Question

I'm trying to create a really simple virtual filesystem (represented as a tree)(for my FTP server) mapped to several places on my real one. Nodes are represented by a Node object which holds pointers to its contents in a vector.

I know that for every new there is a delete for the correct object (debugging to cout), yet I get some "definitely lost" bytes from valgrind.

I am holding the pointers not only in the objects, but also in the Filesystem class and I am deleting them in Filesystem's destructor, so things happening to the pointers held in Node shouldn't matter... right? Or am I missing something here?

valgrind:

==23638== 8 bytes in 1 blocks are definitely lost in loss record 1 of 1
==23638==    at 0x4C2B0E0: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==23638==    by 0x407786: __gnu_cxx::new_allocator<Node*>::allocate(unsigned long, void const*) (new_allocator.h:104)
==23638==    by 0x40733F: std::_Vector_base<Node*, std::allocator<Node*> >::_M_allocate(unsigned long) (stl_vector.h:168)
==23638==    by 0x406FEB: _ZNSt6vectorIP4NodeSaIS1_EE19_M_emplace_back_auxIJS1_EEEvDpOT_ (stl_vector.h:404)
==23638==    by 0x406F2F: _ZNSt6vectorIP4NodeSaIS1_EE12emplace_backIJS1_EEEvDpOT_ (vector.tcc:101)
==23638==    by 0x40404F: std::vector<Node*, std::allocator<Node*> >::push_back(Node*&&) (stl_vector.h:920)
==23638==    by 0x402FD1: Directory::addDirectoryNode(Directory*) (Directory.cpp:20)
==23638==    by 0x409600: Filesystem::addDirectory(std::string, std::string) (Filesystem.cpp:47)
==23638==    by 0x40B519: Configuration::loadPairs(std::basic_ifstream<char, std::char_traits<char> >&) (Configuration.cpp:85)
==23638==    by 0x40B85C: Configuration::load(std::string) (Configuration.cpp:103)
==23638==    by 0x4081A9: main (main.cpp:41)

I am not quite sure what else would be useful.

Filesystem.hpp:

class Filesystem
{
    public:
        Filesystem ( );
        ~Filesystem ( );
        void addDirectory ( string name, string path ); // the first string is the name of the dir in the virtual FS, path is where the directory actually is. 
        void addFile ( string );
    private:
    Directory * root;
    Directory * pwd;
    map <string, Node *> nodes;
};

Filesystem::~Filesystem ( )
{
    for ( auto & it : nodes )
    {
        if ( it.second != NULL )
        {
            cout << "deleting " << it . second -> getName ( ) << endl;
            delete it.second;
        }
    }
}

class Directory : public Node
{
    public:
        Directory ( string name, string path, Directory * mum ) : Node ( name, path ) { parent = mum; }; //assigns name and path, should be clear
        void addDirectoryNode ( Directory * );
    private:
    Directory * parent = NULL;
    vector <Node *> contents;
};

void Filesystem::addDirectory ( string name, string path ) 
{
    Directory * toAdd = new Directory ( name, path + "/", NULL );
    cout << "created " << name << ", path: " << toAdd -> getPath ( ) << endl;
    map <string, Node * > newEntries = toAdd -> loadDirectoryContents ( toAdd -> getDirectoryContents ( ) );
    nodes . insert ( pair <string, Node * > ( toAdd -> getPath ( ), toAdd ) );
    nodes . insert ( newEntries . begin ( ), newEntries . end ( ) );

    if ( name == "/" ) // I know that the first directory added will be "/"
        root = pwd = toAdd;

    else
        root -> addDirectoryNode ( toAdd );
}

// reads the content of this directory and returns it in a set, directories will have a "/" appended
set<string> Directory::getDirectoryContents (  ) const
{
    DIR * dir;
    dirent * currNode;
    set<string> names;

    dir = opendir ( path . c_str ( ) );

    while ( ( currNode = readdir ( dir ) ) )
    {
        if ( string ( currNode -> d_name ) == "." || string ( currNode -> d_name ) == ".." )
            continue;

        if ( currNode -> d_type == DT_DIR ) 
            names.insert ( string ( currNode-> d_name ) + "/" ) ;

        else if ( currNode -> d_type == DT_REG )
            names.insert ( string ( currNode -> d_name ) ) ;

    }

    closedir ( dir );
    return names;
}

// recursively loads the real contents of this directory into the virtual one
map <string, Node * > Directory::loadDirectoryContents ( set <string> names )
{
    map <string, Node * > newEntries;
    for ( auto & it : names )
    {
        if ( it [ ( it . length ( ) - 1 ) ], "/" )
        {
            Directory * newDir = new Directory ( it, path + it, this );
            map <string, Node * > newSubEntries = newDir -> loadDirectoryContents ( newDir -> getDirectoryContents ( ) );
            contents . push_back ( newDir );
            newEntries . insert ( pair <string, Node * > ( path + it, newDir ) );
            newEntries . insert ( newSubEntries . begin ( ), newSubEntries . end ( ) );
        }
        else
        {
            File * newFile;
            contents . push_back ( newFile = new File ( path + it, path ) ); 
            newEntries . insert ( pair <string, Node * > ( it, newFile ) ); 
        }
    }

    return newEntries;
}

void Directory::addDirectoryNode ( Directory * directory )
{
    contents.push_back ( directory );
}

_"I know that for every new there is a delete for the correct object (debugging to cout)"_ Apparently not! What would be useful, as always, is a [testcase that reproduces the problem](http://stackoverflow.com/mcve). :) — Lightness Races in Orbit, Jun 17 '15 at 18:44
Are you calling this like `addDirectoryNode(new Directory());`? Depending on who's claiming ownership of these pointers, you may want to consider changing `contents` to a `std::vector>` — Cory Kramer, Jun 17 '15 at 18:46
`I am holding the pointers not only in the objects, but also in the Filesystem class and I am deleting them in Filesystem's destructor,` Describing what you're doing is not enough. If we could make programs bug-free by describing them, there would be no bugs. Let's see the code. — PaulMcKenzie, Jun 17 '15 at 18:47
@CoryKramer How so, if she made sure to call `delete` on every pointer held in the `contents` container? — jaggedSpire, Jun 17 '15 at 18:49
@jaggedSpire I will assume they aren't doing so until I see them prove otherwise :) — Cory Kramer, Jun 17 '15 at 18:49
@Lea In the end, you're going to need to post a code sample reproducing the problem, so we can spot where you're missing your `delete`s — jaggedSpire, Jun 17 '15 at 18:51
@CoryKramer any reason you prefer `unique_ptr` to `shared_ptr`? — user4581301, Jun 17 '15 at 18:53
@user4581301 It's not up to me, I don't know the design of OP's program, they would have to decide which of the two (if either) is more appropriate. I just meant they should consider smart pointers in general — Cory Kramer, Jun 17 '15 at 18:55
@CoryKramer thanks. Just wondering if you had something sneaky in mind I wasn't seeing. — user4581301, Jun 17 '15 at 19:07
@user4581301: Generally speaking, you *should* prefer `unique_ptr` over `shared_ptr`. The need for shared ownership is something that should be demonstrated, and if it is not, then one should default to `unique_ptr` (assuming of course, that simply storing objects by value has already been ruled out for a very good reason) — Benjamin Lindley, Jun 17 '15 at 19:37

score 1 · Accepted Answer · answered Jun 17 '15 at 22:06

If you do not have virtual destructor in your Node class, deleting your Directory objects through base class pointer is undefined behaviour. Your Directory class' s destructor won't be called, also the destructor of the contained vector. This might cause the memory leak.

vector::push_back ( A * ) creating leaks?

1 Answers1