why is std::string causing a memory leak in a class even after deleting

Viewed 399

I am building a class that uses void* as a storage method. I am aware that it is not a good idea to use void*. But I don't know what value it will hold at compile time, so I thought it was the best solution. It works perfectly with everything, including C strings, but doesn't work with std::string. It causes a really bad memory leak. I've built a basic model of the class that has the same problem.

#include<iostream>
#include<string>

class CLASS {
public:
    void* data;

    CLASS(std::string str) {
        std::string* s = new std::string;
        *s = str;
        data = s;
    }

    ~CLASS() {
        delete data;
    }
};

int main() {
    std::string str = "hi";
        while (true)
        {
            CLASS val(str);
        }
}
3 Answers

delete data does not work. delete will call the destructor and then deallocate the storage for one object, but void cannot be destroyed nor does it have a size.

The destructor of std::string will release the memory it allocated, but it is not being called here. You at the very least need to store the destructor of the value held in data. std::any will handle this all for you.

However, consider restricting it to a few known types with a std::variant<T1, T2, T3, ...> instead.

If you allocate a string, cast the pointer to a void *, then delete that, you will be trying to delete a void thing, not a string thing. For example, any class that allocates secondary storage (above and beyond the actual object being created with new) will most likely run into trouble:

#include <iostream>

class Y {
public:
    Y() { std::cout << "y constructor\n"; }
    ~Y() { std::cout << "y destructor\n"; }
};

class X {
public:
    X() { std::cout << "x constructor\n";  y = new Y(); }
    ~X() { delete y; std::cout << "x destructor\n"; }
private:
    Y *y;
};

int main() {
    X *x = new X();
    delete (void*)x;
}

This code will "work" in that it's legal but it will not do what you expect:

x constructor
y constructor

And a decent compiler should warn you about it:

program.cpp: In function ‘int main()’:
program.cpp:19:19: warning: deleting ‘void*’ is undefined [-Wdelete-incomplete]
   19 |     delete (void*)x;
      |                   ^

You should be deleting the type you allocated, to ensure the correct destructor is used. In other words, getting rid of the cast in the code shown above (so that you delete the correct type) will work out better:

x constructor
y constructor
y destructor
x destructor

Modern C++ has type-safe types which will do the heavy lifting for you, such as variant or any (if your single class needs to store a variety of types decided at run-time), or templates (if it can be used for one type but any of a variety). You should investigate those as an alternative to void *.

When you delete a pointer, it must be the same type that new returned (or, a pointer to a base class, if it has a virtual destructor).

You can't delete a void* pointer and expect it to know what type to destruct. All type information has been lost when the void* is assigned to.

So, you will have to explicitly type-cast the void* pointer back to its original type (just as you would have to to access the contents of the data being pointed at).

Also, you need to be sure that you follow the Rule of 3/5/0 as well, to manage the pointer property during assignments and copy/move operations.

Try this instead:

#include <iostream>
#include <string>

class MyClass {
public:
    void* data;

    MyClass() {
        data = nullptr;
    }

    MyClass(const std::string &str) {
        data = new std::string(str);
    }

    MyClass(const MyClass &src) {
        data = new std::string(*static_cast<std::string*>(src.data));
    }

    MyClass(MyClass &&src) {
        data = src.data; src.data = nullptr;
    }

    ~MyClass() {
        delete static_cast<std::string*>(data);
    }

    MyClass& operator=(MyClass rhs) {
        MyClass tmp(std::move(rhs));
        std::swap(data, tmp.data);
        return *this;
    }
};

int main() {
    std::string str = "hi";
    while (true)
    {
        MyClass val1(str);
        MyClass val2(val1);
        MyClass val3(std::move(val2));
        MyClass val4;
        val4 = val3;
        val4 = std::move(val3);
    }
}

This gets a bit more complex when you start introducing more types for the void* to point at. Then you need a way to identify what type is actually being pointed at, eg:

#include <iostream>
#include <string>

enum MyClassType { mctNull, mctString, mctInteger, ... };

class MyClass {
public:
    void* data;
    MyClassType dataType;

    MyClass() {
        dataType = mctNull;
        data = nullptr;
    }

    MyClass(const std::string &value) {
        dataType = mctString;
        data = new std::string(value);
    }

    MyClass(int value) {
        dataType = mctInteger;
        data = new int(value);
    }

    ...

    MyClass(const MyClass &src) {
        dataType = src.dataType;
        switch (src.dataType) {
            case mctNull:
                data = nullptr;
                break;
            case mctString:
                data = new std::string(*static_cast<std::string*>(src.data));
                break;
            case mctInteger:
                data = new int(*static_cast<int*>(src.data));
                break;
            ...
        }
    }

    MyClass(MyClass &&src) {
        dataType = src.dataType; src.dataType = mctNull;
        data = src.data; src.data = nullptr;
    }

    ~MyClass() {
        switch (dataType) {
            case mctString:
                delete static_cast<std::string*>(data);
                break;
            case mctInteger:
                delete static_cast<int*>(data);
                break;
            ...
        }
    }

    MyClass& operator=(MyClass rhs) {
        MyClass tmp(std::move(rhs));
        std::swap(data, tmp.data);
        std::swap(dataType, tmp.dataType);
        return *this;
    }
};

int main() {
    std::string str = "hi";
    int i = 12345;
    int counter;

    while (true)
    {
        MyClass val1 = (counter++ % 2 == 0) ? MyClass(str) : MyClass(i);

        MyClass val2(val1);
        MyClass val3(std::move(val2));
        MyClass val4;
        val4 = val3;
        val4 = std::move(val3);
    }
}

Fortunately, modern C++ provides std::variant and std::any so you don't have to manage this stuff manually at all, eg:

#include <iostream>
#include <string>
#include <variant>

class MyClass {
public:
    std::variant<std::string, int> data;

    MyClass() = default;

    MyClass(const std::string &value) { data = value; }
    MyClass(int value) { data = value; }
    ...
};

int main() {
    std::string str = "hi";
    int i = 12345;
    int counter;

    while (true)
    {
        MyClass val1 = (counter++ % 2 == 0) ? MyClass(str) : MyClass(i);

        MyClass val2(val1);
        MyClass val3(std::move(val2));
        MyClass val4;
        val4 = val3;
        val4 = std::move(val3);
    }
}

You get all of the copy, move, and destruction logic for free, and it actually does the right thing for you.

Related