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)? [duplicate]

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.

Inspection

You must call the destructor before free. – Hadik

When you do a placement-new you must explicitly call the object destructor. Just like malloc doesn't construct objects, free doesn't destruct objects. – Spielman

A case for alignas, if ever I saw one. – Dishcloth

@Hadik yes thats the problem. Calling destructor before free solves the problem. Please write an answer, I will accept it the correct answer. – Inspection

This is Minimal Working Example -- You forgot #include <string> and #include <cstdlib> – Amara

@Amara It's an understandable oversight though - some (albeit not all) real-world compilers/libraries have <string> and <cstdlib> are included by <iostream> (the standard neither requires nor prevents it). – Coacervate

The other platform-dependent assumption is writing malloc and free instead of std::malloc and std::free. – Camber

I don't see any need for alignas, @PaulSanders, given that std::malloc() returns memory suitably aligned so that it may be assigned to a pointer to any type of object with a fundamental alignment requirement (or a null pointer, of course). – Camber

Just so you know, if you get the piece of memory from a C library, and that library passes the memory outside of the process, that std::string will be invalid (unless you have correct serialization routines). – Celia

@TobySpeight OK, let me try again. I don't like that cast. I think the code should read: void *v = malloc(sizeof(Person)); auto p = new(v)Person; – Dishcloth

Ah, I see what you were getting at, @PaulSanders. And that does make sense, given that the result of std::malloc() is not yet a Person*. It's not clear exactly how the library's allocator is used - if all Person objects need to be allocated by the library, that would be a case for defining Person::operator new() and Person::operator delete() to encapsulate the allocation appropriately. – Camber

As a side-note, given that you specifically say MSG1 is "really big", and it's a compile-time constant, I'd recommend making it a std::string_view (a view of the raw data stored in the executable's constants store) instead of std::string (which must be allocated and populated from the constant store at runtime). Just #include <string_view>, and if you use using namespace std::literals;, initialization simplifies to static constexpr auto MSG1="Something really big message"sv; (note sv suffix). Not a huge difference, but you'll halve memory overhead of MSG1 and speed startup. – Walli

You say you're bound to using malloc because you're using a C library, but at the same time you are also using a std::string which internally uses the standard C++ operator new() instead of malloc. I don't really understand how you end up in a situation where you are required to use malloc while std::string isn't. (This is not a critique, but genuine curiosity.) – Whiffletree

@Whiffletree I explictly never called malloc in my c++ code. I call a C function (from a lib) which internally uses malloc to provide some functionality. This C lib provides its custom freeing method when I wanted to release this memory. However when I free it that way, it was giving me error. But as other's have pointed out, that if I use placement new operator than I do need to call destructor manually before releasing memory. Which on doing so solves the issue. – Inspection

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);

Nipping

Discussion about comments in code are always a bit tricky, but for me, the only comment in the code I would expect is the reason for using placement-new with malloc (allocator, exercise...) – Aureus

It is. And it makes my comment completely moot. Good one. – Aureus

To make it really clear what's leaking: because the Person destructor isn't being called, Person::name isn't being destructed, so the memory allocated by std::string isn't being deallocated. That is: Person is getting freed, but any memory it points to isn't. – Couthie

If he didn't have any project constraints, couldn't he just use delete p;? – Magnetograph

@AnsonSavage without any constraints they'd probably not use dynamic allocation for something that is basically a std::string which already does manage dynamically allocated memory – Nipping

@AnsonSavage I think both valgrind and address sanitizer would give an error regarding the mismatch between malloc and operator delete if you tried to do that. – Spacetime

To be paranoid, ::new((void*)p)Person() I think avoids some pathological evil overload issues. – Handspike

@AnsonSavage: new can adds specific metadata before the allocated item, different (often an expansion of or replacement for) the metadata malloc is smuggling. delete relies on that metadata to do the cleanup properly. If new and malloc disagree on the exact metadata structure, then delete and free will as well, and terrible things will happen when you mix them. – Walli

Answer 2

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

p->~Person();

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

Hadik

Answer 3

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.

Autotomize

Answer 4

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.

Settles

Though I might be mistaken, I understand from the question that OP has malloc-obtained storage from a C API and must thus use placement-new to construct an object in it, i.e., OP is not responsible for the allocation. I don't think overriding operator new / delete would make sense in his application if I am correct. – Wexford

Note that operator new gets a size as input - so you should normally use that instead of computing sizeof(Person); it is e.g., used by any derived classes (I wouldn't expect any). – Singletary

@Wexford The minimal example does not illustrate the precise use case, but the example leads us to believe the OP is calling an API to obtain memory for an object, and then is releasing that object. These activities are precisely what overloaded new and delete can do for you. – Settles

@HansOlsson Fixed. When overloading new and delete, I usually assume the coder will likely perform the overload for every object, including derived. My code would normally assert the provided size matches the size of the class that is being allocated. – Settles

Answer 5

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.

Waggon

"STL" being some kind of synonym? Not the literal STL? – Excision

@PeterMortensen I cleaned up the wording on that (and which memory is leaking) slightly. – Waggon