search

lathoub / Arduino-AppleMIDI-Library

Send and receive MIDI messages over Ethernet (rtpMIDI or AppleMIDI)
Other
306 stars 66 forks source link

[ESP32] "Stack smashing protect failure!" error with default bonjour name MIDI #97

Closed midasgossye closed 3 years ago

midasgossye commented 3 years ago

First and foremost, thanks for the awesome library! I managed to make a wireless recording light that interfaces with Logic Pro X with an ESP32.

When trying to run the ESP32_callbacks example, I always encountered the Stack smashing protect failure! error message when I hit "Connect" on the mac network MIDI setup page. After troubleshooting a bit, I noticed that the cause of this issue was the length of the bonjour name of the mac hosting the MIDI network session. The default was set to "Macbook Pro of (insert name)". I presume the lenghty name overflows the text buffer reserved for this name in the library. Changing this lengthy name to a shorter string fixed the issue.

It would be nice if this could be added in the README, and/or a larger buffer size can be reservered for this name to fix the issue. Since this happened with the default assigned name, I can imagine that other people can also easily run into this issue.

lathoub commented 3 years ago

Thanks for reporting @midasgossye. the parser should survive a longer than expected session name. After some investigation, i noticed that the issue is with https://github.com/lathoub/Arduino-AppleMIDI-Library/blob/ba3db187764bc44f8ccc18e49f8532e86e48d043/src/AppleMIDI_Parser.h#L87

and it should be: if (bi < DefaultSettings::MaxSessionNameLen) (boundary should not be included)

Should also be corrected here https://github.com/lathoub/Arduino-AppleMIDI-Library/blob/ba3db187764bc44f8ccc18e49f8532e86e48d043/src/AppleMIDI_Parser.h#L277 and here https://github.com/lathoub/Arduino-AppleMIDI-Library/blob/ba3db187764bc44f8ccc18e49f8532e86e48d043/src/AppleMIDI_Parser.h#L339

@midasgossye Do you want to create a PR with the correction?

lathoub commented 3 years ago

I have included the fix in the v3.0.0rc branch, for you to test

midasgossye commented 3 years ago

Wow, that was fixed quickly! I just tested out the library with the same scenario from the v3.0.0rc branch. It didn't produce any errors with the long bonjour name aymore. So that fixed the issue I was having. A new issue popped up with the v3.0.0rc branch version though: a compile error: 'class appleMidi::AppleMIDISession<WiFiUDP>' has no member named 'setHandleError' Since I didn't use the HandleError event I just commented out the AppleMIDI.setHandleError(OnAppleMidiError); line and the OnAppleMidiError() routine. Maybe this is expected since V3.0.0 is still a WIP?

lathoub commented 3 years ago

A new issue popped up with the v3.0.0rc branch version though: a compile error: 'class appleMidi::AppleMIDISession' has no member named 'setHandleError'

Indeed, the reason to bump the major version number. 'setHandleError' has been removed and replaced with 'setHandleException' that has an additional value parameter (compared to the old 'setHandleError'). The examples in v3.0.0rc have been updated to reflect this change. In v3, the memory footprint can be reduced significantly (not as important for ES32 users).

I'm about to push out the Release Candidate, so any comments are appriciated.

lathoub commented 3 years ago

(and i added a note in the ReadMe.md that long session names will be truncated)

lathoub commented 3 years ago

solved in latest release #98