[PATCH] ntdll: Only call wine exception handlers on the current stack.

[Paul Gofman]

On 2/1/22 15:49, Rémi Bernon wrote:
> On 2/1/22 13:25, Paul Gofman wrote:
>> On 2/1/22 14:54, Bernhard Übelacker wrote:
>>> Am 31.01.22 um 16:24 schrieb Rémi Bernon:
>>>> MK11 creates an alternate stack and sometimes throws an exception 
>>>> which
>>>> gets incorrectly handled by a Wine exception handler, causing the game
>>>> to crash.
>>>>
>>>> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
>>>> ---
>>>>   dlls/ntdll/signal_x86_64.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/dlls/ntdll/signal_x86_64.c b/dlls/ntdll/signal_x86_64.c
>>>> index 7e77329363c..36985832e4a 100644
>>>> --- a/dlls/ntdll/signal_x86_64.c
>>>> +++ b/dlls/ntdll/signal_x86_64.c
>>>> @@ -463,7 +463,9 @@ static NTSTATUS call_stack_handlers( 
>>>> EXCEPTION_RECORD *rec, CONTEXT *orig_contex
>>>>               }
>>>>           }
>>>>           /* hack: call wine handlers registered in the tib list */
>>>> -        else while ((ULONG64)teb_frame < context.Rsp)
>>>> +        else while ((ULONG64)teb_frame < context.Rsp &&
>>>> +                    (ULONG64)teb_frame >= 
>>>> (ULONG64)NtCurrentTeb()->Tib.StackLimit &&
>>>> +                    (ULONG64)teb_frame <= 
>>>> (ULONG64)NtCurrentTeb()->Tib.StackBase)
>>>>           {
>>>>               TRACE_(seh)( "found wine frame %p rsp %p handler %p\n",
>>>>                            teb_frame, (void *)context.Rsp, 
>>>> teb_frame->Handler );
>>>
>>> I am not sure but this seems kind of similar to what I think I found in
>>> this bug: https://bugs.winehq.org/show_bug.cgi?id=52159
>>>
>> Yes, that looks quite similar. All these fix attempts workaround the 
>> problem where it is first visibly hit (calling wrong handler) but 
>> ignore what may happen to these handlers after. What if the app, e. 
>> g., switches stack back? And RtlUnwindEx()?
>>
>> Actually the majority of Wine __TRY blocks should have rather low 
>> chances to hit app's stack switch in between the handler is installed 
>> and removed (unless the app is doing something really fun with 
>> hotpatching). Those IsBad..Ptr(), a bunch of cases when the parameter 
>> access is wrapped into __TRY, OutputDebugString() are not much likely 
>> to hit such an issue. There are relatively few __TRY blocks which 
>> embrace application functions: calling thread func, calling DLL entry 
>> points, exception handlers calls in signal_x86_64.c (and maybe I 
>> missed some). Since you probably hit the issue with Proton that is 
>> not the nested exception handlers as those in Proton already have 
>> manual asm wrappers to use straightforward x64 .seh handling and 
>> don't install Teb handlers. If maybe there are just one or two of 
>> other "global" handlers which are problematic maybe it worth solving 
>> the issue with some sort of manual .seh specifically for those ones?
>>
>
> In the case of MK11, the wine handler is the one installed by 
> RtlUserThreadStart.

Yes, RtlUserThreadStart() is the one which will always be there. So if 
we replace that single one with an asm wrapper (and there is already a 
wrapper for i386) we will likely solve the big share of practical issues 
in a straightforward way. I can make such a patch (unless you want to do 
it?). Not sure though if that is good enough though, the prior 
understanding was that these sort of fixes are waiting for mingw .seh 
support.

I suppose MK11 will work this way (e. g., works with just removing __TRY 
in RtlUserThreadStart())?


>
> I don't see anywhere any clue about how the thread is switching 
> stacks, it just happens to use alternate stacks after a while, and has 
> actually quite a few different stacks, probably re-implementing its 
> own fibers (it doesn't call SwitchToFiber).
>
> When they switch stacks they don't change the ExceptionList pointer 
> though (probably as I understand it because it's not supposed to be 
> used on Win64), so it's always ultimately pointing to the initial 
> handler on the initial stack.
Yes, sure, they should not, ExceptionList on x64 is a Wine thing.