1945967 - Sending a large MIDI message crashes FireFox on MacOS

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P2)

Description

[mark]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:134.0) Gecko/20100101 Firefox/134.0

Steps to reproduce:

Use WebMIDI to send a MIDI SysEx message greater than 65k bytes. This is not uncommon for things like patch managers for synthesizers.

Actual results:

Sending such a message on Linux works just fine.
Same thing running on Mac OS X crashes the entire browser immediately.

This crash stems from a bug in /dom/midi/midir_impl/src/lib.rs:

WebMIDI is implemented using a Rust crate called midir_impl in the Gecko codebase.
...which is built on the Rust crate midir (https://github.com/Boddlnagg/midir)
...which, for MacOS, is built on the Rust crate coremidi (https://github.com/chris-zen/coremidi)
...which is built on the Rust crate coremidi-sys (https://github.com/jonas-k/coremidi-sys)
...which finall calls CoreMIDI in the Mac OS.

Gecko's midir_impl makes this call to send bytes from WebMIDI:

connection.send(data)

This calls midir's MidiOutputConnection::send() which starts by creating a PacketBuffer:

let packets = PacketBuffer::new(send_time, message);

This uses coremidi's PacketBuffer which is a thin wrapper around coremidi-sys's (and ultimately CoreMIDI's) MIDIPacketList and MIDIPacket.

The bug is that Mac OS's MIDIPacketList can be no bigger than 64k...
...coremidi-sys doesn't check the size of the PacketLists created
...coremidi doesn't check the size of the data passed to MIDIPacketList functions
...midir doesn't check the size of the data it passes to PacketBuffer()
...midir_impl doesn't check the size of the data it passes to send()

... and the result is a corrupted MIDIPacketList that when ulitimately sent to CoreMIDI (via MIDISend) - crashes the process hard.

It is arguable that the API presented by coremidi-sys and coremidi are correct, in that they are thin wrappers around Apple's CoreMIDI.

It is arguable that the API presented by midir should do the work to take large sysex's and break them up and send them in chunks on Mac OS. I have filed a bug on that project to this effect: https://github.com/Boddlnagg/midir/issues/165

Chromium handles this situation properly in it's MacOS backend: It breaks the large
message up into chunks (currently 32k, but that is probably pessamistic) - and sends them one by one. While it would be nice for midir to fix this -- currently it is a hard crash for FireFox - so perhaps FireFox should in the midir_impl crate.

It isn't reasonable to fix this in the WebMIDI code or in applications that send WebMIDI: WebMIDI insistes that sends be complete messages, and so do not accept the SysEx broken up in the segments.

Expected results:

Shouldn't have crashed.

I'm more than happy to provide details, source line references, sample programs that demo the bug, etc.... I'm just not in an environment where I can build FireFox and give a patch.

Comment 1

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:cmartin, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Chris Martin [:cmartin]]

+Gabriele for WebMIDI-related stuff.

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: WIP: Bug 1945967 - Update the midir crate to version 0.10.1 to better handle large SysEx MIDI messages

Comment 4

[Gabriele Svelto [:gsvelto]]

Wrong bug number, sorry for the noise.

Comment 5

[Nishchay [:nish]]

Hello! i would like to try to fix this issue :D
Below are some questions i have:

1. it looks like from the midi docs, messages this huge (bigger than 65536 bytes) are only possible when a SysEx message is sent. (other all midi messages are 1 to 3 bytes)
2. do we chunk messages like chrome does at 32k or is exact size fine (im chunking it with u16::MAX as per MIDIPacket length

if all looks well, can i submit a patch? thank you!

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to [:nish] Nishchay from comment #5)

1. it looks like from the midi docs, messages this huge (bigger than 65536 bytes) are only possible when a SysEx message is sent. (other all midi messages are 1 to 3 bytes)

Yes, that's also my understanding.

2. do we chunk messages like chrome does at 32k or is exact size fine (im chunking it with u16::MAX as per MIDIPacket length

I guess we'll have to test. If u16::MAX works then why not?

if all looks well, can i submit a patch? thank you!

Sure, go ahead, and if you need help jump into the #introduction channel on chat.mozilla.org. You'll find me there.

Comment 7

[Nishchay [:nish]]

Attachment: Bug 1945967 - Chunking MIDI SysEx messages to avoid crash on mac r?gsvelto

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1d9d0c449b6e
https://hg.mozilla.org/integration/autoland/rev/250aff013dd4
Chunking MIDI SysEx messages to avoid crash on mac r=gsvelto

Comment 9

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/250aff013dd4