Truncate MIDI SysEx messages after termination byte
titan.costa at gmail.com
Tue Jan 15 07:04:17 CST 2013
2013/1/15 <Joerg-Cyril.Hoehle at t-systems.com>:
> 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.
More information about the wine-devel