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

[Rémi Bernon]

On 2/1/22 13:57, Paul Gofman wrote:
> 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.
> 

Well if it's better than checking the handler location go for it, I'm 
not sure to see what that wrapper would need to be doing.

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

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