New subject: Fwd: [PATCH 1/3] winmm: Fix mciSendString command parsing on 64-bit.

27 Aug 2010

      For some reason I forgot to hit reply to all.
---------- Forwarded message ----------
From: Octavian Voicu octavian.voicu@gmail.com
Date: Thu, Aug 26, 2010 at 2:15 PM
Subject: Re: [PATCH 1/3] winmm: Fix mciSendString command parsing on 64-bit.
To: Joerg-Cyril.Hoehle@t-systems.com
On Thu, Aug 26, 2010 at 1:11 PM,  Joerg-Cyril.Hoehle@t-systems.com wrote:
...
are you aware of bug http://bugs.winehq.org/show_bug.cgi?id=22146
and the partial patch and additional test cases there?
First of all, I wasn't aware of it, so thanks for pointing it out. I
just saw the crash on my system and tried to fix it; to my shame, I
didn't search wine bug database. Sorry for the duplicate work.
...
...
winmm: Add MCI_INTEGER_PTR return type for DWORD_PTR return values.
+#define MCI_INTEGER_PTR         13
What kind of testing did you perform on native systems to derive the existence of this value?
If such value does not exist, then I fear your patch set looses the ability to
drop in native dlls for compatibility testing and validation.
If it exists, then it's good news you found it.
The only testing I ran is winmm:mci tests. With my patch set, 27 tests
fail on 64 bit (compared to 23 on 32bit). I'm guessing the 4 extra
failures are in other parts of the code. Not sure about TODOs, as I
had no tests marked as TODO when I ran the tests from command line
(the 23 tests that also fail on 32-bit are definitely marked as TODOs
on test.winehq.org).
I didn't think it was necessary to test on native systems. The command
parsing looks like internal winmm stuff, which makes no difference to
the exported interfaces, as long as the command string is parsed into
the corresponding MCI_ structures correctly.
MCI_GetDWord, MCI_GetString, MCI_ParseOptArgs, MCI_HandleReturnValues,
the functions which I modified, are explicitly listed as internal
functions.
I just looked in mmddk.h and they have this:
#ifdef _WIN64
#define MCI_INTEGER64          13
#endif // _WIN64
Which is really funny, because I wrote that patch without knowing
anything about that. Gonna take a look at resources inside a 64-bit
winmm.dll and report back, but it is pretty obvious what I'll find --
they wouldn't define MCI_INTEGER64 for nothing.
...
...
-static        DWORD   MCI_ParseOptArgs(DWORD_PTR* data, int _offset, LPCWSTR lpCmd,
+static        DWORD   MCI_ParseOptArgs(BYTE* data, int _offset, LPCWSTR lpCmd,

if (!MCI_GetDWord(&(data[offset+0]), &args) ||
!MCI_GetDWord(&(data[offset+1]), &args) ||
!MCI_GetDWord(&(data[offset+2]), &args) ||
!MCI_GetDWord(&(data[offset+3]), &args)) {

if (!MCI_GetDWord(data+offset+0*sizeof(DWORD), &args) ||
!MCI_GetDWord(data+offset+1*sizeof(DWORD), &args) ||
!MCI_GetDWord(data+offset+2*sizeof(DWORD), &args) ||
!MCI_GetDWord(data+offset+3*sizeof(DWORD), &args)) {

The MCI_XYZ_PARMS structures are neither DWORD_PTR, not BYTE*,
they are DWORD with some DWORD_PTR in between.
Wouldn't using DWORD* data help avoid such pointer arithmetic
and eliminate several hunks of the patch ?
The reason for which I opted for a BYTE* is so that I can use sizeof()
to increment the pointer correctly. DWORD_PTR has a different size of
32- and 64-bit systems, so using DWORD* would make it more complicated
to increment with sizeof(DWORD_PTR). Another reason is that offset is
now in bytes, so supplying a pointer of different size wouldn't make
much sense.
Argument type in MCI_GetDWord and MCI_GetString was changed for
consistency, it wouldn't matter very much there. For the same reason I
changed all the &data[offset] into data+offset.
In any case, using the MCI_XYZ_PARMS union looks much better :) Maybe
also stick MCI_GENERIC_PARMS in there.