[fluid-dev] fluid_midi: Bug handling invalid MIDI files

[Matt Giuca]

Hi Fluid-Dev,

I found a small bug in fluid_midi.c:fluid_midi_file_getc: the function
returns an undefined value (usually 0) if there is an end-of-file
condition, while all usages of the function check for a negative value
to signal EOF. This means that files which end unexpectedly (in
certain positions -- where getc is used) will go undetected, probably
generating other spurious errors.

See example-eof.mid (attached) -- this is a MIDI file which is
truncated at the first event:

$ fluidsynth -lni -a alsa example-eof.mid

Expected output:
fluidsynth: error: Unexpected end of file

Actual output:
fluidsynth: error: Unrecognized MIDI event
(i.e., some random error)

It's caused because fluid_midi_file_getc calls fread to read into the
unsigned char c, which returns the number of bytes read. If the number
of bytes read is 0 (EOF), it stores 0 into n (which is ignored) and
doesn't modify c. Thus, fluid_midi_file_getc returns c anyway (which
was uninitialised). This doesn't make sense as all the calls to
fluid_midi_file_getc check if the return is negative, and raise an
"Unexpected end of file" error.

I don't know why fluid_midi_file_getc doesn't just call fgetc instead
of fread (except for the fact that there is no abstract FLUID_FGETC
defined in utils/fluidsynth_priv.h, which could easily be added), but
for now, I'll just do as little damage as possible and keep the call
to fread, adding a check for n < 1:

--- fluidsynth/src/midi/fluid_midi.c    2010-07-28 20:21:17 +0000
+++ fluidsynth/src/midi/fluid_midi.c    2010-10-20 12:44:55 +0000
@@ -95,6 +95,9 @@
         mf->c = -1;
     } else {
         n = FLUID_FREAD(&c, 1, 1, mf->fp);
+        if (n < 1) {
+            return -1;
+        }
         mf->trackpos++;
     }
     return (int) c;

Note that this also fixes another (minor) bug, which is that
mf->trackpos is incremented even though no bytes were read -- with
this patch applied it is not incremented unless a byte is read. The
fact that trackpos was over-incremented isn't a problem (since loading
halts as soon as getc fails), except that due to the first bug the
loading doesn't halt, so this could just potentially cause more
spurious errors.

I'm reasonably confident this isn't a security vulnerability, as you
can't get any data into the program with an undefined return from getc
that you couldn't from just having the byte in the file. The
trackpos++ bug is also not a problem, since the only bad thing about
having trackpos too high is ending tracks too early or not seeking far
enough. So these bugs just cause general load-time mayhem.

Anyway, the patch is attached. An alternative fix is to add
FLUID_FGETC to fliudsynth_priv.h, and use that instead.

Matt Giuca

example-eof.mid
Description: MIDI audio

fluid_midi.patch
Description: Text Data