[MU3] Fix 310158: Don't null terminate MIDI sequence track name messages #7003

drebelsky · 2020-12-02T21:21:59Z

Resolves: https://musescore.org/en/node/310158

This PR makes it so that the null terminator from a part's name is not copied.

I signed CLA
I made sure the code in the PR follows the coding rules
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code
I made sure the title of the PR reflects the core meaning of the issue you are solving
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
I made sure the commit message title starts with "fix #424242:" if there is a related issue
I created the test (mtest, vtest, script test) to verify the changes I made

Jojo-Schmitz · 2020-12-03T07:17:02Z

the mtests are not happy, which I guess is understandable, but needs fixing anyway?

I don't quite understand why the vtests failed, but can't imagine this to be your (PR's) fault.

drebelsky · 2020-12-04T06:36:11Z

It looks like the layout code for 3.x has been changed since I opened the PR, but since my commits don't change these, I believe they could be merged without problems. I could also pull in the 3.x branch if that makes more sense.

Jojo-Schmitz · 2020-12-04T07:06:21Z

As far as I can tell the build in CI is always done against the then latest commit.
But try a rebase to see if it helps here

Remove MIDI sequence track name null terminators

psmokotnin · 2020-12-05T13:02:58Z

audio/exports/exportmidi.cpp

@@ -90,7 +90,7 @@ void ExportMidi::writeHeader()
            Staff* staff  = cs->staff(staffIdx);

            QByteArray partName = staff->partName().toUtf8();
-            int len = partName.length() + 1;
+            int len = partName.length();


This line was correct. An increasing of the length must be here.

See Qt's documentation: https://doc.qt.io/qt-5/qbytearray.html#data

Since we're doing a memcpy instead of a strcpy, I don't think the + 1 is necessary.

psmokotnin · 2020-12-05T13:58:39Z

I would fix it here.

MuseScore/audio/midi/midifile.cpp

Lines 138 to 139 in 8c98ded

    
           putvl(event.len()); 
        
           write(event.edata(), event.len());

drebelsky · 2020-12-05T20:40:31Z

I feel like it makes sense to store the non-null-terminated string as the data for the event since it's the actual data. Fixing it in

MuseScore/audio/midi/midifile.cpp

Lines 138 to 139 in 8c98ded

    
           putvl(event.len()); 
        
           write(event.edata(), event.len());

probably works, but then we have to do a special check for a track name message. We could also change

MuseScore/audio/exports/exportmidi.cpp

Line 102 in 8c98ded

ev.setLen(len);

, but then the actual data length and nominal data lengths would be different.

psmokotnin · 2020-12-08T14:02:47Z

But creation of char string without a null termination is very crashable way.

MuseScore/audio/midi/event.h

Line 205 in 6e28741

    
           uchar* _edata { nullptr };           // always zero terminated (_data[_len] == 0; )

It's good to fix it where file are saved instead of creation of broken MIDI events. For an example, midi import adds null termination while reading a file.

Jojo-Schmitz · 2020-12-09T06:42:43Z

Please squash the commits git rebase -i HEAD~3.

psmokotnin · 2020-12-09T09:04:50Z

audio/midi/midifile.cpp

@@ -135,8 +135,14 @@ void MidiFile::writeEvent(const MidiEvent& event)
            case ME_META:
                  put(ME_META);
                  put(event.metaType());
-                  putvl(event.len());
-                  write(event.edata(), event.len());
+                  if (event.metaType() == META_TRACK_NAME) {


Why track name type only? What about the other text types (0x01 - 0x07)?

Because the issue is about track name only I guess, "Sequence Track Name MIDI messages wrongly null-terminated"

Obviously, we have to check out what the MIDI standard says here.

In https://www.midi.org/component/edocman/rp-001-v1-0-standard-midi-files-specification-96-1-4-pdf/fdocument?Itemid=9999, it looks like types 0x01–0x07 are text types, and https://www.midi.org/component/edocman/smf-device-name-and-program-name-meta-events/fdocument?Itemid=9999 adds 0x08 and 0x09. In

MuseScore/audio/midi/event.h

Lines 77 to 81 in 6e28741

META_TITLE = 0x10, // mscore extension

META_SUBTITLE = 0x11, // mscore extension

META_COMPOSER = 0x12, // mscore extension

META_TRANSLATOR = 0x13, // mscore extension

META_POET = 0x14, // mscore extension

, we define some "mscore extensions" which are also (I believe) text types. Should we null terminate these extensions? Also, tangentially related, would it make sense if we changed the (nonstandard) extensions to be sent as System Exclusive messages or sequencer specific meta messages, instead?

AndroYD84 · 2022-01-25T09:33:21Z

This problem resurfaced in the current stable and nightly build.
If I export a midi score from MuseScore and run this simple Python script:

import os,sys
import muspy

midi = muspy.read_midi('midi_file.mid')
print([track.name for track in midi.tracks])

It outputs something like ['Piano\x00', 'Strings\x00'] instead of ['Piano', 'Strings'], happens only if the MIDI file was exported from MuseScore.

Jojo-Schmitz · 2022-01-25T09:34:39Z

Of course it does, the changes from this PR haven't been merged into 3.x nor been ported/merged to master.

The logs for the failing tests (vtests and mtest) have expired, are no longer available (as have the artifacts, so no way to test using those), so impossible to check what went wrong there, and so equally impossible to tell why they went wrong.

Please squash the commits, git rebase -i HEAD~4 and then git push -f the result.

Also better rebase the entire thing on top of the master branch, as there won't be any further 3.x release.

The mtest failures are most probably due to the *-ref.mid files having been changed only after the 1st commit, which then later got reverted, and that after the 4th commit all text meta events are not null-terminated anymore.

Port of PR musescore#7003 to the master branch

Jojo-Schmitz · 2022-01-25T14:32:46Z

OK, I bit the bullet and ported it over to master (see PR #10374), was pretty easy and straight forward, esp. as none of the tests needed to get ported ;-)

Port of PR #7003 to the master branch

Jojo-Schmitz · 2022-01-25T16:20:53Z

I'd like to include this PR in my PR #9000, but can't seem to get the mtests to work, how would these *-ref.mid files need to get generated?

Port of PR musescore#7003 resp. backport of PR musescore#10374

Jojo-Schmitz · 2022-01-25T20:06:47Z

OK, I managed it, it is now part of PR #9000, As no further 3.x is planned and the changes are in master already too, I believe this PR here could/should get closed.

Port of PR musescore#7003 resp. backport of PR musescore#10374

RomanPudashkin · 2022-12-20T08:56:27Z

3.x is closed for any changes

Port of PR musescore#7003 resp. backport of PR musescore#10374

drebelsky added 2 commits December 4, 2020 02:37

Fix #310158: don't null terminate MIDI sequence track names

f496931

Updated MIDI reference files

6e28741

Remove MIDI sequence track name null terminators

drebelsky force-pushed the fix-310158 branch from 614aac8 to 6e28741 Compare December 4, 2020 08:38

Jojo-Schmitz mentioned this pull request Dec 4, 2020

MuseScore 3.6 Beta/RC/Release. PRs to be merged and issues to be fixed. #6887

Closed

psmokotnin suggested changes Dec 5, 2020

View reviewed changes

Don't include track name null terminator when writing

bf6c6d3

psmokotnin reviewed Dec 9, 2020

View reviewed changes

Don't null terminate text meta events

f155672

Jojo-Schmitz mentioned this pull request Jan 15, 2021

MuseScore 3.6.1. PRs to be merged and issues to be fixed. #7244

Closed

49 tasks

Jojo-Schmitz mentioned this pull request Jan 28, 2021

MuseScore 3.6.2. PRs to be merged and issues to be fixed. #7347

Closed

53 tasks

igorkorsukov changed the title ~~Fix 310158: Don't null terminate MIDI sequence track name messages~~ [MU3] Fix 310158: Don't null terminate MIDI sequence track name messages Feb 5, 2021

Jojo-Schmitz mentioned this pull request Feb 8, 2021

[MU3 Issue] MuseScore 3.6.3 or 3.7.0, PRs to be merged and issues to be fixed. #7449

Closed

Jojo-Schmitz mentioned this pull request Sep 4, 2021

[MU3] Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 #9000

Closed

Jojo-Schmitz mentioned this pull request Jan 25, 2022

[MU4] Fix #310158: Don't null terminate MIDI text meta events #10374

Merged

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jan 25, 2022

Fix #310158: Don't null terminate MIDI text meta events

7043405

Port of PR musescore#7003 to the master branch

vpereverzev pushed a commit that referenced this pull request Jan 25, 2022

Fix #310158: Don't null terminate MIDI text meta events

f74adf6

Port of PR #7003 to the master branch

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jan 25, 2022

Fix #310158: Don't null terminate MIDI text meta events

6db04ce

Port of PR musescore#7003 resp. backport of PR musescore#10374

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 4, 2022

Fix #310158: Don't null terminate MIDI text meta events

0fd9fa5

Port of PR musescore#7003 resp. backport of PR musescore#10374

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 5, 2022

Fix #310158: Don't null terminate MIDI text meta events

4fd9023

Port of PR musescore#7003 resp. backport of PR musescore#10374

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 7, 2022

Fix #310158: Don't null terminate MIDI text meta events

15d2e47

Port of PR musescore#7003 resp. backport of PR musescore#10374

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 24, 2022

Fix #310158: Don't null terminate MIDI text meta events

6cc2d0c

Port of PR musescore#7003 resp. backport of PR musescore#10374

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 12, 2022

Fix #310158: Don't null terminate MIDI text meta events

f39e81f

Port of PR musescore#7003 resp. backport of PR musescore#10374

RomanPudashkin closed this Dec 20, 2022

Jojo-Schmitz mentioned this pull request Mar 1, 2023

Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 Jojo-Schmitz/MuseScore#80

Closed

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Mar 5, 2023

Fix #310158: Don't null terminate MIDI text meta events

1767cf9

Port of PR musescore#7003 resp. backport of PR musescore#10374

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MU3] Fix 310158: Don't null terminate MIDI sequence track name messages #7003

[MU3] Fix 310158: Don't null terminate MIDI sequence track name messages #7003

drebelsky commented Dec 2, 2020

Jojo-Schmitz commented Dec 3, 2020 •

edited

Loading

drebelsky commented Dec 4, 2020

Jojo-Schmitz commented Dec 4, 2020

psmokotnin Dec 5, 2020

drebelsky Dec 5, 2020

psmokotnin commented Dec 5, 2020

drebelsky commented Dec 5, 2020

psmokotnin commented Dec 8, 2020

Jojo-Schmitz commented Dec 9, 2020

psmokotnin Dec 9, 2020

Jojo-Schmitz Dec 9, 2020 •

edited

Loading

psmokotnin Dec 9, 2020

drebelsky Dec 9, 2020

AndroYD84 commented Jan 25, 2022

Jojo-Schmitz commented Jan 25, 2022 •

edited

Loading

Jojo-Schmitz commented Jan 25, 2022 •

edited

Loading

Jojo-Schmitz commented Jan 25, 2022

Jojo-Schmitz commented Jan 25, 2022 •

edited

Loading

RomanPudashkin commented Dec 20, 2022

	META_TITLE = 0x10, // mscore extension
	META_SUBTITLE = 0x11, // mscore extension
	META_COMPOSER = 0x12, // mscore extension
	META_TRANSLATOR = 0x13, // mscore extension
	META_POET = 0x14, // mscore extension

[MU3] Fix 310158: Don't null terminate MIDI sequence track name messages #7003

[MU3] Fix 310158: Don't null terminate MIDI sequence track name messages #7003

Conversation

drebelsky commented Dec 2, 2020

Jojo-Schmitz commented Dec 3, 2020 • edited Loading

drebelsky commented Dec 4, 2020

Jojo-Schmitz commented Dec 4, 2020

psmokotnin Dec 5, 2020

Choose a reason for hiding this comment

drebelsky Dec 5, 2020

Choose a reason for hiding this comment

psmokotnin commented Dec 5, 2020

drebelsky commented Dec 5, 2020

psmokotnin commented Dec 8, 2020

Jojo-Schmitz commented Dec 9, 2020

psmokotnin Dec 9, 2020

Choose a reason for hiding this comment

Jojo-Schmitz Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

psmokotnin Dec 9, 2020

Choose a reason for hiding this comment

drebelsky Dec 9, 2020

Choose a reason for hiding this comment

AndroYD84 commented Jan 25, 2022

Jojo-Schmitz commented Jan 25, 2022 • edited Loading

Jojo-Schmitz commented Jan 25, 2022 • edited Loading

Jojo-Schmitz commented Jan 25, 2022

Jojo-Schmitz commented Jan 25, 2022 • edited Loading

RomanPudashkin commented Dec 20, 2022

Jojo-Schmitz commented Dec 3, 2020 •

edited

Loading

Jojo-Schmitz Dec 9, 2020 •

edited

Loading

Jojo-Schmitz commented Jan 25, 2022 •

edited

Loading

Jojo-Schmitz commented Jan 25, 2022 •

edited

Loading

Jojo-Schmitz commented Jan 25, 2022 •

edited

Loading