search

krevis / MIDIApps

MIDI apps for Mac OS X: MIDI Monitor and SysEx Librarian.
http://www.snoize.com/
BSD 3-Clause "New" or "Revised" License
712 stars 112 forks source link

Potential memory corruption when receiving dense event streams #94

Closed orchetect closed 2 years ago

orchetect commented 2 years ago

I'm running into a situation where extremely rapid MIDI events generated by an application on the system via virtual MIDI port are causing MIDI Monitor 1.4.1 to receive substantially malformed/garbage data.

This is happening on macOS Monterey 12.0.1, Apple Silicon (M1 Max). I am not sure if it also happens on Intel or other versions of macOS. But I've been using MIDI Monitor for years and I've never seen this specific issue until now.

krevis commented 2 years ago

How fast is "extremely rapid"? Is there an easy way I can run this app (which app is it?) to reproduce the problem? I can definitely give it a try.

orchetect commented 2 years ago

I'm firing multiple single-event packets as fast as possible from a custom MIDI library to MIDI Monitor using the old CoreMIDI API. Back to back synchronously in the same function call. The library can receive its own events in such fashion without issue, and also send to other applications without issue. That is why it appears that MIDI Monitor may have an issue receiving such messages.

Often it's literally only 2 packets that are immediately back-to-back and many times MIDI Monitor receives partially correct data but partially garbage data. It's random. Seems like some sort of pointer or memory corruption. I ran into similar issues when testing the new Core MIDI API.

krevis commented 2 years ago

OK, so just to make sure, is this what you're doing?

(Or are you setting MIDI Monitor to "act as a destination for other apps", so it makes a virtual destination, and then you use MIDISend to send to that destination port?)

orchetect commented 2 years ago

Yes exactly. Create a virtual source, tick that source in MIDI Monitor under "MIDI sources". Not using the 'spy on output' feature.

orchetect commented 2 years ago

I also observed the same behavior when MIDI Monitor receives data from Pro Tools (same system as mentioned above).

If Pro Tools is set up to send HUI data to MIDI Monitor, it will sometimes send burst data where there are multiple packets sent in immediate succession, and MIDI Monitor randomly shows some events as garbage data.

(Which means currently, MIDI Monitor can't be relied on to display accurate data, at least on this system OS/architecture combination, and possibly others.)

krevis commented 2 years ago

So far I haven't been able to repro with my own test app. (Attached: SendMIDIDataFast.zip. Build, run, and use the menu commands under SendMIDIDataFast to send just one packet, a bunch of packets all at once, or a bunch of packets repeatedly.)

This seems to be working w/o corruption on my M1 MacBook Pro, Monterey 12.0.1, with both MIDI Monitor 1.4.1 (old ObjC code) and the current main branch (Swift rewrite).

If you could give me some way to run your code to generate events, it would save me a great deal of guesswork. Happy to do it over email. I unfortunately don't have Pro Tools.

orchetect commented 2 years ago

Thanks for putting that together.

I noticed a few things when tinkering with parameters. Using the "Send A Bunch" menu item:

krevis commented 2 years ago

Aha, I didn't remember that a zero timestamp is not technically correct to give MIDIReceived(), according to the documentation. I can repro the same problem, if I just set the timestamp to AudioGetCurrentHostTime(), which is more correct anyway. Thanks for the help, investigating.

orchetect commented 2 years ago

Yes, just checked the docs on MIDIReceived:

Unlike MIDISend(), a timestamp of 0 is not equivalent to "now"; the driver or virtual source is responsible for putting proper timestamps in the packets.

Anecdotally, I also recall on very rare occasion, some software (such as DAWs, which care about timing information) would not recognize MIDI events that had a timetag of 0, depending on their particular implementation. However this was exceedingly rare as most applications ignore the timetag entirely.

AudioGetCurrentHostTime() just returns mach_absolute_time(), FWIW.

krevis commented 2 years ago

OK, this is my fault, and (AFAIK) it is only broken on M1 machines.

When CoreMIDI gives us a MIDIPacketList we were incorrectly computing its size if there was more than one packet. On M1 (ARM) there may now be up to 3 bytes of padding between subsequent MIDIPackets. (On PowerPC and Intel they were packed tightly together.) As a result some data at the end of a packet list could get corrupted.

This is already fixed on the main branch, because during the rewrite in Swift, I switched to MIDIPacketList.sizeInBytes() and SMPacketListSize() which doesn't have that bug.

Now to decide whether I should put out another release from the old ObjC branch, or just take this as a cue to finally finish up the Swift branch and release it.

orchetect commented 2 years ago

Ok interesting. May be architecture memory alignment semantics which could be obfuscated behind their size method so you'd be lucky if any of that was mentioned in the documentation.

krevis commented 2 years ago

It's documented in the header.... well, tersely, but it is there:

    #if TARGET_CPU_ARM || TARGET_CPU_ARM64
        // MIDIPacket must be 4-byte aligned
        #define MIDIPacketNext(pkt) ((MIDIPacket *)(((uintptr_t)(&(pkt)->data[(pkt)->length]) + 3) & ~3))
    #else
        #define MIDIPacketNext(pkt) ((MIDIPacket *)&(pkt)->data[(pkt)->length])
    #endif

If I had ever used CoreMIDI on iOS devices (which have always been ARM) I would have noticed the ramifications 10 years ago. Oh well.

orchetect commented 2 years ago

Makes sense, as I've seen many StackOverflow threads over the years about Core MIDI working in unexpected/inconsistent ways on iOS. I'm sure this played some part.

krevis commented 2 years ago

I just released MIDI Monitor 1.5 which has this fix.