0

I found this memory leak from Valgrind which gave the memory error: Invalid free() / delete / delete[] / realloc().

==7367== Invalid free() / delete / delete[] / realloc()
==7367==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7367==    by 0x40077F: ItemSet::~ItemSet() (cart.cpp:33)
==7367==    by 0x40086B: ShoppingCart::~ShoppingCart() (cart.cpp:14)
==7367==    by 0x400828: main (cart.cpp:45)
==7367==  Address 0x5a1c0b0 is 0 bytes inside a block of size 40 free'd
==7367==    at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7367==    by 0x40077F: ItemSet::~ItemSet() (cart.cpp:33)
==7367==    by 0x400800: ShoppingCart::ShoppingCart() (cart.cpp:39)
==7367==    by 0x400817: main (cart.cpp:43)
==7367== 
==7367== 
==7367== HEAP SUMMARY:
==7367==     in use at exit: 40 bytes in 1 blocks
==7367==   total heap usage: 2 allocs, 2 frees, 80 bytes allocated
==7367== 
==7367== LEAK SUMMARY:
==7367==    definitely lost: 40 bytes in 1 blocks
==7367==    indirectly lost: 0 bytes in 0 blocks
==7367==      possibly lost: 0 bytes in 0 blocks
==7367==    still reachable: 0 bytes in 0 blocks
==7367==         suppressed: 0 bytes in 0 blocks
==7367== Rerun with --leak-check=full to see details of leaked memory
==7367== 
==7367== For counts of detected and suppressed errors, rerun with: -v
==7367== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Here is the code. Its a simplified version of my program, but it still has the problem I am having.

#include <cstdlib>

class ItemSet {
    private:
        int* items;

    public:
        ItemSet();
        virtual ~ItemSet();

        // ItemSet Methods...
};

class ShoppingCart {
    private:
        ItemSet items_in_cart;
    public:
        ShoppingCart();

        // ShoppingCart methods...
};

ItemSet::ItemSet() {
    // Constructor
    int max_num_of_items = 10;
    items = (int*)calloc(sizeof(int), max_num_of_items);
}

ItemSet::~ItemSet() {
    // Destructor
    if (NULL != items) {
        // Frees the dynamically allocated register files
        free(items);
    }
}

ShoppingCart::ShoppingCart() {
    // Constructor
    items_in_cart = ItemSet();
}

int main() {
    ShoppingCart cart = ShoppingCart();

    return 0;
}
superflux
  • 117
  • 2
  • 10
  • 3
    Possible duplicate of [What is The Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) – Weak to Enuma Elish Feb 12 '16 at 08:37
  • 2
    You are implementing a class that manages a resource, then you don't manage the resource properly, you don't implement the copy constructor and assignment. The error is not a leak, it's a double "free". – Niall Feb 12 '16 at 08:49
  • And this is why you should use `std::vector`, `std::array`, or some other sensible method of having a collection. – David Schwartz Feb 12 '16 at 08:54

1 Answers1

1
ShoppingCart::ShoppingCart() {
    // Constructor
    items_in_cart = ItemSet();
}

That last line can't possibly work. Right after the assignment, we have two ItemSet objects with the same value for items -- the temporary created on the right side and items_in_cart. When the second one is destroyed, you have a double free.

Use a sensible method of having a collection of objects like std::list, std::vector, or std::array. Otherwise, follow the rule of 3, rule of 5, or of zero.

Community
  • 1
  • 1
David Schwartz
  • 179,497
  • 17
  • 214
  • 278
  • You are right. There was no need to have items_in_cart = ItemSet(). That would create a second ItemSet object. – superflux Feb 12 '16 at 09:18
  • @superflux This is going to happen *any time* you assign an ItemSet object, or a ShoppingCart object. And that's something that is *really easy* to do accidentally. That is why the Rule of Three exists, and also one reason the built-in container classes exist. – user253751 Feb 12 '16 at 09:37