Truncate MIDI SysEx messages after termination byte

Christian Costa titan.costa at gmail.com
Tue Jan 15 07:04:17 CST 2013


Hi,

2013/1/15  <Joerg-Cyril.Hoehle at t-systems.com>:
> Hi,
>
> Christian costa wrote:
>>+            lpNewData[lpMidiHdr->dwBufferLength + len_add] = 0xF7;
> Here you're using spaces only, whereas the surrounding code uses TAB8.

It was not intended for submission. If I submit a patch, I will
removed all these tabs.

>
>>I tested it with the unit test attached.
> Instead of some isolated unit test, it would be preferable to
> add to winmm/tests/midi.c such that midiOutLongMsg gets covered
> by the tests. But I don't know whether there's some harmless
> general SysEx that we can send out.  Maybe some MIDI guru could
> suggest some?
>

The unit test was just to check byte insertion code. I attached for info only.
I could add a test but this will be only for debug purpose as we
cannot verify the output.
What do you by harmless ? For what ? A midi HW device connected to the
midi port ?


>
> The fix looks good, however while we are busy fixing lpNewData handling,
> I'd like to see the same patch fix its broken memory handling:
>
> 1. In the MOD_FMSYNTH branch, no
>          HeapFree(GetProcessHeap(), 0, lpNewData);
> Here I'm surprised that the static analysers did not spot it.
>
> 2. Wrong pointer being freed -- is that an indirect proof by absence of
> crash that this missing F0/F7 situation was never seen in practice?
> 1007         if (lpNewData)
> 1008                 HeapFree(GetProcessHeap(), 0, lpData);
> Also, Wine style is to suppress the if()
>
> 3. Not checking for success
> 971         lpNewData = HeapAlloc(GetProcessHeap(), 0, lpMidiHdr->dwBufferLength + 2);
> Either exit or skip adding F0/F7
>

I saw the memory leak. That needs to be fixed as well. And indeed,
this code is definitely broken
and I doubt it has been ever exercised unless it has been coded first
and them break afterwards.

Christian



More information about the wine-devel mailing list