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  :
> 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 :
> 2013/1/14  :
> > 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 :
> > 2013/1/14  :
> > > 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  :
> 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

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  :
> 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.