Valgrind claims there are too many frees in memory deallocation

Question

I am reading Stroustrup "the c++ programming language 4th edition" and reached the part where he talks about destructors.

I tried to follow Stroustrup example and checked if valgrind is happy, but it is not: it claims there are some errors.

I do not really understand its comments, or where the problem might be. What I do understand is that valgrind thinks there is an extra free, which I failed to detect.

Would very much appreciate experts help here. Thanks.

please note that I use try catch in this specific example but I don't think this is the problem... What is really important in this code is probably the constructors and destructors. However, I post the whole thing for completeness

#include <iostream>
using namespace std;
class Vector
{
public:
    Vector();
    Vector(int s);
    ~Vector();
    double &operator[](int i);
    int size();
    static int get_default_size();

private:
    double *elem;
    int sz;
    static const int default_size = 5;
};

Vector::Vector()
{
    elem = new double[default_size];
    sz = default_size;
    for (int i = 0; i < sz; ++i)
    { //initialize elem array's values
        elem[i] = i;
    }
}

Vector::Vector(int s)
//: elem{new double[s]}, sz{s} //a dangerous idea - what if s is negative?
{
    if (s < 0)
        throw length_error{""};
    elem = new double[s];
    sz = s;
    for (int i = 0; i < sz; ++i)
    { //initialize elem array's values
        elem[i] = 0;
    }
}

Vector::~Vector()
{
    cout << "destructor working now" << endl;
    delete[] elem;
}

double &Vector::operator[](int i)
{
    return elem[i];
}

int Vector::size()
{
    return sz;
}

int Vector::get_default_size()
{
    return default_size;
}

Vector test()
{
    try
    {
        Vector v(-27);
        return v;
    }
    catch (std::length_error)
    {
        cout << "negative length" << endl;
        cout << "length will get default size, " << Vector::get_default_size() << endl;
        Vector v = Vector();
        return v;
    }
}

int main()
{
    Vector v = test();
    for (int i = 0; i < v.size(); ++i)
        cout << v[i] << endl;
}

I expected valgrind to show 0 errors but this is the output I receive from it:

<pre>==14640== Memcheck, a memory error detector
==14640== Copyright (C) 2002-2017, and GNU GPL&apos;d, by Julian Seward et al.
==14640== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==14640== Command: ./a.out
==14640== 
negative length
length will get default size, 5
destructor working now
==14640== Invalid read of size 8
==14640==    at 0x10933D: main (vector_improved1.cpp:83)
==14640==  Address 0x5b7e190 is 0 bytes inside a block of size 40 free&apos;d
==14640==    at 0x4C3173B: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14640==    by 0x10913A: Vector::~Vector() (vector_improved1.cpp:45)
==14640==    by 0x109299: test() (vector_improved1.cpp:74)
==14640==    by 0x10930E: main (vector_improved1.cpp:81)
==14640==  Block was alloc&apos;d at
==14640==    at 0x4C3089F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14640==    by 0x108FBF: Vector::Vector() (vector_improved1.cpp:21)
==14640==    by 0x10927A: test() (vector_improved1.cpp:74)
==14640==    by 0x10930E: main (vector_improved1.cpp:81)
==14640== 
0
1
2
3
4
destructor working now
==14640== Invalid free() / delete / delete[] / realloc()
==14640==    at 0x4C3173B: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14640==    by 0x10913A: Vector::~Vector() (vector_improved1.cpp:45)
==14640==    by 0x10937B: main (vector_improved1.cpp:81)
==14640==  Address 0x5b7e190 is 0 bytes inside a block of size 40 free&apos;d
==14640==    at 0x4C3173B: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14640==    by 0x10913A: Vector::~Vector() (vector_improved1.cpp:45)
==14640==    by 0x109299: test() (vector_improved1.cpp:74)
==14640==    by 0x10930E: main (vector_improved1.cpp:81)
==14640==  Block was alloc&apos;d at
==14640==    at 0x4C3089F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14640==    by 0x108FBF: Vector::Vector() (vector_improved1.cpp:21)
==14640==    by 0x10927A: test() (vector_improved1.cpp:74)
==14640==    by 0x10930E: main (vector_improved1.cpp:81)
==14640== 
==14640== 
==14640== HEAP SUMMARY:
==14640==     in use at exit: 0 bytes in 0 blocks
==14640==   total heap usage: 4 allocs, 5 frees, 73,912 bytes allocated
==14640== 
==14640== All heap blocks were freed -- no leaks are possible
==14640== 
==14640== For counts of detected and suppressed errors, rerun with: -v
==14640== ERROR SUMMARY: 6 errors from 2 contexts (suppressed: 0 from 0)

Answer 1

To explain why this happens, have a look at this dumbed down version of your code:

#include <iostream>


class Vector {
public:
    Vector() {
        std::cout << "Vector constructor (this=" << std::hex << this << ")" << std::endl;
    }
    Vector(const Vector& other){
        std::cout << "Vector copy constructor (other=" << std::hex << (&other) << " this=" << this << ")" << std::endl;
    }
    Vector& operator=(const Vector& other){
        std::cout << "Vector assignment operator (other="  << std::hex << (&other) << " this=" << this << ")" << std::endl;
        return (*this);
    }
    ~Vector(){
        std::cout << "Vector destructor (this=" << this << ")" << std::endl;
    }
};

Vector test() {
    Vector o = Vector();
    return o;
}

int main() {
    Vector o = test();
    return 0;
}

And its output when run:

Vector constructor (this=0x7ffee4a777d0)
Vector copy constructor (other=0x7ffee4a777d0 this=0x7ffee4a777d8)
Vector destructor (this=0x7ffee4a777d0)
Vector copy constructor (other=0x7ffee4a777d8 this=0x7ffee4a77810)
Vector destructor (this=0x7ffee4a777d8)
Vector copy constructor (other=0x7ffee4a77810 this=0x7ffee4a77818)
Vector destructor (this=0x7ffee4a77810)
Vector destructor (this=0x7ffee4a77818)

As you can see, the vector initially creates in test() gets copied a few times through various instances (each created instance also gets destroyed). Whenever this happens in your code, the default copy constructor automatically generated by the compiler gets called.

The default copy constructor just copies each attribute from the source instance to the destination instance (including double *elem). The semantics are usually wrong when working with dynamically allocated memory, because multiple instances then 'own' the same underlying data. When the vector created first is destroyed, it frees elem, making the other instances pointing to dangling (invalid) memory.

In order to solve this, you need to make sure your vector implementation includes an assignment and copy constructor that actually allocate new memory, and copy the underlying data from the copied vector to the destination vector.