Truncate MIDI SysEx messages after termination byte

Hi,

Christian Costa wrote:
> I took a look at the
> alsa code and this code simply does not do what it is supposed to.
> I also looked at it today and noted those bogus lines you quote. Needs
> a patch (+ fix memory leak).

However, Johannes' change is presumably different, as he wants to
scan the buffer contents for a F7 terminator and ignore subsequent
bytes.  If I were to decide, I'd like to see more supporting evidence.

I'd appreciate if Johannes would open a bug report and attach some WINEDEBUG=+midi,+mmdevapi,+alsa,+winmm,+driver,+tid,+timestamp logs to bugzilla,
much like bug #26928. http://bugs.winehq.org/show_bug.cgi?id=26928
Having some logs with *complete* SysEx contents helps.

There's also a "closed as abandoned" bug about microKORG
http://bugs.winehq.org/show_bug.cgi?id=17608
which might be of interest to Johannes.

Regards,
	Jörg Höhle

[Christian Costa]

2013/1/14  <Joerg-Cyril.Hoehle(a)t-systems.com>:
> Hi,
>
> Christian Costa wrote:
> > I took a look at the
> > alsa code and this code simply does not do what it is supposed to.
> > I also looked at it today and noted those bogus lines you quote. Needs
> > a patch (+ fix memory leak).

I will take a look at it and send a patch unless someone plan to do it.
I will not  be able to test with my 2 HW midi devices right now so I need
someone to test it. That would be good to enter a bug report for that.
Johannes, can you do that?
> However, Johannes' change is presumably different, as he wants to
> scan the buffer contents for a F7 terminator and ignore subsequent
> bytes.  If I were to decide, I'd like to see more supporting evidence.

In the case the first by is F0 and the last one is F7, the code passes
a uninitialized buffer to the alsa function with a wrong terminating
byte F0 inserted which
is even worst because it opens a new sys ex sequence without end.
The Johanness patch prevents this case to happend by making last byte to be F7.

Bye
Christian

[Christian Costa]

2013/1/14 Christian Costa <titan.costa(a)gmail.com>:
> 2013/1/14  <Joerg-Cyril.Hoehle(a)t-systems.com>:
> > Hi,
> >
> > Christian Costa wrote:
> > > I took a look at the
> > > alsa code and this code simply does not do what it is supposed to.
> > > I also looked at it today and noted those bogus lines you quote. Needs
> > > a patch (+ fix memory leak).
>
> I will take a look at it and send a patch unless someone plan to do it.
> I will not  be able to test with my 2 HW midi devices right now so I need
> someone to test it. That would be good to enter a bug report for that.
> Johannes, can you do that?
> > However, Johannes' change is presumably different, as he wants to
> > scan the buffer contents for a F7 terminator and ignore subsequent
> > bytes.  If I were to decide, I'd like to see more supporting evidence.
>
> In the case the first by is F0 and the last one is F7, the code passes

I meant "last one if not F7".

[Christian Costa]

Le 14/01/2013 19:04, Christian Costa a écrit :
> 2013/1/14 Christian Costa <titan.costa(a)gmail.com>:
> > 2013/1/14  <Joerg-Cyril.Hoehle(a)t-systems.com>:
> > > Hi,
> > >
> > > Christian Costa wrote:
> > > > I took a look at the
> > > > alsa code and this code simply does not do what it is supposed to.
> > > > I also looked at it today and noted those bogus lines you quote. Needs
> > > > a patch (+ fix memory leak).
> > > > I will take a look at it and send a patch unless someone plan to do it.
> > > > I will not  be able to test with my 2 HW midi devices right now so I need
> > > > someone to test it. That would be good to enter a bug report for that.
> > > > Johannes, can you do that?
> > > However, Johannes' change is presumably different, as he wants to
> > > scan the buffer contents for a F7 terminator and ignore subsequent
> > > bytes.  If I were to decide, I'd like to see more supporting evidence.
> > > In the case the first by is F0 and the last one is F7, the code passes
> > > I meant "last one if not F7".

Here are a patch to fix modLongData.
I tested it with the unit test attached.
Please give it a try.

Hi,

Christian costa wrote:
> +            lpNewData[lpMidiHdr->dwBufferLength + len_add] = 0xF7;
> Here you're using spaces only, whereas the surrounding code uses TAB8.
> 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 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

Regards,
	Jörg Höhle

[Christian Costa]

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

Christian costa wrote:
> > But I don't know whether there's some harmless general SysEx that we 
> > can send out.  Maybe some MIDI guru could suggest some?
> > What do you by harmless ? For what ? A midi HW device connected to the midi port ?

A harmless MIDI message, perhaps there exists a NOP SysEx?
E.g. a reset command is *not* harmless.

Here are a few SysEx I gathered, looking for MIDI SysEx documentation
Proteus SoundEngine System Reset SysEx F0 18 04 00 23 F7
without knowing what they do:
F0 56 64 4F 6C F7
F0 7D 12 34 10 02 11 00 F7
> we cannot verify the output.
> Sure, however increased code coverage *has* benefits.  E.g. sometimes I use Valgrind.

Regards,
 Jörg

[Christian Costa]

2013/1/15  <Joerg-Cyril.Hoehle(a)t-systems.com>:
> Christian costa wrote:
> > > But I don't know whether there's some harmless general SysEx that we
> > > can send out.  Maybe some MIDI guru could suggest some?
> > > What do you by harmless ? For what ? A midi HW device connected to the midi port ?
>
> A harmless MIDI message, perhaps there exists a NOP SysEx?
> E.g. a reset command is *not* harmless.
>
> Here are a few SysEx I gathered, looking for MIDI SysEx documentation
> Proteus SoundEngine System Reset SysEx F0 18 04 00 23 F7
> without knowing what they do:
> F0 56 64 4F 6C F7
> F0 7D 12 34 10 02 11 00 F7

SysEx are quite device specific and I don't kwnon about nop sysex.
Anyway I can take one of these.
> > we cannot verify the output.
> > Sure, however increased code coverage *has* benefits.  E.g. sometimes I use Valgrind.

Ok. Makes sense.