[Bug 56260] New: 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

Bug ID: 56260
           Summary: 16-bit Myst deadlocks when entering Book
           Product: Wine
           Version: 9.1
          Hardware: x86-64
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: -unknown
          Assignee: [e-mail]
          Reporter: [e-mail]
      Distribution: ---

First, work around bug 56225.

- Run winecfg and mount the Myst/cd folder as D:\
- Run `sudo sysctl vm.mmap_min_addr=0`
- In one terminal, run `Xephyr :2 -ac -screen 800x600x8`
- In another terminal, run `DISPLAY=:2 openbox`
- In a third terminal, cd to "Myst/win3.1/MYST/" and run `DISPLAY=:2 wine
'MYST.EXE'`

Click through the intro and into the image in the book, this should get you
into the first world. On wine it just plays a sound, and then locks up.

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

--- Comment #1 from Fabian Maurer  ---
Created attachment 75971
  --> https://bugs.winehq.org/attachment.cgi?id=75971
Win16 code that it is stuck in

When the deadlock occurs, winevdm uses one thread fully. Attaching the
disassembly that is continuously run. As I see it, it just waits for a variable
to be written. This variable should (AFAIK) be written when the waveOutOpen
callback gets called with WOM_DONE.

Not exactly sure how that is supposed to work with cooperative multitasking,
but I suspect that is an interrupt that allows that to work.

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

--- Comment #2 from Fabian Maurer  ---
Created attachment 75972
  --> https://bugs.winehq.org/attachment.cgi?id=75972
Test program to reproduce the issue

Attaching a test program that does what I think Myst is doing. Compile using
OpenWatcom (command included in file).

For some reason, I get "invalid parameter" on my XP VM, Win3.1 Dosbox and Win
2000 VM... But another user in IRC tested on a real Win98 Machine and it works.

It hangs in Wine though, so I do think this is the issue. On Wine the WOM_DONE
message is never sent because the thread tries to get the Win16 lock mutex, but
since one thread is currently running that fails and waits forever.

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

--- Comment #3 from Fabian Maurer  ---
Created attachment 75973
  --> https://bugs.winehq.org/attachment.cgi?id=75973
Patch to avoid deadlock

Attaching a patch to avoid the deadlock. Not 100% sure this is the right thing
to do, but my example serves as evidence that the callback should be possible
even if the program is spinning endlessly.

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

Alex Henrie  changed:

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

--- Comment #4 from Alex Henrie  ---
You did an amazing job debugging this, thank you Fabian!

Instead of using a mutex to ensure that only one 16-bit thread is running at a
time, I wonder if we could make the callback pause/suspend the original thread
to simulate what would happen on Windows 3.1 during an interrupt.

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

--- Comment #5 from Fabian Maurer  ---
Created attachment 75991
  --> https://bugs.winehq.org/attachment.cgi?id=75991
Second Testprogram to reproduce the issue

I found a way to work around the "Invalid parameter" issue, attaching a new
testprogram that hacks around the issue (Win16 progamming is a massive PITA,
did I ever say that?)
Anyways, this should work at least on XP with original ntvdm.
> Instead of using a mutex to ensure that only one 16-bit thread is running at a time,
> I wonder if we could make the callback pause/suspend the original thread
> to simulate what would happen on Windows 3.1 during an interrupt.

Sounds at least sensible to me, after all (if I understand things right) 16bit
programs allow only one running thread and having it run code outside of where
it's expected could lead to issues.

Btw, not sure if that is obvious, but I couldn't use
ReleaseThunkLock/RestoreThunkLock because that thread doesn't own the Win16
Mutex so it can't release it.

Could also be interesting to see how otvdm does it, from what I saw they also
employ a mutex, but that sample (and MYST) work there. Maybe there's something
to be learned here?

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

--- Comment #6 from Alex Henrie  ---
Created attachment 76136
  --> https://bugs.winehq.org/attachment.cgi?id=76136
[PATCH] krnl386: Pause other tasks while executing a callback.

Hi Fabian, I put together a patch to emulate 16-bit hardware interrupts in Wine
by suspending other task threads while executing the callback. Could you take a
look before I send it upstream?

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

