[MU3] fix #89216: multiple possible causes of crashes or audible artefacts

[mirabilos]

Resolves: https://bugs.debian.org/985129 and https://musescore.org/en/node/89216 and probably others

• Track sample name so we can issue proper warning messages, show filename
• On read errors, issue an error message and mark sample as invalid
• Mark sample as invalid if Ogg Vorbis decompression (SF3) fails
• Do all sanity checks on {,loop}{start,end} with SF2 semantics for end;
  only switch end to point to the last sample afterwards in only one place
• Add sanity check provided by the SoundFont spec as extra warning
• Do not crash if there is no data[]
• Issue diagnostics if disabling a sample
• Swap two members to improve structure packing/alignment while there
• Use unsigned integers for SoundFont element sizes properly

• 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

I’ve tested this patch so far against MuseScore 2.3.2 and am currently testing it against 3.2.3 as well. I’ve added + collect_artifacts so @Jojo-Schmitz can test 3.x HEAD ☺

With this patch enabled, multiple issues in MuseScore_General_HQ are found (and also in the nōn-HQ soundfont). I’ve already sent @mrbumpy409 an eMail regarding these, so you can ignore them.

This change basically reverts commit 5aea0ab as I found the root causes for that. I did not do a full revert because I’m unsure if the log.h stuff is used elsewhere; if not, please feel free to g/c that as well.

[Jojo-Schmitz]

Just FYI: that + collect_artifacts is no longer needed.

What has commit 5aea0ab got to do with it? I don't see anything in your PR that reverts anything from that, except for the last chunk, the changes to audio/midi/fluid/voice.cpp

[mirabilos]

commit 5aea0ab was “try fixed crash on sample optimize”, which basically is the two ifs in fluid/voice.cpp; the rest was just adding those macros.

As I wrote, if these macros are now unused they can be g/c’d, but the commit was about, and this reverts, these two ifs.

[mirabilos]

I’m in discussion with @mrbumpy409 (regarding how the MuseScore_General soundfont relates to these) and have feedback from FluidSynth upstream and am especially looking at FluidSynth’s current set of start/end/loopstart/loopend checks:

https://github.com/FluidSynth/fluidsynth/blob/b8fb6c81e1ca27c0bba2f6a0168832214f91d497/src/sfloader/fluid_sfont.c#L773..L832

Considering both, I’ll probably change this:

• demote the qWarning about SoundFont spec noncompliance (regarding the 8/32/8 minimum sizes) into a qDebug
• make loopstart == start valid (need to test this, of course)
• make loopend == loopstart to avoid looping valid (MuseScore_General apparently has samples that “are not supposed to be looped”, which the SoundFont spec does not even consider)

This is more conservative than doing what FluidSynth does, but FluidSynth has tons of other changes which may or may not impact this, so I’d prefer that.

Comments?

[Jojo-Schmitz]

I'd say: go for it.

[Jojo-Schmitz]

Not sure whether it is related, but MuseScore 4 gives a few errors on the MuseScore General.sf3 soundfont:

10:34:53.965 | ERROR | 2 | FluidSynth | init: requested set value for 'synth.sample-rate' out of range
10:34:53.965 | ERROR | 2 | FluidSynth | init: requested set value for 'synth.sample-rate' out of range

[Jojo-Schmitz]

I've played with this PR and found out that it crashes when on a percussoin staff switching to Note Input mode !?!

[mirabilos]

Joachim Schmitz dixit:

I've played wiht this PR and found out that it crashes when on a drum staff switching to Note Input mode !?!

Oh, ouch. That we can’t have, of course. I’ll try to reproduce this, will look at it (when doing the other discussed changes). Thanks, //mirabilos -- <ch> you introduced a merge commit │<mika> % g rebase -i HEAD^^ <mika> sorry, no idea and rebasing just fscked │<mika> Segmentation <ch> should have cloned into a clean repo │ fault (core dumped) <ch> if I rebase that now, it's really ugh │<mika:#grml> wuahhhhhh

[Jojo-Schmitz]

Unless I screwed up, that is. Reproducing should be easy: new score, add Congas, select first rest. press N, KABOOM (after a while, several seconds).
But none of the files you modified mentioned in the stack trace, very strange.

