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

[Rémi Bernon]

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.

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.

There's then still quite a lot of Wine exception handlers being pushed / 
poped (IsBadString, C++ dynamic casts, etc...), though they don't 
usually callback any user code.

-- 
Rémi Bernon <rbernon at codeweavers.com>