--- Comment #7 from Fabian Maurer  ---
Comment on attachment 75991
  --> https://bugs.winehq.org/attachment.cgi?id=75991
Second Testprogram to reproduce the issue

/* WATCOM=/opt/watcom PATH=$WATCOM/binl:$PATH INCLUDE=$WATCOM/h:$WATCOM/h/win
LIB=$WATCOM/lib286:$WATCOM/lib286/dos wcl -bcl=windows main.c -q mmsystem.lib
-zu */

#include <windows.h>
#include <stddef.h>
#include <stdlib.h>
#include <stdio.h>
#include <mmsystem.h>
#include <i86.h>

/* Error handling */

void error(const char* str)
{
    MessageBox((HWND)0, str, "", 0);
    exit(1);
}

void errori(const char* str, DWORD i)
{
    static char t[20];
    sprintf(t, "%s %lx", str, i);
    error(t);
}

void mesi(const char* str, DWORD i)
{
    static char t[20];
    sprintf(t, "%s %lx", str, i);
    MessageBox((HWND)0, t, "", 0);
}

void mes(const char* str)
{
    MessageBox((HWND)0, str, "", 0);
}

/* HOOK Stuff */

typedef struct
{
    BYTE code_original[5];
    BYTE code_hooked[5];
    BOOL hooked;
    LPBYTE address;
    UINT selector;
    FARPROC callback;
    HGLOBAL hhook;
} _hook_info;
typedef _hook_info FAR* hook_info;

typedef UINT (WINAPI* tAllocCsToDsAlias)(UINT);

hook_info hook_init(HINSTANCE instance, const char* name_lib, const char*
name_func, FARPROC hook_func)
{
    HMODULE mod_kernel, mod_lib;
    FARPROC func;
    tAllocCsToDsAlias pAllocCsToDsAlias;
    int i;
    HGLOBAL hhook;
    hook_info hook;

hhook = GlobalAlloc(GMEM_MOVEABLE, sizeof(_hook_info));
    hook = (hook_info)GlobalLock(hhook);
    hook->hhook = hhook;
    mod_kernel = GetModuleHandle("KERNEL");
    mod_lib = GetModuleHandle(name_lib);

pAllocCsToDsAlias =
(tAllocCsToDsAlias)GetProcAddress(mod_kernel,"AllocCsToDsAlias");
    if (!pAllocCsToDsAlias)
    {
        error("AllocCsToDsAlias missing");
    }

func = GetProcAddress(mod_lib, name_func);
    if (!func)
    {
        error("Can't get original func");
    }

hook->callback = MakeProcInstance((FARPROC)hook_func, instance);
    if (hook->callback == NULL || hook->callback == hook_func)
    {
        error("MakeProcInstance failed");
    }

hook->hooked = FALSE;
    hook->selector = pAllocCsToDsAlias(FP_SEG(func));
    hook->address = (LPBYTE)MK_FP(hook->selector, FP_OFF(func));
    if (hook->address[0] == 0xEA)
    {
        error("Already hooked, something went terribly wrong!");
    }
    hook->code_hooked[0] = 0xEA;
    *(LPDWORD)(&hook->code_hooked[1]) = (DWORD)hook->callback;

for (i = 0; i< 5; i++)
        hook->code_original[i] = hook->address[i];

return hook;
}

void hook_enable(hook_info hook)
{
    int i;
    if (hook->hooked)
        return;

for (i = 0; i < 5; i++)
        hook->address[i] = hook->code_hooked[i];
    hook->hooked = TRUE;
}

void hook_disable(hook_info hook)
{
    int i;
    if (!hook->hooked)
        return;

for (i = 0; i < 5; i++)
        hook->address[i] = hook->code_original[i];
    hook->hooked = FALSE;
}

void hook_free(hook_info hook)
{
    HGLOBAL hhook;
    hook_disable(hook);
    FreeSelector(hook->selector);
    FreeProcInstance(hook->callback);
    hhook = hook->hhook;
    GlobalUnlock(hhook);
    GlobalFree(hhook);
}

/* Main code */

static hook_info hook;

static int done = 0;