Stack trace:
1 std::_Rb_tree<int, std::pair<int const, Ms::StaffType>, std::_Select1st<std::pair<int const, Ms::StaffType>>, std::less, std::allocator<std::pair<int const, Ms::StaffType>>>::empty stl_tree.h 1009 0x10ad72c
2 std::map<int, Ms::StaffType>::empty stl_map.h 464 0x1095988
3 Ms::StaffTypeList::staffType stafftypelist.cpp 29 0xbbad4e
4 Ms::Staff::staffType staff.cpp 1012 0xaa8afa
5 Ms::Staff::isTabStaff staff.cpp 1518 0xaaaf26
6 Ms::Chord::add chord.cpp 521 0x9347d5
7 Ms::DrumTools::updateDrumset drumtools.cpp 155 0x4a2814
8 Ms::DrumTools::setDrumset drumtools.cpp 175 0x4a2a42
9 Ms::MuseScore::showDrumTools musescore.cpp 6827 0x4346e4
10 Ms::MuseScore::changeState musescore.cpp 4456 0x424d5b
11 Ms::MuseScore::updateInputState keyb.cpp 92 0x4d7c62
12 Ms::ScoreView::startNoteEntry scoreview.cpp 3238 0x64aa0d
13 Ms::ScoreView::changeState events.cpp 1186 0x67511e
14 Ms::ScoreView::<lambda(Ms::ScoreView *, const QByteArray&)>::operator()(Ms::ScoreView *, const QByteArray &) const scoreview.cpp 2112 0x63fea2
15 std::_Function_handler<void(Ms::ScoreView *, const QByteArray&), Ms::ScoreView::cmd(const char *)::<lambda(Ms::ScoreView *, const QByteArray&)>>::_M_invoke(const std::_Any_data &, Ms::ScoreView *&&, const QByteArray &) std_function.h 297 0x65ac7a
16 std::function<void (Ms::ScoreView *, QByteArray const&)>::operator()(Ms::ScoreView *, QByteArray const&) const std_function.h 687 0x10afbad
17 Ms::ScoreView::cmd scoreview.cpp 2890 0x648719
18 Ms::ScoreView::cmd scoreview.cpp 2089 0x63fdff
19 Ms::MuseScore::cmd musescore.cpp 6601 0x4338b4
20 Ms::MuseScore::cmd musescore.cpp 6032 0x43024a
...

Does look like some kind of stack corruption to me.

The CI artifact (for Windows) does not crash ... Might a MinGW issue, or a Qt 5.15.2 one? Or Debug vs. RelWithDebInfo?

[mirabilos]

Joachim Schmitz dixit:

Unless I screwed up that is. Reproducing should be easy: new score, add Congas, select first rest. press N, KABOOM

I built that very same commit, and I cannot reproduce this. From the backtrace you’ve given…

1 std::_Rb_tree<int, std::pair<int const, Ms::StaffType>, std::_Select1st<std::pair<int const, Ms::StaffType>>, std::less<int>, std::allocator<std::pair<int const, Ms::StaffType>>>::empty stl_tree.h 1009 0x10ad72c 2 std::map<int, Ms::StaffType>::empty stl_map.h 464 0x1095988 3 Ms::StaffTypeList::staffType stafftypelist.cpp 29 0xbbad4e

29 if (staffTypeChanges.empty()) 30 return firstStaffType;

4 Ms::Staff::staffType staff.cpp 1012 0xaa8afa

