EXC_BAD_ACCESS accessing packet pointer property in MIDIPacketList.parse

[MicheleLonghi]

Hi
I'm heaving quite often a memory access error on the following line of MIDIPacketList.parse, specifically accessing packet.pointee.

let bytes = UnsafeBufferPointer<UInt8>(start: data, count: Int(packet.pointee.length))

Typically happens with large amount of data received, and almost always using a wired connection.

[dnadoba]

Can you reliably reproduce the issue?
It would be great if you could create a failing unit test for this error.

[dnadoba]

@MicheleLonghi
I rewrote the parsing logic and added an assert that checks if the pointer of the packet ist null.
I could still not reproduce the issue and can not check if that fixes your issue.
The changes are in #7.
Please try it and report back if this fixes your issue or if the assert is triggered.
The assert is only active in debug builds but you can change it to a precondition.
Thanks

[MicheleLonghi]

Hi
just tried, still crashing on

packet = nextPacket.pointee

from

let parsedPacket = queue.sync { () -> [Result<[MIDIMessage], Error>] in
                    return packetList.pointee.parse(using: &connection.parser)
                }

in MIDIInputPort.init

[dnadoba]

Ah okay, I think I might have it now.
I have update #7 and the pointer dereference is now only done if we are not the last packet.
Please try it again.

[dnadoba]

Any news on this? @MicheleLonghi

[MicheleLonghi]

Any news on this? @MicheleLonghi

I apologize for the delay. I'm in the middle of a big release these days. I plan to do a deep test during the weekend.

[orchetect]

Plot twist...

I recently ran into an issue where a packet had a reported MIDIPacket.length > 256. The length was 15237 which is pretty nuts. It was 0-based index 23 out of a total of 48 MIDIPacketList.numPackets. Calling MIDIPacketNext after that caused a EXC_BAD_ACCESS crash.

Some googling seems to infer that a MIDI packet is not always 256 bytes but defaults to 256. But this is additionally confusing because Swift's CoreMIDI/MIDIServices implementation uses a 256-wide UInt8 tuple for MIDIPacket.data.

So until I can dig more, I'm not sure what exactly is going on here.

[orchetect]

This may be a hint. Using a C function to iterate the MIDIPacketList may be safer.

https://github.com/krevis/MIDIApps/blob/main/Frameworks/SnoizeMIDI/SMMIDIUtilities.m

void SMPacketListApply(const MIDIPacketList *packetList, void (NS_NOESCAPE ^block)(const MIDIPacket *packet))
{
    // This is similarly maddening to implement in Swift. If you have an UnsafePointer<MIDIPacketList>
    // which is based on data that is exactly sized to fit the packet list, calling `pointee`
    // will crash (at least, under ASAN). Just do it in C.

    if (packetList->numPackets == 0) {
        return;
    }
    const MIDIPacket *packet = &packetList->packet[0];
    for (UInt32 i = 0; i < packetList->numPackets; i++) {
        block(packet);
        packet = MIDIPacketNext(packet);
    }
}

https://github.com/krevis/MIDIApps/blob/main/Frameworks/SnoizeMIDI/SMMIDIUtilities.h

#import <Foundation/Foundation.h>
#import <CoreMIDI/CoreMIDI.h>

// Iterate through the packets in the given packet list, calling the given block for each packet.
// In Swift, use packetList.unsafeSequence() instead, when it's available.
extern void SMPacketListApply(const MIDIPacketList *packetList, void (NS_NOESCAPE ^block)(const MIDIPacket *packet));

Along with some unsafe memory access by accessing the MIDIPacket data directly, using its length property to read all bytes in the packet (which can be > 256 but the .data tuple is strangely only 256 bytes wide. It sorely needs updating by Apple.)

[orchetect]

I applied the private @_optimize(none) attribute to my encapsulating Swift method that calls MIDIPacketNext and although it showed initial promise, it didn't solve the issue.

[dnadoba]

Hi @orchetect, thank you for looking into this and reporting your results!
I will take some time and look into this issue again, probably this evening. Your work seems already really promising.

I think I have an idea to reliably reproduce the issue.
We might be able to creating a virtual MIDI loopback device and sending large messages through it.
A quick google search showed that It might be possible to do this with the built in Audio MIDI Setup App.
It would be better if we could do that in code for a unit test but this is still better as relying on a physical device.

BTW: I stumbled across MIDIPacketList.UnsafeSequence a while ago but I could not find a way to create one because MIDIPacketList does not conform to Sequence nor does MIDIPacketList.UnsafeSequence has any public initialiser. It's only available in iOS 13+ but an official Swift version from Apple could potentially fixes all our issues!

[dnadoba]

Just found UnsafeMutableMIDIPacketListPointer.
Anyone tried this already?

@MicheleLonghi @orchetect do you have apps which support iOS 12 or lower?
If not and UnsafeMutableMIDIPacketListPointer works, I would raise the minimum deployment target of this package to iOS 13. WDYT?

[orchetect]

There have been quite a few additions to CoreMIDI in Swift recently but the additions require macOS 10.15 or Big Sur 11.0.

I'm trying to retain backwards compatibility to at least 10.13.

[orchetect]

After further testing, the crashes persist so back to square one I guess.

[dnadoba]

I have tried to reproduce the issue yesterday but just found other issues 😅

I have create a demo program which just sends many MIDI messages in a loop and also tries to split it into two packages.
You can find my try in the loopback branch. There is a new executable defined in the Package.swift called MIDIKitDemo.

I have used the IAC Drive to get a loopback MIDI device to receive the messages back that I send:

Important thing is to check the Device is online checkbox.

I randomly get the newControllByteBeforePreviousMessageCouldReadCompletely error from the MIDIKit parser.
If I try to send ~1000 messages at once, the program crashes when it tries to send the messages with MIDISend(_:_:_:) with a SIGABRT and logs libc++abi.dylib: terminating.

But parsing does not crash.
I'm on Xcode 12.5 Beta 3, so this may effect the behaviour but I run the executable in release configuration.