void CALLBACK _export waveOutProc(HWAVEOUT hwo, UINT uMsg, DWORD dwInstance,
DWORD dwParam1, DWORD dwParam2)
{
    if (uMsg == WOM_DONE)
    {
        done = 1;
    }
}

int WINAPI _export HookGetCodeInfoFunc(FARPROC proc, SEGINFO FAR* seginfo)
{
    hook_disable(hook);
    GetCodeInfo(proc, seginfo);
    hook_enable(hook);

seginfo->flags &= ~(0x10);
    return 1;
}

BOOL IsWine(void)
{
    HMODULE kernel = GetModuleHandle("KERNEL");
    FARPROC proc = GetProcAddress(kernel, "__wine_call_int_handler");
    return proc != NULL;
}

int PASCAL WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR
lpCmdLine, int nCmdShow)
{
    HWAVEOUT hout;
    PCMWAVEFORMAT format;
    WAVEHDR NEAR* block;
    int result;
    FARPROC callback;
    char NEAR* buffer;
    OFSTRUCT ofstruct;
    HFILE file;

if (!IsWine())
    {
        hook = hook_init(hInstance, "KERNEL", "GetCodeInfo",
(FARPROC)HookGetCodeInfoFunc);
        hook_enable(hook);
    }

if ((file = OpenFile("test.wav", &ofstruct, OF_READ)) == HFILE_ERROR) {
        error("CreateFile failed");
    }

block = (WAVEHDR NEAR*)LocalAlloc(0, 8000 + sizeof(WAVEHDR));

buffer = (char NEAR*)block + sizeof(WAVEHDR);
    block->dwBufferLength = 8000;
    block->lpData = buffer;

if (_lread(file, buffer, 8000) == HFILE_ERROR)
    {
        error("ReadFile failed");
    }

format.wf.wFormatTag = WAVE_FORMAT_PCM;
    format.wf.nChannels = 1;
    format.wf.nSamplesPerSec = 11025;
    format.wf.nBlockAlign = 1;
    format.wf.nAvgBytesPerSec = 11025;
    format.wBitsPerSample = 8;

callback = MakeProcInstance((FARPROC)waveOutProc, hInstance);
    if (callback == NULL)
    {
        error("MakeProcInstance failed");
    }

if ((result = waveOutOpen(&hout, WAVE_MAPPER, (LPWAVEFORMAT)&format,
(DWORD)callback, 0, CALLBACK_FUNCTION)) != MMSYSERR_NOERROR)
    {
        errori("waveOutOpen failed", result);
    }
    if (!IsWine())
    {
        hook_free(hook);
    }
    waveOutPrepareHeader(hout, block, sizeof(WAVEHDR));
    waveOutWrite(hout, block, sizeof(WAVEHDR));
    while(!done)
    {
    }
    error("success!");
    return 0;
}

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

Fabian Maurer  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #75972|0                           |1
        is obsolete|                            |
  Attachment #75991|0                           |1
        is obsolete|                            |

--- Comment #8 from Fabian Maurer  ---
Created attachment 76137
  --> https://bugs.winehq.org/attachment.cgi?id=76137
Second Testprogram to reproduce the issue v2

Wanted to edit the attachment, guess that just created a comment-spam. sorry
about that. Here the fixed testprogram.

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

--- Comment #9 from Fabian Maurer  ---
> Hi Fabian, I put together a patch to emulate 16-bit hardware interrupts in
> Wine by suspending other task threads while executing the callback. Could
> you take a look before I send it upstream?

Sure, thanks!

I find the wowthunk diff hard to follow (that's why I tend to add extra commits
just to fix indentation/names), but as I understand it you replace the lock
with a suspension of threads instead.

Questions:
- If we don't have an interrupt, in WOWTHUNK_CallWithRegs, why don't we use the
lock instead?
- I don't really understand the WCB16_REGS flag yet, what's the difference?
- in task_start can't we move the assignment to teb before the TASK_LinkTask
and then let TASK_LinkTask/TASK_UnLinkTask handle the critical section
themselves? If I understand correctly, you only have that outside to guard
against therace condition where task is linked but teb not assigned.

