[Bug 22146] New: 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

Summary: 64bit compatibility issue with Wine's MCI string
                    command parser
           Product: Wine
           Version: 1.0.0
          Platform: x86-64
        OS/Version: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: winmm&mci
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]

Created an attachment (id=27005)
 --> (http://bugs.winehq.org/attachment.cgi?id=27005)
work in progress MCI parser that knows about 64bit structures

Hi,

Wine's MCI string command parser is one reason why test.winehq.org shows
failures with 64bit machines.  I've created this issue so everybody can discuss
and contribute patches until evolution leads to a correct one.

The attached patch fixes part of what's currently in git, but it is still
incomplete.  Therefore I don't plan to submit it.

I've run tests on WTB's 64bit machines that proove that MCI_STATUS_PARMS and
MCI_GETDEVCAPS indeeed process differently sized dwReturn types on x64:
DWORD_PTR and DWORD.  That's what's missing from my patch.

A solution that sounds ideal is to define something like MCI_INTEGER64 and
change the resource files mciavi_res.rc and winmm_res.rc.  Given that new type,
the MCI string parser would compute the correct offset.
I'd be very pleased if somebody could conform that MS does it this way and tell
us the value of this constant.

Another solution path is to special-case MCI_STATUS_PARMS in the parser, as it
uses the only 64bit construct I knew until 5 minutes ago (beside dwCallback of
course) that's not a LP(W)STR.  HWND seems to be 32bit.

5 minutes later: What about HDC in MCI_DGV_UPDATE_PARMS?  Wait, wtypes.h says
void*!  The MCI string command parser needs to handle this type as well:
MCI_INTEGER as currently used in mciav_res.rc is not adequate on 64bit. 
In the meantime, I'll submit a patch that adds MCI_HDC and a few other missing
defines I know to the includes.

Regards,
    Jörg Höhle

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

Jörg Höhle  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |win64

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

--- Comment #1 from Jörg Höhle   2010-03-24 04:23:40 ---
Dmitry Timoshkov tells me:
"HWND is defined in windef.h ...  a generic pointer."
Perhaps I looked in some other place like wtypes.h and was mislead.

Well, we have those options
A) Define and use MCI_INTEGER64, and
B) Special-case MCI_STATUS' integer return type
C) ?

My plan is to produce 2 patches along the B route. That's "good enough" and
much better than what's currently in git (i.e. it might work :).  It can and
should be superseded by an "A route" patch as soon as somebody shows evidence
that native knows about such a special return type, either from documentation
or from a resource dump of a 64bit winmm.dll or mciav32.dll if that's legal.
The second patch would handle MCI_HWND, MCI_HDC and MCI_HPAL types.

Perhaps MS changed MCI_STATUS' return type to DWORD_PTR precisely because some
status command may return handles, e.g. MCI_DGV_STATUS_HWND or
MCI_DGV_STATUS_HPAL?

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|1.0.0                       |1.1.41

--- Comment #2 from Dmitry Timoshkov   2010-03-24 04:30:50 ---
Obviously your Wine version is not 1.0.0

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

Jörg Höhle  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27005|0                           |1
        is obsolete|                            |

