How to avoid this memory leak?

Question

This is my code:

void MIDITest::CreateNoteBlock() {
    IMidiMsgExt* midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(57, 100, 0, 0, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(60, 100, 0, tickSize * 38, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(62, 100, 0, 0, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(65, 100, 0, tickSize * 32, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(57, 0, tickSize * 111, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(60, 0, tickSize * 111, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(62, 0, tickSize * 75, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(65, 0, tickSize * 105, 0);
    queuedNotes.insert(*midiMessage);
}

so at every new operator it will allocate a block of memory.

Should I use free after any insert inside the queuedNotes? Or it will be released after void function return? (i.e. the brackets of CreateNoteBlock).

Or I can "reuse" each time the midiMessage pointer for a new IMidiMsgExt?

@Chiel this is `multiset queuedNotes;`. That's the code: it is called within the CTOR of a class. — markzzz, Apr 21 '16 at 12:44
What about using 'IMidiMsgExt midiMessage;' instead of 'IMidiMsgExt* midiMessage = new IMidiMsgExt'? — Tom Deseyn, Apr 21 '16 at 12:46
Doesn't matter, really, *midiMessage is passing by value, that is, a copy is made. queuedNotes cannot take ownership of the pointer. So yes, you'll have to delete for every new. — Adrian Roman, Apr 21 '16 at 12:46
You should **never** use `free` on something that was created with `new`. `free` is used together with `malloc`; for `new` you have to use `delete`. Don't mix them, ever! Mixing them is like renting a car from Avis and giving it back to Hertz, it's simply wrong! And in C++ you shouldn't use `malloc` and `free`, only `new` and `delete` (leaving aside smart pointers), because you don't just want to allocate/reclaim memory, you also want the constructors/destructors to run. Please check http://stackoverflow.com/questions/240212/what-is-the-difference-between-new-delete-and-malloc-free — Fabio says Reinstate Monica, Apr 21 '16 at 13:03
@AdrianRoman. Only true when a proper deepcopy is implemented for `IMidiMsgExt`. Otherwise it is a choice between undefined behavior or a memory leak. — Chiel, Apr 21 '16 at 13:32

NathanOliver · Accepted Answer · 2016-04-21T13:11:47.760

6

The answer is not to use new at all. Create a object with automatic storage duration

IMidiMsgExt midiMessage;

And then you can keep calling MakemidiMessageMsg and insert a copy of the message into the multiset.

midiMessage.MakemidiMessageMsg(57, 100, 0, 0, 0);
queuedNotes.insert(midiMessage);
midiMessage.MakemidiMessageMsg(60, 100, 0, tickSize * 38, 0);
queuedNotes.insert(midiMessage);
//...

Now the multiset has a copy of all of the messages and at the end of the function midiMessage is destroyed and no memory management needs to be done.

If IMidiMsgExt has a constructor that is like MakemidiMessageMsg where you can construct a complete message then you could boild this down even further and use something like

queuedNotes.insert(IMidiMsgExt(57, 100, 0, 0, 0));
queuedNotes.insert(IMidiMsgExt(60, 100, 0, tickSize * 38, 0));

And now we don't even need midiMessage.

edited Apr 21 '16 at 13:11

answered Apr 21 '16 at 12:48

NathanOliver

171,901
28
288
402

But at the end of the function won't "all" midiMessage (created by using new) destroyed as well? – markzzz Apr 21 '16 at 12:51
@paizza No. If you call `new` the only way to free the memory is to call `delete`. The pointer is destroyed but the memory will not be automatically returned. – NathanOliver Apr 21 '16 at 12:53
It's due to automatic storage duration right? So your suggestion is about `automatic storage duration`, `new` about `dynamic storage duration` http://stackoverflow.com/questions/6337294/creating-an-object-with-or-without-new – markzzz Apr 21 '16 at 13:00
Can I create a `automatic storage duration` using pointer? Or that's valid only using object itself and not pointer? – markzzz Apr 21 '16 at 13:01
@paizza You can but those are called smart pointers and there is really not a need for them from what I see here, If all you want to do is create messages and and add them to the multiset then pretend that pointers don't exist. Pointers have there uses but in C++ if can use an object with automatic storage duration you should. It is almost like 0 cost garbage collection. – NathanOliver Apr 21 '16 at 13:03
Ok. Not sure why I should use pointer or object so. It's a guess for the moment. For what I see, many methods use pointer instead of passing directly the object. So i need to use `&var` every time instead of create pointer and just pass `var`. – markzzz Apr 21 '16 at 13:06
It could be that what you are seeing is a C interface. In C the only way to pass an object to a function without making a copy was to pass by pointer. In C++ we have references which abstracts that away and allows us to pass without copying and gets rid of having to use `&variable_name` when calling the function. – NathanOliver Apr 21 '16 at 13:10
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/109821/discussion-between-paizza-and-nathanoliver). – markzzz Apr 21 '16 at 13:42

score 5 · Answer 2 · answered Apr 21 '16 at 12:48

Do you come from a Java or C# background? In C++ you don't have to use new to create objects, just declaring them will work fine:

IMidiMsgExt midiMessage;
midiMessage.MakemidiMessageMsg(57, 100, 0, 0, 0);
queuedNotes.insert(midiMessage);

It's either that (which is my recommended solution), or you have to explicitly free the objects:

IMidiMsgExt* midiMessage = new IMidiMsgExt;
midiMessage->MakemidiMessageMsg(57, 100, 0, 0, 0);
queuedNotes.insert(*midiMessage);
delete miniMessage;

score 1 · Answer 3 · answered Apr 21 '16 at 12:51

It seem redundant to dynamically allocate IMidiMsgExt using new. You can just allocate it directly on the stack (no pointers), and then it will be destroyed when your method returns. I.e.:

void MIDITest::CreateNoteBlock() {
    IMidiMsgExt midiMessage();
    midiMessage.MakemidiMessageMsg(57, 100, 0, 0, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(60, 100, 0, tickSize * 38, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(62, 100, 0, 0, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(65, 100, 0, tickSize * 32, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(57, 0, tickSize * 111, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(60, 0, tickSize * 111, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(62, 0, tickSize * 75, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(65, 0, tickSize * 105, 0);
    queuedNotes.insert(midiMessage);
}

score 1 · Answer 4 · answered Apr 21 '16 at 13:37

Try to make your API more C++ like.

Use one object on the stack instead of creating a lot of new ones in the heap.

void MIDITest::CreateNoteBlock() {
    IMidiMsgExt midiMessage;

    midiMessage.MakemidiMessageMsg(57, 100, 0, 0, 0);
    queuedNotes.insert(midiMessage);

    midiMessage.MakemidiMessageMsg(60, 100, 0, tickSize * 38, 0);
    queuedNotes.insert(midiMessage);

    // ...
}

Initialize your objects in the constructor. Define an operator = (const IMidiMsgExt&) for IMidiMsgExt.

void MIDITest::CreateNoteBlock() {
    IMidiMsgExt midiMessage(57, 100, 0, 0, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt(60, 100, 0, tickSize * 38, 0);
    queuedNotes.insert(midiMessage);

    // ...
}

I guess, that insert() expects a const IMidiMsgExt&. So you can directly pass freshly initialized objects:

void MIDITest::CreateNoteBlock() {
    queuedNotes.insert({57, 100, 0, 0, 0});
    queuedNotes.insert({60, 100, 0, tickSize * 38, 0});

    // ...
}

BTW: you should prefer to use e.g. std::queue<> for queuedNotes. Then you'll not use insert(), but push(), or emplace(). The advantage of emplace() is, that it constructs the object in the container instead of first creating it and then copying it to the container:

void MIDITest::CreateNoteBlock() {
    queuedNotes.emplace(57, 100, 0, 0, 0);
    queuedNotes.emplace(60, 100, 0, tickSize * 38, 0);

    // ...
}

Also your typename IMidiMsgExt signals to me, that you are trying to mimic the C# thinking in C++. It is possible, but usually it is not the preferred solution. From your question I don't know enough about your class tree and the underlying requirements to provide a suggestion, but in C++ that usually is a code smell.

How to avoid this memory leak?

4 Answers4