Apart from not understanding the interactions with the rest of the Win16
system, this looks good to me.

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

--- Comment #10 from Alex Henrie  ---
(In reply to Fabian Maurer from comment #9)
> I find the wowthunk diff hard to follow (that's why I tend to add extra
> commits just to fix indentation/names), but as I understand it you replace
> the lock with a suspension of threads instead.

I find it's easier to commit the patch and then look at it with `git show -w`.
On the surface the patch seems a lot more invasive than it is.
> - If we don't have an interrupt, in WOWTHUNK_CallWithRegs, why don't we use
>
> the lock instead?

The only function that calls WOWTHUNK_CallWithRegs with interrupt=FALSE is
NE_StartTask, and the only function that calls NE_StartTask is task_start,
which already locks Win16Mutex for the duration of the call. It didn't seem
necessary to lock the mutex again, which would simply increment the lock count.
Do you see anything that might depend on that side effect?
> - I don't really understand the WCB16_REGS flag yet, what's the difference?

I couldn't find any documentation about it, but it evidently restores a whole
set of saved registers instead of doing an ordinary function call. For example,
WCB16_REGS would let you jump to the middle of a function instead of the
beginning.
> - in task_start can't we move the assignment to teb before the TASK_LinkTask
>
> and then let TASK_LinkTask/TASK_UnLinkTask handle the critical section
> themselves? If I understand correctly, you only have that outside to guard
> against therace condition where task is linked but teb not assigned.

I thought about that, but SetPriority16 needs to remove a running task from the
list and then add it back to the end of the list, and Task_SuspendAll needs all
potentially running tasks to be in the list at all times. So, SetPriority16
can't unlock task_list_cs in between TASK_UnlinkTask and TASK_LinkTask.
Similarly, TASK_CreateMainTask needs to hold the lock in between when it starts
the main task and when it adds it to the list. In task_start we could set
pTask->teb before locking task_list_cs, but there doesn't seem to be much to
gain one way or the other.
> Apart from not understanding the interactions with the rest of the Win16
> system, this looks good to me.

Thanks for looking it over!

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

--- Comment #11 from Fabian Maurer  ---
> The only function that calls WOWTHUNK_CallWithRegs with interrupt=FALSE is
> NE_StartTask, and the only function that calls NE_StartTask is task_start,
> which already locks Win16Mutex for the duration of the call. It didn't seem
> necessary to lock the mutex again, which would simply increment the lock
> count. Do you see anything that might depend on that side effect?

Not really, but I'm missing the bigger picture. Win16 lock has the weird
behavior that can be gotten multiple times recursively, but I don't know how
important the actual count is. Probably not too important.
> I thought about that, but SetPriority16 needs to remove a running task from
> the list and then add it back to the end of the list, and Task_SuspendAll
> needs all potentially running tasks to be in the list at all times. So,
> SetPriority16 can't unlock task_list_cs in between TASK_UnlinkTask and
> TASK_LinkTask. Similarly, TASK_CreateMainTask needs to hold the lock in
> between when it starts the main task and when it adds it to the list. In
> task_start we could set pTask->teb before locking task_list_cs, but there
> doesn't seem to be much to gain one way or the other.

Ah, I see.

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

Alex Henrie  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://bugs.winehq.org/sho
                   |                            |w_bug.cgi?id=56225
             Status|NEW                         |RESOLVED
      Fixed by SHA1|                            |ff0fbb1ff5f76d1d206aea45290
                   |                            |7ad2dd29206ea
         Resolution|---                         |FIXED

--- Comment #12 from Alex Henrie  ---
Fixed by:
https://gitlab.winehq.org/wine/wine/-/commit/e7b396428f44206b0b388f524bca0dd...
https://gitlab.winehq.org/wine/wine/-/commit/ff0fbb1ff5f76d1d206aea452907ad2...

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #13 from Alexandre Julliard  ---
Closing bugs fixed in 10.2.

[Bug 56260] 16-bit Myst deadlocks when entering Book

[WineHQ Bugzilla]

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

Alex Henrie  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://bugs.winehq.org/sho
                   |                            |w_bug.cgi?id=57974