Fix memory leak in MIDI export #26415 #26798

tstibor · 2025-02-27T15:07:50Z

Replace new unsigned char allocations with std::vector and make sure that MidiEvent::setEData takes vector reference rather than raw pointer.

Resolves: #26415

Replace new unsigned char allocations with std::vector<unsigned char> and make sure that MidiEvent::setEData takes vector reference rather than raw pointer.

Replace new unsigned char allocations with QVector<unsigned char> and make sure that MidiEvent::setEData takes vector reference rather than raw pointer. Backport of musescore#26798

Replace new unsigned char allocations with std::vector<unsigned char> and make sure that MidiEvent::setEData takes vector reference rather than raw pointer. Backport of musescore#26798

Jojo-Schmitz · 2025-02-27T18:54:25Z

src/importexport/midi/internal/midishared/midifile.cpp

        if (dataLen) {
-            read(data, dataLen);
+            read(data.data(), dataLen);
        }
        data[dataLen] = 0;          // always terminate with zero so we get valid C++ strings


My backport to Mu3 crashes here under AddressSanitizer, any idea why?
Or why not here?

Shouldn't it rather be data[dataLen + 1] = 0;?

@Jojo-Schmitz thanks for checking there is indeed a problem.

0x00005555571ec727 in mu::iex::midi::MidiFile::readEvent (this=this@entry=0x7fffffff8a30, event=event@entry=0x7fffffff8670) at /musescore/src/importexport/midi/internal/midishared/midifile.cpp:537 537 { (gdb) n 541 if (nclick == -1) { (gdb) 545 click += nclick; (gdb) 549 LOGD("MIDI: Unknown Message 0x%02x", me & 0xff); (gdb) 547 read(&me, 1); (gdb) 548 if (me >= 0xf1 && me <= 0xfe && me != 0xf7) { (gdb) 99 _Vector_impl_data() _GLIBCXX_NOEXCEPT (gdb) 555 std::vector<unsigned char> data; (gdb) 558 if (me == 0xf0 || me == 0xf7) { (gdb) 555 std::vector<unsigned char> data; (gdb) 580 if (me == ME_META) { (gdb) 581 status = -1; // no running status (gdb) 583 read(&type, 1); (gdb) 584 dataLen = getvl(); // read len (gdb) 585 if (dataLen == -1) { (gdb) p dataLen $1 = 8 (gdb) n 589 if (dataLen) { (gdb) 590 read(data.data(), dataLen); (gdb) p data $2 = std::vector of length 0, capacity 0 (gdb) p data ... (gdb) c Continuing. [Thread 0x7fff86bfd6c0 (LWP 230282) exited] [Thread 0x7fff9f1fe6c0 (LWP 230276) exited] [Thread 0x7fffb0e786c0 (LWP 230271) exited] Thread 1 "main" received signal SIGSEGV, Segmentation fault. mu::iex::midi::MidiFile::readEvent (this=this@entry=0x7fffffff8a30, event=event@entry=0x7fffffff8670) at /usr/include/c++/12/bits/stl_vector.h:1121 1121 operator[](size_type __n) _GLIBCXX_NOEXCEPT

Shouldn't it rather be data[dataLen + 1] = 0;?

If a vector has length dataLen + 1, then dataLen is the index of the final position and dataLen + 1 is beyond the end of the vector. So data[dataLen] = 0; was correct, but the problem seems to be that the vector has not length dataLen + 1 but length 0.

Anyway, as said above, I doubt whether the null termination, i.e. data[dataLen] = 0;, is still necessary.

cbjeukendrup · 2025-02-28T13:59:12Z

src/importexport/midi/internal/midishared/midifile.cpp

@@ -562,9 +562,8 @@ bool MidiFile::readEvent(MidiEvent* event)
            LOGD("readEvent: error 3");
            return false;
        }
-        data    = new unsigned char[len + 1];


Maybe this needs to be replaced with data.resize(len + 1)? (And the same below)

I doubt whether the null termination is still necessary now that we use std::vector. So perhaps data.resize(len) is enough, and data[dataLen] = 0; can be removed.

cbjeukendrup · 2025-02-28T14:12:15Z

src/importexport/midi/internal/midishared/midievent.h

-    const uchar* edata() const { return _edata; }
-    void setEData(uchar* d) { _edata = d; }
+    const uchar* edata() const { return _edata.data(); }
+    void setEData(std::vector<uchar>& d) { _edata = d; }


To eliminate the copying that happens in the = operator, you could let it take an rvalue reference:

Suggested change

void setEData(std::vector<uchar>& d) { _edata = d; }

void setEData(std::vector<uchar>&& d) { _edata = d; }

so that move assignment will be performed instead of copy assignment.

Then you will need to call it like event.setEData(std::move(data)).

Jojo-Schmitz · 2025-02-28T15:37:09Z

src/importexport/midi/internal/midishared/midifile.cpp

@@ -587,9 +586,8 @@ bool MidiFile::readEvent(MidiEvent* event)
            LOGD("readEvent: error 6");


Shouldn't/Couldn't the above if be an else if?

if (me == 0xf0 || me == 0xf7) { ... } else if (me == ME_META) { ... } if (me & 0x80) { // status byte

@Jojo-Schmitz I think you are right and that should not introduce any issues.

if (me == 0xf0 || me == 0xf7) { // SysEx handling ... return .. } else if (me == ME_META) { // Meta handling ... return ... } if (me & 0x80) { ... }

It seems to be a small optimization

Make sure the vector size is properly resized before read operation and remove null termination. Avoid copy assignment by using move semantics.

Replace new unsigned char allocations with std::vector<unsigned char> and make sure that MidiEvent::setEData takes vector reference rather than raw pointer. Backport of musescore#26798

Jojo-Schmitz · 2025-02-28T17:18:09Z

I'm puzzled about the vtest failure:

Seems entirely unrelated, maybe a rebase helps?

Fix memory leak in MIDI export musescore#26415

5f0cf0b

Replace new unsigned char allocations with std::vector<unsigned char> and make sure that MidiEvent::setEData takes vector reference rather than raw pointer.

Jojo-Schmitz mentioned this pull request Feb 27, 2025

Fix GH#26414: Memory leak in MIDI export Jojo-Schmitz/MuseScore#823

Open

Jojo-Schmitz reviewed Feb 27, 2025

View reviewed changes

cbjeukendrup reviewed Feb 28, 2025

View reviewed changes

Jojo-Schmitz reviewed Feb 28, 2025

View reviewed changes

Fix segmentation fault and use move semantics

04fdc89

Make sure the vector size is properly resized before read operation and remove null termination. Avoid copy assignment by using move semantics.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak in MIDI export #26415 #26798

Fix memory leak in MIDI export #26415 #26798

tstibor commented Feb 27, 2025

Jojo-Schmitz Feb 27, 2025 •

edited

Loading

Jojo-Schmitz Feb 27, 2025 •

edited

Loading

tstibor Feb 28, 2025

cbjeukendrup Feb 28, 2025

cbjeukendrup Feb 28, 2025

cbjeukendrup Feb 28, 2025

Jojo-Schmitz Feb 28, 2025 •

edited

Loading

tstibor Feb 28, 2025

Jojo-Schmitz Feb 28, 2025

Jojo-Schmitz commented Feb 28, 2025 •

edited

Loading

	void setEData(std::vector<uchar>& d) { _edata = d; }
	void setEData(std::vector<uchar>&& d) { _edata = d; }

		@@ -587,9 +586,8 @@ bool MidiFile::readEvent(MidiEvent* event)
		LOGD("readEvent: error 6");

Fix memory leak in MIDI export #26415 #26798

Are you sure you want to change the base?

Fix memory leak in MIDI export #26415 #26798

Conversation

tstibor commented Feb 27, 2025

Jojo-Schmitz Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

tstibor Feb 28, 2025

Choose a reason for hiding this comment

cbjeukendrup Feb 28, 2025

Choose a reason for hiding this comment

cbjeukendrup Feb 28, 2025

Choose a reason for hiding this comment

cbjeukendrup Feb 28, 2025

Choose a reason for hiding this comment

Jojo-Schmitz Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

tstibor Feb 28, 2025

Choose a reason for hiding this comment

Jojo-Schmitz Feb 28, 2025

Choose a reason for hiding this comment

Jojo-Schmitz commented Feb 28, 2025 • edited Loading

Jojo-Schmitz Feb 27, 2025 •

edited

Loading

Jojo-Schmitz Feb 27, 2025 •

edited

Loading

Jojo-Schmitz Feb 28, 2025 •

edited

Loading

Jojo-Schmitz commented Feb 28, 2025 •

edited

Loading