Memory leak in MidiFile::operator=

[no-more-secrets]

Hello, I am running the midifile library under ASAN (Address Sanitizer) and it is telling me that there are some memory leaks.

One of them appears to be caused by using MidiFile move assignment operator under certain circumstances. On the first line of that function there appears to be a problem:

m_events = std::move(other.m_events);

but the problem is, the pointers stored in m_events (before the move) won't be freed, and you have a leak. The vector's destructor will be called, but it won't free raw pointers. It appears that the issue goes away when I insert these lines at the top of the function:

    clear();
    if (m_events[0] != NULL) {
        delete m_events[0];
        m_events[0] = NULL;
    }
    ...

Could you please fix this? By the way, I also had a look in the MidiFile copy assignment operator and it also looks dubious, in particular you are reserving space in the m_events array and then adding new events onto the end of it, when you should probably be freeing the events in that vector, then resizing it to zero, then reserving, then finally adding new elements (note that calling reserve will not change the size of the vector).

Overall, I would really recommend avoiding to use raw pointers that own memory; the fact that you are using naked new and delete statements throughout is a red flag... I am thus honestly not surprised to find these memory leaks. If you have a pointer that owns memory the default choice is std::unique_ptr which will handle memory management for you so that you never have to worry about calling delete at the right time.

Thanks
David
P.S. I have a pull request pending about restructuring the include folders in this repository, can you please respond? :-)

[nandlab]

Hello,
I am planning on using this library, was this issue fixed?

[wherefree]

您好，您所发送的邮件我已收到。

[no-more-secrets]

I don't think I've heard anything from the maintainer since I posted this issue a couple of years ago. On looking at my code though, it looks like I just employed a workaround where I avoid writing any code that causes the smf::MidiFile move-assignment operator to be called, and that seems to get rid of the leaks, at least as far as ASan is concerned.

[nandlab]

I looked into the code, and the old m_events resources are indeed not freed. As @dpacbach said, you can probably get away with not calling the move assignment, but I think this memory leak should be fixed.
It seems generally like a bad idea to store raw pointers in a vector. You can at least use std::unique_ptr<MidiEventList>, so that the allocated MidiEventList gets freed automatically.