`MidiPlayer` crashes when `note_off()` is called

[Xrayez]

Goost and Godot version: 608257a2bd15395cd8188ebc122c12e02a303a6a

OS/platform/device including version: Windows 10

Issue description: Issue detected by fuzzer: https://github.com/goostengine/goost-fuzzer/actions/runs/1523496655

[0] tsf_note_on (goost\thirdparty\tsf\tsf.h:1429)
[1] MidiPlayer::note_off (goost\scene\audio\midi_player.cpp:232)
[2] MethodBind2<MidiPlayer,int,int>::call (goost\godot\core\method_bind.gen.inc:1619)
[3] Object::call (goost\godot\core\object.cpp:918)
[4] Variant::call_ptr (goost\godot\core\variant_call.cpp:1180)
[5] GDScriptFunction::call (goost\godot\modules\gdscript\gdscript_function.cpp:1049)
[6] GDScriptInstance::call (goost\godot\modules\gdscript\gdscript.cpp:1179)
[7] Object::call (goost\godot\core\object.cpp:899)
[8] Object::_call_bind (goost\godot\core\object.cpp:686)
[9] MethodBindVarArg<Object>::call (goost\godot\core\method_bind.h:333)
[10] Object::call (goost\godot\core\object.cpp:918)
[11] Variant::call_ptr (goost\godot\core\variant_call.cpp:1180)
[12] GDScriptFunction::call (goost\godot\modules\gdscript\gdscript_function.cpp:1045)
[13] GDScriptInstance::call (goost\godot\modules\gdscript\gdscript.cpp:1179)
[14] Object::call (goost\godot\core\object.cpp:899)
[15] Variant::call_ptr (goost\godot\core\variant_call.cpp:1180)
[16] GDScriptFunction::call (goost\godot\modules\gdscript\gdscript_function.cpp:1045)
[17] GDScriptFunctionState::resume (goost\godot\modules\gdscript\gdscript_function.cpp:1810)
[18] GDScriptFunctionState::_signal_callback (goost\godot\modules\gdscript\gdscript_function.cpp:1758)
[19] MethodBindVarArg<GDScriptFunctionState>::call (goost\godot\core\method_bind.h:333)
[20] Object::call (goost\godot\core\object.cpp:918)
[21] Object::emit_signal (goost\godot\core\object.cpp:1224)
[22] Object::_emit_signal (goost\godot\core\object.cpp:1158)
[23] MethodBindVarArg<Object>::call (goost\godot\core\method_bind.h:333)
[24] Object::call (goost\godot\core\object.cpp:918)
[25] Variant::call_ptr (goost\godot\core\variant_call.cpp:1180)
[26] GDScriptFunction::call (goost\godot\modules\gdscript\gdscript_function.cpp:1049)
[27] GDScriptInstance::call_multilevel (goost\godot\modules\gdscript\gdscript.cpp:1194)
[28] Node::_notification (goost\godot\scene\main\node.cpp:56)
[29] Node::_notificationv (goost\godot\scene\main\node.h:45)
[30] CanvasItem::_notificationv (goost\godot\scene\2d\canvas_item.h:163)
[31] Control::_notificationv (goost\godot\scene\gui\control.h:47)
[32] Object::notification (goost\godot\core\object.cpp:929)
[33] SceneTree::_notify_group_pause (goost\godot\scene\main\scene_tree.cpp:992)
[34] SceneTree::idle (goost\godot\scene\main\scene_tree.cpp:536)
[35] Main::iteration (goost\godot\main\main.cpp:2229)
[36] OS_Windows::run (goost\godot\platform\windows\os_windows.cpp:3360)
[37] widechar_main (goost\godot\platform\windows\godot_windows.cpp:161)
[38] _main (goost\godot\platform\windows\godot_windows.cpp:183)
[39] main (goost\godot\platform\windows\godot_windows.cpp:195)
[40] __scrt_common_main_seh (D:\a01\_work\9\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[41] BaseThreadInitThunk
-- END OF BACKTRACE --
================================================================
=================================================================
==12812==ERROR: AddressSanitizer: access-violation on unknown address 0x000000000020 (pc 0x7ff6b15c6e48 bp 0x000000000000 sp 0x00b92d9ff790 T0)
==12812==The signal is caused by a READ memory access.
==12812==Hint: address points to the zero page.
    #0 0x7ff9aa902650  (C:\WINDOWS\SYSTEM32\ntdll.dll+0x180052650)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: access-violation (C:\WINDOWS\SYSTEM32\ntdll.dll+0x180052650)
==12812==ABORTING

Steps to reproduce:

extends "res://addons/gut/test.gd"

func test_crash():
    var player = MidiPlayer.new()
    add_child_autofree(player)
    player.note_off(91, 6)

Only happens if MidiFile is null.

[Xrayez]

There seems to be an easy fix and I'd fix this myself, but I've noticed another potential bug which I'm not sure about:

https://github.com/goostengine/goost/blob/59a9f1302b58bf90e02a8f8cd8faf7beddb460c2/scene/audio/midi_player.cpp#L230-L232

The method says note_off(), but implementation uses tsf_note_on().

Also, I'd suggest replacing these type of checks:

https://github.com/goostengine/goost/blob/59a9f1302b58bf90e02a8f8cd8faf7beddb460c2/scene/audio/midi_player.cpp#L220-L222

into:

ERR_FAIL_NULL_MSG(mTsf, "MidiFile is not loaded");
// or with return value:
ERR_FAIL_NULL_V_MSG(mTsf, 0.0, "MidiFile is not loaded");

Unless there's a use case where mTsf is expected to be nullptr.

CC @filipworksdev

[ghost]

Thanks for spotting that! It calls tsf_note_on(mTsf, inPresetIndex, inKey, 0.0); with a null mTsf CRASH. Fixed it on my end.

mTsf is the soundfont so it actually crashes because I didn't load the SoundFont.

About the error I have no opinion. It makes sense to not be an error if you have no SoundFont loaded and simply not do anything. Without a SoundFont, MidiPlayer is completely useless lol.

Regarding note_off calling tsf_note_on there is actually a note off you can replace that line with tsf_note_off(mTsf, inPresetIndex, inKey); I inherited some of that code and I think it might simply be an honest copy paste error from note_on above.

[ghost]

Damn this fuzzer is good though! Be careful the fuzzer seems to love abusing set_data in the MidiFile which will cause crashes. These crashes are expected if you directly manipulate byte data. Unforunately I cannot disable setters because the properties also stop working internally and there is no good way to check if data is valid.

[Xrayez]

About the error I have no opinion. It makes sense to not be an error if you have no SoundFont loaded and simply not do anything. Without a SoundFont, MidiPlayer is completely useless lol.

An error would suggest that one needs to assign a SoundFont in order to use those methods. In fact, it's so crucial to SoundFont functionality so that it crashes without the check. If there's no practical use case for when you have no SoundFont loaded (like falling back to default library, for instance), it makes sense to me to print an error in those cases.