Hi, 2013/1/15 <Joerg-Cyril.Hoehle(a)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