RFR: 8319598: SMFParser misinterprets interrupted running status

[Jan Trukenmüller]

Am 07.11.23 um 20:37 schrieb Jan Trukenmüller:
> The MIDI file parser misinterprets events without status byte when they appear directly after a Meta of SysEx event.
> 
> For my bugfix I had to decide between two possible solutions:
> - Strict solution: Throw an InvalidMidiDataException
> - Tolerant solution: Use the status of the last channel event as running status
> 
> I used the tolerant solution.
> I will send an email to the list client-libs-dev with my reasons.
> 
> -------------
> 
> Commit messages:
>   - 8319598: Fixed incorrect interpretation of interrupted running status in SMFParser
> 
> Changes: https://git.openjdk.org/jdk/pull/16546/files
>   Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16546&range=00
>    Issue: https://bugs.openjdk.org/browse/JDK-8319598
>    Stats: 121 lines in 2 files changed: 118 ins; 0 del; 3 mod
>    Patch: https://git.openjdk.org/jdk/pull/16546.diff
>    Fetch: git fetch https://git.openjdk.org/jdk.git pull/16546/head:pull/16546
> 
> PR: https://git.openjdk.org/jdk/pull/16546


Hi,

As I promised, here are my reasons for choosing the tolerant solution.
But first a short description what my PR is about.

Definition:
An "interrupted running status" means:
- In a MIDI file a running status is active
- The next event is a Meta or SysEx event
- The next event does not contain a status byte

Problem:
Files with "interrupted running status" are not parsed correctly.

Possible solutions:
- Strict solution: Throw an InvalidMidiDataException whenever
   an interrupted running status is found
- Tolerant soution: Use the status byte of the last channel
   event with a status byte

Here are 4 reasons for choosing the tolerant solution, sorted from weak 
to strong:


Reason 1:

The SMFParser is very tolerant against even worse violations (e.g. data 
bytes greater than 0x7F, meta data with wrong length and so on)
So I think it is in the spirit of the parser to also allow this minor 
violation.
I think that interrupted running status is only a "minor" violation 
because there is only one sentence in the specification with this 
assumption.
And because there is hardly any other possible interpretation that could 
make sense.


Reason 2:

I tried out a minimal file with interrupted running status with several 
other applications.
The following ones interpret the file like suggested by the tolerant 
solution:

- timidity
- Rosegarden
- Musescore3
- aplaymidi
- Audacious
- midicsv
- JZZ.midi.SMF.js (JZZ.js)
- VLC (with fluidsynth plugin)

The following tools behave differently:

- Pipewire's pw-mididump
   reacts with a segmentation fault
- fluidsynth -a alsa -m alsa_seq -l -i testfile.mid
   Doesn't play the file but returns a successful exit code.
   No error message.
- pmidi
   Error (Meta event 60 not implemented\nUnexpected end of file)

That's 8:3.
And I don't think that the behavior of the 3 is desirable.


Reason 3:

I downloaded a large archive with more than 169.000 MIDI files and did a 
lot of analyses and tests.
First without the bugfix and then with the bugfix.
The results can be summarized like this:

- 1.1% of all MIDI files fail to be loaded by SMFParser (for ANY reason)
- 0.43% of all MIDI files contain interrupted running status
   (maybe some more that have a different problem before the first
   IRS occurs)
- From the files with IRS
   - before the bugfix:
     - 15.67% can be parsed without exception
              (but with incorrectly interpreted data)
     - 69.9% fail with InvalidMidiDataException
     - 14.42% fail with EOFException
   - after the bugfix:
     - 94.2% can be parsed without exception
     - 5.4% fail with InvalidMidiDataException
     - 0.4% fail with EOFException
- The bugfix fixes exceptions for:
   - 78.5% of files with interrupted running status
   - 30.25% of ALL failing MIDI files in general !!!!!
- The bugfix doesn't change anything to any MIDI file without
   interrupted running status

Details:

169457 files are in the archive.
721 contain interrupted running status. (I copied them to a different 
folder to be able to analyze them separately.)

Statistics from (only) the 721 files with interrupted running status:
- before the fix:
   - all:                      721
   - success:                  113
   - InvalidMidiDataException: 504
   - EOFException:             104
- after the fix:
   - all:                      721
   - success:                  679
   - InvalidMidiDataException:  39
   - EOFException:               3

Statistics from ALL files before and after the fix:
- before the fix:
   - all:                       169457
   - success:                   167586
   - fail:                        1871
     - InvalidMidiDataException:  1606
     - EOFException:               265
- after the fix:
   - all:                       169457
   - success:                   168152
   - fail:                        1305
     - InvalidMidiDataException:  1141
     - EOFException:               164


Reason 4:

In a different branch of the JDK I added my bugfix and also a lot of 
debug information to print out several events before and after every 
interrupted running status. I tested this against the 721 files with IRS.
The result was more than 8000 occurrences of IRS, and I looked at each 
of them!
Manually!!!
The vast majority of these occurrences shows very clearly that the data 
makes absolute sense.

However a few of them (maybe 20-30) made no sense at all or were at 
least "suspicious".
So I looked at these remaining files with a hex editor.
In most cases there was a meta message with an incorrect length field or 
something similar, so that the following bytes were interpreted incorrectly.


Conclusion:

After this investigation I'm very confident that the tolerant solution 
is the best.
And isn't it pretty cool to fix 30% of all exceptions with such a simple 
fix?

Best regards
Jan Trukenmüller
(Github user: truj)