Valgrind indicates memory leak with shared_ptr c++11

Question

I've implemented a Linked list using C++11 smart pointers. This implementation uses shared_ptr to store the internal data struct, which is used to implement implicit-sharing of it. I provide the relevant part of the source code bellow:

namespace Algos {

template <typename T>
struct LinkedListData{
    struct Node {
        std::unique_ptr<Node> next;
        Node *prev = nullptr;
        T data;
    };

    std::unique_ptr<Node> root;
    Node *last = nullptr;
    int size = 0;

    LinkedListData() {
        root = std::make_unique<Node>();
        root->prev = nullptr; //last virtual element
        root->next = std::make_unique<Node>();
        last = root->next.get();
        last->prev = root.get();
        last->next = nullptr;
    }

    //deferr pointers manually to avoid stackoverflow due to 
    //recursion explosion
    static void cleanup(LinkedListData<T> *data) { 
        #ifdef DEBUG_TXT 
            int nodeCount=0;
        #endif
        Node *n = data->last;
        if(n==nullptr) { return; }
        while(n) {
            #ifdef DEBUG_TXT 
                if(n->next.get())
                    std::cout << "Release {n->next()} [" << ++nodeCount  << 
                    "]: "<< n->next.get() << std::endl;
            #endif
            n->next.release();
            ALGO_ASSERT(n->next.get() == nullptr, "Node reference not deferred");
            n = n->prev;
        }
        data->size = 0;
        #ifdef DEBUG_TXT
            std::cout << "Release {Root} [" << ++nodeCount  << "]: "<< data->root.get() << std::endl;
        #endif
        data->root.release();
    } 
};

template <class T>
class LinkedList {

    typedef typename LinkedListData<T>::Node node_type;
    std::shared_ptr<LinkedListData<T> > d;

    public:

        LinkedList() : d(new LinkedListData<T>(), LinkedListData<T>::cleanup){}

/* Code omitted .... */

};

}

The following code is triggering memory leak error on valgrind in the shared_pointer constructor due to the use of new:

#include <iostream>
#include <string>
#include <unistd.h>

// #define DEBUG_TXT

#include "global/assert.h"

#include "linked_list/linkedlist.h"

#define TEST_SIZE 5000

struct DataTest {
   int integer;
   bool boolean;
   std::string txt;
   DataTest& operator=(const DataTest& other) {
      integer = other.integer;
      boolean = other.boolean;
      txt = other.txt;
      return (*this);
   }

   bool operator==(const DataTest& other) const {
      return (
         integer == other.integer &&
         boolean == other.boolean &&
         txt == other.txt
      );
    }   
};

struct Data {
    DataTest data[TEST_SIZE];
    const int n = TEST_SIZE;

    static void initDataSample(Data &d) {
        for(int i=0; i<d.n; i++) {
            d.data[i].integer = i;
            d.data[i].boolean = (i%2 == 0);
            d.data[i].txt = "abc";
        }
    }
};

void appendElements(Algos::LinkedList<DataTest> &l, const Data& d){
    for(int i=0; i<d.n; i++) {
        l.append(d.data[i]);
    }
}

void prependElements(Algos::LinkedList<DataTest> &l, const Data& d) {
    for(int i=d.n-1; i>=0; i--) {
        l.prepend(d.data[i]);
    }
}



int main(int argv, char* argc[]) {

   Data d;  
    Data::initDataSample(d);

    Algos::LinkedList<DataTest> l1;
    {
        Algos::LinkedList<DataTest> l2;
        l1 = l2;
    }

    sleep(2);

    appendElements(l1, d);

    int removeSize = l1.size()/2;

    for(int i=0; i<removeSize; i++)
        l1.takeFirst();


    prependElements(l1, d);

    removeSize = l1.size()/2;
    for(int i=0; i<removeSize; i++)
        l1.takeLast();


   return 0;
}

This is the message I'm getting in the valgrind console:

> ==8897== HEAP SUMMARY:
==8897==     in use at exit: 282,976 bytes in 3,757 blocks
==8897==   total heap usage: 10,009 allocs, 6,252 frees, 633,040 bytes allocated
==8897== 
==8897== 136 (24 direct, 112 indirect) bytes in 1 blocks are definitely lost in loss record 5 of 8
==8897==    at 0x4C2E216: operator new(unsigned long) (vg_replace_malloc.c:334)
==8897==    by 0x407C1C: Algos::LinkedList<DataTest>::LinkedList() (linkedlist.h:74)
==8897==    by 0x4074B4: main (insert_delete_rounds.cpp:63)
==8897== 
==8897== 210,136 (24 direct, 210,112 indirect) bytes in 1 blocks are definitely lost in loss record 8 of 8
==8897==    at 0x4C2E216: operator new(unsigned long) (vg_replace_malloc.c:334)
==8897==    by 0x407C1C: Algos::LinkedList<DataTest>::LinkedList() (linkedlist.h:74)
==8897==    by 0x4074C3: main (insert_delete_rounds.cpp:65)
==8897== 
==8897== LEAK SUMMARY:
==8897==    definitely lost: 48 bytes in 2 blocks
==8897==    indirectly lost: 210,224 bytes in 3,754 blocks
==8897==      possibly lost: 0 bytes in 0 blocks
==8897==    still reachable: 72,704 bytes in 1 blocks
==8897==         suppressed: 0 bytes in 0 blocks
==8897== Reachable blocks (those to which a pointer was found) are not shown.
==8897== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==8897== 
==8897== For counts of detected and suppressed errors, rerun with: -v
==8897== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

As far as I know there is no other way to initiate a shared_ptr with custom Deleter without using new in C++11. After reading the documentation and different threads on stackoverflow, I noticed that std::make_shared() doesn't support passing a custom Deleter. So, my question is: Is this memory leak warning legit ? If yes, is it possible to avoid it ?

Dev tools setup:

• gcc: 5.4.0
• Valgrind: 3.13.0
• OS: Linux Ubuntu 16.04

Answer 1

Since you are using smart pointers internally you should not specify LinkedListData::cleanup as a custom deleter and just use default one. If you are trying to stick with smart pointers it may be a good idea to avoid using new calls if you are not explicitly calling delete. So, d member may be initialized with just d(std::make_shared<LinkedListData<T>>()). Also, it maybe can be declared just as LinkedListData<T> d; without any smart pointers there.

Then, you should not call release in LinkedListData::cleanup. This method detaches raw pointer from the smart pointer wrapper and does not deallocate associated object. You probably want reset instead. But once again, this method should not be necessary at all since all your pointers are smart and associated data should be automatically cleaned up when the parent destructor is called.

Answer 2

You create a new LinkedListData<T>(), but you don't call delete on it inside cleanup.

Answer 3

To answer your questions:

1. You make use allocate_shared to allow the use of a custom memory allocator. The signature is identical, except it takes a const reference to an allocator as the first argument.

2. Although you may clean up the internals of LinkedListData, you never delete the allocated pointer in your cleanup method. cleanup should be called from the LinkedListData destructor, and you should use the regular deleter (or a custom deleter if using a custom allocator) to call delete on the LinkedListData pointer.

In short, this line will always lead to a memory leak:

LinkedList() : d(new LinkedListData<T>(), LinkedListData<T>::cleanup){}