[…] … I’m assuming this to be unrelated. Look at libmscore/stafftypelist.h: 28 class StaffTypeList { 30 std::map<int, StaffType> staffTypeChanges; 33 StaffTypeList() {} That std::map is never instantiated as far as I can tell. Perhaps it only works by accident but if the stack/heap contains something else in that place, it explodes. bye, //mirabilos -- <ch> you introduced a merge commit │<mika> % g rebase -i HEAD^^ <mika> sorry, no idea and rebasing just fscked │<mika> Segmentation <ch> should have cloned into a clean repo │ fault (core dumped) <ch> if I rebase that now, it's really ugh │<mika:#grml> wuahhhhhh

[Jojo-Schmitz]

Yes, maybe your change is just the straw that breaks the camel's back, under some circumstances and on some platforms, compilers, Qt versions...
But it happens only with that change of your's, not with any other.

[mirabilos]

Joachim Schmitz dixit:

Yes, maybe your change is just the straw that breaks the camel's back, under some cicumstancesne and some platforms. But it happens only with that change of your's, not with any other.

My patch changes the structure layout of a structure which may just be enough to hit this. But I assume a new on the map in the constructor should fix it?

[Jojo-Schmitz]

Where/how to place such a new?

There is a(n empty) constructor, StaffTypeList() {}, shouldn't that instantiate all (private) members?

std::map is used in multiple places, but I can't see a new on any of them.

Happens on MSVC (and with RelWithDebInfo) too, so does not depend on compiler (nor build options).
So might still be a Qt 5.15.2 issue (which I'm using currently), or a plain Windows one?

[mirabilos]

Joachim Schmitz dixit:

`std::map` is used in multiple places, but I can't see a `new` on any of them

Oh okay, so this is an embedded object, not a pointer? Hrm. Then I’m fresh out of ideas.

[mirabilos]

Joachim Schmitz dixit:

Happens on MSVC (and with RelWithDebInfo) too. So might still be a Qt 5.15.2 issue (which I'm using currently), or a plain Windows one?

Huh. I have no way to test the latter. I’m both compiling against and running with 5.15.2+dfsg-5 so either they fixed something in Qt in the Debian patches or the latter or… dunno. I’ve also no idea how to debug that under Windows. I’d try to inspect the various objects involved and their members, even internal ones, and maybe set a breakpoint to just before the crash (or, if the debugger has a “time machine”, rewind a bit).

[AntonioBL]

@mirabilos : if it is a hard-to-find bug, under Linux you could try to compile (in debug mode) with Address Sanitizer turned on (and then run from command line without the need for a debugger). It then halts whenever a memory/address violation is found.
Just clean the build directory and build after having defined a couple of additional env variables, see:

MuseScore/.github/workflows/ci_linux.yml

Lines 121 to 122 in 6fe71f9

	CFLAGS: "-fsanitize=address -fno-omit-frame-pointer"
	CXXFLAGS: "-fsanitize=address -fno-omit-frame-pointer"

Maybe it can be enabled also with CMake, but it has been a while since I last compiled MuseScore (and my laptop broke so I cannot check at the moment).

If it is a Windows-only bug then it is harder to deal with. I don't know if it is possible to (compile and) run under Wine.

Ciao,
ABL

[Jojo-Schmitz]

Maybe it only happens if you rebase to the latest 3.x? So that one of the (21) commits between 5c548c3 (which your PR is based on) and 7d1c732 (the currently latest) is triggering this?
That's at least the way I reproduced it, your PR on top of the latest 3.x one.

[mirabilos]

Joachim Schmitz dixit:

Maybe it only happens if you rebase to the latest 3.x? So that some of the commits between 5c548c3 (which your PR is based on) and 7d1c732 is triggigering this?

Oh, I thought you tested this commit, not a rebased version of it?

[Jojo-Schmitz]

I tested this commit, built myself and on top of the latest 3.x.

Sorry, I should have made that clear earlier...

[mirabilos]

Joachim Schmitz dixit:

I tested this commit, built myself and on top of the latest 3.x.

No, then you did not test this commit, you tested the commitdiff between this commit and its parent applied on top of a different commit. A commit is always the state of the source tree (in git), not a change (like in other version control systems). So… on top of which precise commit in the 3.x branch (which *is* a moving target) did you test this?

[Jojo-Schmitz]

The latest, 7d1c732, as mentioned above. Last time yesterday. It is if at all, it a slow moving target currently.
First time on whatever was current at the day I reported the crash here, 17 days ago, so that would have been 42c75b5. That should exclude 10 commits as the possible culprits, leaving 11 more.

[Jojo-Schmitz]

Very strange, I can not reproduce the issue any longer, not even when rebasing to the latest 3.x.

[mirabilos]

I’ve updated this as follows:

• change “SoundFont 2.04 spec chapter 7.10 recommendation” warning
  from qWarning to qDebug so it doesn’t show up in upstream mu͒ 3.x
  release builds
• change “loop is fowled??” check-and-fixup code to match recent
  FluidSynth and real-existing soundfonts better:
  • permit loopstart==loopend to avoid looping
    (fix them up as loopstart=loopend=start, so it’s within the
    sample always) which FluidSynth can do, even here:
    fluid/voice.cpp:#define FLUID_MIN_LOOP_SIZE 2
fluid/voice.cpp:#define FLUID_MIN_LOOP_PAD 0
  • permit loopstart==start since FLUID_MIN_LOOP_PAD is 0 and
    this is used but do ensure loopstart and loopend are both
    within the sample, at least (and warn)
  • if loopstart > loopend, swap them around, with warning
  • stop forcibly padding the loop when not necessary, fixes
    playback of some bad/old samples
• more unsigned int (XXX should be size_t?) for sample offsets

This works well for me, in both 2.3.2 and 3.2.3 with which I’ve locally tested this. Tests and further review (mostly by diff from the previous installment? though only the audio/midi/fluid/ changes, the rest is a rebase on top of latest 3.x…) welcome!

I plan on trying to get this into the really-soon-to-be-released Debian 11, so timely OK or veto appreciated.

[mirabilos]

@Jojo-Schmitz did you have a chance to look at this again?

[Jojo-Schmitz]

It works fine for me, but I don't recall ever having had the problem this PR is trying to fix

[RomanPudashkin]

3.x is closed for any changes