Why is there a memory leak in this program and how can I solve it, given the constraints (using malloc and free for objects containing std::string)?

Question

This is a minimal working example for the problem I am facing in my real code.

#include <iostream>

namespace Test1 {
    static const std::string MSG1="Something really big message";
}

struct Person{
    std::string name;
};

int main() {
    auto p = (Person*)malloc(sizeof(Person));
    p = new(p)Person();
    p->name=Test1::MSG1;

    std::cout << "name: "<< p->name << std::endl;

    free(p);

    std::cout << "done" << std::endl;

    return 0;
}

When I compile it and run it via Valgrind, it gives me this error:

definitely lost: 31 bytes in 1 blocks

---

Constraints

1. I am bound to use malloc in the example above, as in my real code I use a C library in my C++ project, which uses this malloc internally. So I can't get away from malloc usage, as I don't do it explicitly anywhere in my code.
2. I need to reassign std::string name of Person again and again in my code.

Answer 1

The important pieces of your code line by line...

Allocate memory for one Person object:

auto p = (Person*)malloc(sizeof(Person));

Construct a Person object in that already allocated memory via calling its constructor:

p = new(p)Person();

Free the memory allocated via malloc:

free(p);

Calling the constructor via placement new creates a std::string. That string would be destroyed in the destructor but the destructor is never called. free does not call destructors (just like malloc does not call a constructor).

malloc only allocates the memory. Placement new only constructs the object in already allocated memory. Hence you need to call the destructor before calling free. This is the only case I am aware of where it is correct and necessary to explicitly call a destructor:

auto p = (Person*)malloc(sizeof(Person));
p = new(p)Person();
p->~Person();
free(p);

Answer 2

You must manually call the destructor before free(p);:

p->~Person();

Or std::destroy_at(p), which is the same thing.

Answer 3

As mentioned in other answers, the source of the leak is that the destructor for the name member of Person does not get called. It would normally be called implicitly when the destructor for Person is called. However, Person is never destructed. The memory for the Person instance is simply released with free.

So, just as you had to explicitly invoke the constructor with the placement new after malloc, you also need to explicitly invoke the destructor before free.

You can also consider overloading the new and delete operators.

struct Person {
    std::string name;
    void * operator new (std::size_t sz) { return std::malloc(sz); }
    void operator delete (void *p) { std::free(p); }
};

This way, you can use new and delete normally, when underneath they will use malloc and free.

int main (void) {
    auto p = new Person;
    //... 
    delete p;
}

And this way, you can more naturally use a smart pointer.

int main (void) {
    auto p = std:make_unique<Person>();
    //... unique pointer will delete automatically
}

Of course, you could have used unique_ptr with a custom deleter with your explicit calls to malloc and free, but it would have been much more cumbersome, and your deleter would still need to know to explicitly invoke the destructor as well.

Answer 4

As others have mentioned, dynamic memory allocated by the members of Person only gets freed by the destructor ~Person, which free() does not call.

If you have to use this function with a library that requires some initialization and clean-up other than the default, such as here, one approach is to define a new deleter, for the standard libray smart pointers to use: This will work even with a block of memory you did not allocate yourself.

#include <memory>
#include <new> // std::bad_alloc
#include <stdlib.h>
#include <string>

struct Person{
    std::string name;
};

struct PersonDeleterForSomeLib {
  constexpr void operator()(Person* ptr) const noexcept {
    ptr->~Person();
    free(ptr);
  }
};


Person* Person_factory() // Dummy for the foreign code.
{
  Person* const p = static_cast<Person*>(malloc(sizeof(Person)));
  if (!p) {
    throw std::bad_alloc();
  }
  new(p) Person();
  return p;
}

This lets you safely use:

const auto p =
  std::unique_ptr<Person, PersonDeleterForSomeLib>(Person_factory());

with automatic memory management. You can return the smart pointer from the function, and both the destructor and free() will be called when its lifetime ends. You can also create a std::shared_ptr this way. If for some reason you need to destroy the object while the smart pointer is still alive, you can reset or release it.

Answer 5

Pinpointing the problem

First of all, let us be clear about exactly what the problem is by illustrating the state of the memory after each statement.

int main() {
    auto p = (Person*)malloc(sizeof(Person));

    //  +---+    +-------+
    //  | p | -> | ~~~~~ |
    //  +---+    +-------+

    p = new(p)Person();

    //  +---+    +-------+
    //  | p | -> | name  |
    //  +---+    +-------+

    p->name=Test1::MSG1;

    //  +---+    +-------+    +---...
    //  | p | -> | name  | -> |Something...
    //  +---+    +-------+    +---...

    free(p);

    //  +---+                 +---...
    //  | p |                 |Something...
    //  +---+                 +---...

    return 0;
}

As you can see, calling free(p) freed up the memory originally allocated by malloc, but it didn't free the memory allocated by p->name when it was assigned to.

This is your leak.

Solving the problem

There are two aspects to having a Person object on the heap:

• A memory allocation—handled by malloc/free here.
• Initializing and Finalizing that memory—handled by calls to constructors and destructors.

You're lacking the call to the destructor, hence resources held by Person are leaked. Here it's memory, but if Person held a lock you could have a forever locked mutex, etc... executing the destructor is therefore necessary.

The C-style approach is to call the destructor yourself:

int main() {
    auto p = (Person*)malloc(sizeof(Person));
    p = new(p) Person();
    p->name = Test1::MSG1;

    std::cout << "name: "<< p->name << "\n";

    //  Problem "fixed".
    p->~Person();

    free(p);

    std::cout << "done" << "\n";

    return 0;
}

However this is not idiomatic C++: it's error prone, etc...

The C++ approach is to use RAII to ensure that when p goes out of scope, all its resources are properly disposed of: the destructor of Person is executed and the memory allocated for Person itself is freed.

First of all, we're going to create some helpers. I used the c namespace since I don't know the name of the C library you use, but I invite you to be more specific:

namespace c {
struct Disposer<T> {
    void operator()(T* p) {
        p->~T();
        free(p);
    }
};

template <typename T>
using UniquePointer<T> = std::unique_ptr<T, Disposer<T>>;

template <typename T, typename... Args>
UniquePointer<T> make_unique(T* t, Args&&... args) {
    try {
        new (t) T(std::forward<Args>(args)...);
    } catch(...) {
        free(t);
        throw;
    }

    return UniquePointer{t};
}
} // namespace c

And with that, we can improve the original example:

int main() {
    auto raw = (Person*) malloc(sizeof(Person));

    auto p = c::make_unique(raw);

    p->name = Test1::MSG1;

    std::cout << "name: "<< p->name << "\n";

    //  No need to call the destructor or free ourselves, welcome to RAII.

    std::cout << "done" << "\n";

    return 0;
}

Note: Do not use std::endl, use '\n' or "\n" instead. std::endl calls .flush() on top of putting an end of line, which is rarely what you want -- it slows things down.