Re: Truncate MIDI SysEx messages after termination byte

[Christian Costa]

Hi,

2013/1/15  :
> 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:
> - In the MOD_FMSYNTH branch, no
>       HeapFree(GetProcessHeap(), 0, lpNewData);
>
> Here I'm surprised that the static analysers did not spot it.
> - 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()
> - 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