Hi, 2013/1/15 <joerg-cyril.hoe...@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