--- Comment #3 from Jörg Höhle   2010-03-24 18:14:35 ---
Created an attachment (id=27027)
 --> (http://bugs.winehq.org/attachment.cgi?id=27027)
patch: use 64bit compatible structures with the MCI.

It would be nice if somebody with x64-64 could provide feedback on the patch.

What it does:
 - MCI_STATUS and MCI_GETDEVCAPS aka. "capability" ought to work in simple
cases, e.g. not "status x palette handle", to demonstrate that (some) offsets
are correctly computed.

What it does not:
 - conversion of return values >FFFFFFFF (e.g. handles)
 - handle MCI_HWND etc. "window x handle xyz" "open parent xyz" etc.
 - Hence "window x text hello" is broken.

BTW, did you know that the MCI string command parser is covered by a US patent
from IBM? I couldn't believe my eyes. Some sentence therein is 1:1 what's in
the MCI SDK.
http://www.freepatentsonline.com/6397263.html

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

--- Comment #4 from Jörg Höhle   2010-03-24 18:20:17 ---
Created an attachment (id=27028)
 --> (http://bugs.winehq.org/attachment.cgi?id=27028)
test case involving MCI_STATUS and MCI_GETDEVCAPS aka. capability

These tests pass on WTB's 64 bit MS boxes.

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

Eric Pouech  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #5 from Eric Pouech   2010-03-28 03:54:43 ---
Actually, we should check if the content of the MCI table in winmm.dll has been
changed on 64bit platforms (ie if we get something like you suggest about 32
and 64 bit integers described in this table).

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

Wylda  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #6 from Wylda   2010-03-28 04:17:32 ---
(In reply to comment #0)
> Hi,
>
> Wine's MCI string command parser is one reason why test.winehq.org shows
> failures with 64bit machines.

Hi, actually my experience is a bit different. The only test which does not
fail is 64bit with ie6. See:

http://test.winehq.org/data/719d904e351fa8fd1c7d1ebfbbae809ef27aa47c/index_X...
> It would be nice if somebody with x64-64 could provide feedback on the patch.

If you would melt that into wine test exe, i could give it a try on 64bit and
give feedback. Don't know if this is you are looking for ;) I mean i could try
that on WinXP x64 (but i could NOT on 64bit linux/wine).

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

--- Comment #7 from Jörg Höhle   2010-03-29 03:41:22 ---
Eric wrote:
> check if the content of the MCI table in winmm.dll
> has been changed on 64bit platforms

I'd be very pleased if somebody would supply that data.  Because a new
MCI_INTEGER64 type would make the parser more regular and simplify my patch.

Wylda wrote:
> If you would melt that into wine test exe

Go to WTB job 1156 before it gets deleted, you can download both patch and exe.
https://winetestbot.geldorp.nl/JobDetails.pl?Key=1156
> actually my experience is a bit different.

Your results are crazy indeed.  Perhaps MS auto-detects a 32bit driver and
sends plain old DWORD-sized objects instead?!? That also hints in the direction
of additional MCI_INTEGER64 types.
Austin English machine's shows exactly the crash that one expects from a
32/64bit pointer mismatch:
http://test.winehq.org/data/f7cc8f695b954bf98bd34bb7adccc09378349a1a/wine_ae...

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

--- Comment #8 from Jörg Höhle   2010-03-29 11:58:05 ---
Wylda confused me.
 - Native machines pass the tests, validating the current mmsystem and
digitalv.h structure definitions to some extent.
   Wylda's XP results are not surprising at all.
 - Wine64 machines crash, because winmm/mci.c is wrong for 64bit.

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

Jörg Höhle  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27027|0                           |1
        is obsolete|                            |

--- Comment #9 from Jörg Höhle   2010-04-15 07:05:33 ---
Created an attachment (id=27369)
 --> (http://bugs.winehq.org/attachment.cgi?id=27369)
patch: For MCI parsing, use 64bit compatible structures.

Here's the current version of the first patch. It could be simplified a little
by what I put in git meanwhile.

It's the first of three patches that should allow MCI to work on Wine64.
2. Fix offset calculation in MCI_CONSTANT (not yet written).
3. Introduce MCI_HWND etc. and other such DWORD_PTR sized objects (done).
(not mentioning tests).

The advantage of that approach is that a 4th one could be added anytime after
we learn whether or not MS introduced new constants for 64bit objects in 64bit
MCI drivers: there would be few things to change.

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

Jerome Leclanche  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #10 from Jerome Leclanche   2010-06-30 01:34:11 ---
Update on this bug?

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

--- Comment #11 from Jörg Höhle   2010-06-30 09:47:44 ---
The state is unchanged since my comment #3 and comment #9: 64bit support is
broken for apps that use MCI string commands.

I somehow convinced myself that the 3-4 patches are not trivial and therefore
not suitable for integration during code freeze, hence I invested my time
somewhere else meanwhile (testing apps rather than writing patches).
Am I wrong because 64bit support is the one goal for 1.2?

André Hentschel was so kind to validate on a 64bit system the patches that I
wrote so far, but they are still incomplete:
 - nr.2 from comment #9 still not written
 - nr.3 still contains one #ifdef WIN64 TODO
 - nr.4 missing extensive tests that specifically target the structures that
matter (e.g. those involving DWORD_PTR: MCI_HWND, MCI_HDC etc.) & validate
nothing was broken by the rewrite.  So far I only wrote tests for MCI_STATUS
(and MCI_GETDEVCAPS or MCI_INFO IIRC).

MCI_HWND, MCI_HDC and MCI_HPAL mostly occur with the mciavi and mciquartz
modules, for which there are currently no tests at all.  Such tests could be
written by a volunteer independently on the 64bit state.

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

--- Comment #12 from Jörg Höhle   2010-08-26 14:44:16 ---
Created an attachment (id=30414)
 --> (http://bugs.winehq.org/attachment.cgi?id=30414)
Use MCI_HWND & MCI_HDC in resource files

This patch introduces MCI_HWND, MCI_HDC and MCI_PALETTE in the
resource files.  The MCI parser still need be fixed to handle them.

In particular, the parser performs incorrect offset calculations
around the MCI_HWND element in the
L"handle\0", 0x00010000L, MCI_CONSTANT,
L"default\0", 0x00000000L, MCI_HWND,
L"\0", 0x00000000L, MCI_END_CONSTANT,
construct in MCI_DGV_WINDOWS_PARMS.  Obviously, the contents of the
CONSTANT block must be scanned to determine the offset of the union.
Don't try "window x text foobar" prior to that!

I'd be glad if Octavian would jump in.  I don't find time for a
rewrite but I've written many parser (unpublished) tests that show differences
between native and Wine.  My conclusion is that native's parser cannot
be implemented as the simple loop across the resources that Wine uses.
Yet I believe Wine's loop is enough for what apps require.

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

--- Comment #13 from Jörg Höhle  2010-10-02 02:25:26 CDT ---
W.r.t. patch numbering in comment #9, Alexandre reworked patch 1 for 1.3.4.
test.winehq shows that it's enough to make current MCI tests pass. Good news &
thanks, Alexandre.
patch 2 (correct offset computations) is still not written - any volunteer?
patch 3 is attached to comment #12 (however it predates the time when Maarten
copied mciavi_res.rc into mciqtz)

As I wrote to Octavian Voicu, a good testcase for patch 2 is
open foo.avi alias a
window a text "hello world"
info a window text => "hello world"
(oops, MCIAVI_mciInfo does not yet implement MCI_DGV_INFO_TEXT)

Writing a reasonable set of mciavi tests is still in my queue, but don't hold
your breath.

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

--- Comment #14 from Jerome Leclanche  2011-02-09 22:52:16 CST ---
(In reply to comment #13)
> W.r.t. patch numbering in comment #9, Alexandre reworked patch 1 for 1.3.4.
> test.winehq shows that it's enough to make current MCI tests pass. Good news &
> thanks, Alexandre.
> patch 2 (correct offset computations) is still not written - any volunteer?
> patch 3 is attached to comment #12 (however it predates the time when Maarten
> copied mciavi_res.rc into mciqtz)
>
> As I wrote to Octavian Voicu, a good testcase for patch 2 is
> open foo.avi alias a
> window a text "hello world"
> info a window text => "hello world"
> (oops, MCIAVI_mciInfo does not yet implement MCI_DGV_INFO_TEXT)
>
> Writing a reasonable set of mciavi tests is still in my queue, but don't hold
> your breath.

Saw some patches going in during October. Status on this bug?

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

--- Comment #15 from Jörg Höhle  2011-02-10 04:28:53 CST ---
Comment #13 still applies.  Anybody is welcome to rewrite the part of the
parser that incorrectly computes the offsets, aka. MCI_ParseOptArgs.

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

André H.  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

joaopa  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #16 from joaopa  2012-11-03 14:34:00 CDT ---
Still a bug in current wine?

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

http://bugs.winehq.org/show_bug.cgi?id=22146

--- Comment #17 from Jörg Höhle  2012-11-05 04:38:45 CST ---
Git history shows that nothing changed:
http://source.winehq.org/git/wine.git/history/HEAD:/dlls/winmm/mci.c
Anybody - preferably a user of a 64bit system - is invited to try and fix 64bit
MCI parser support, see comment #13.

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

https://bugs.winehq.org/show_bug.cgi?id=22146

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |download, testcase

[Bug 22146] 64bit compatibility issue with Wine's MCI string command parser

https://bugs.winehq.org/show_bug.cgi?id=22146

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]