weixin_39987138 2020-11-20 21:02
浏览 0

Incorrect error semantics in fluid_midi.c

Sorry to bring this up again -- it's a minor nit-pick, but I am trying to rewrite the implementation of several functions in fluid_midi.c without changing the interface, and the current interface is a bit weird. I'm putting two issues into the one bug report since they're related and minor.

Both of these problems came up in r392, when plcl closed ticket #92. It's very hard to tell from the diff (http://fluidsynth.svn.sourceforge.net/viewvc/fluidsynth/trunk/fluidsynth/src/midi/fluid_midi.c?r1=392&r2=391&pathrev=392) because a lot changed in one commit.

The first issue is in fluid_midi_file_getc, in response to my patch on ticket #92. On line 104, the line "return FLUID_FAILED" was added. However, in my patch I had "return -1". Now FLUID_FAILED just happens to be defined as -1 (so it still works), but this is the wrong constant. Most functions return FLUID_FAILED and the calls check "if (result != FLUID_OK)", but this is not the case with fluid_midi_file_getc (which is emulating fgetc).

All of the calls to fluid_midi_file_getc check for the EOF condition with "if (result < 0)". Therefore, it is correct to return -1 to match these checks. If FLUID_FAILED were changed to a different constant, this code would break. It should be changed to "return -1" (or "return EOF" if you want to use a constant with the same semantics as fgetc).

The second issue is in fluid_midi_file_read. I am not sure what the intention behind this was, but note that on line 128, the check "if (num == len)" was added, preventing mf->trackpos from being incremented if fewer than len bytes were read. Now again, this doesn't matter too much, since it will subsequently return FLUID_FAILED and this will pretty much terminate the loading of the file. But in principle, a bug has been introduced here: the FILE*'s internal read pointer has been advanced by some number of bytes (less than len but possibly greater than 0), but the trackpos has not been incremented, so the state of mf is inconsistent. I would recommend that this be reverted such that mf->trackpos is always incremented regardless of the success or failure of the function.

I have attached a small patch for each of these issues.

Reported by: mattgiuca

Original Ticket: fluidsynth/tickets/94

该提问来源于开源项目:FluidSynth/fluidsynth

  • 写回答

5条回答 默认 最新

  • weixin_39987138 2020-11-20 21:02
    关注

    Patch for fluid_midi_file_getc

    Original comment by: mattgiuca

    评论

报告相同问题?