[PATCH 14/18] ntdll: Don't use NtSetContextThread in NtContinue.

[Jacek Caban]

On 05.02.2021 03:43, Zebediah Figura (she/her) wrote:
> On 1/22/21 9:51 AM, Jacek Caban wrote:
>>
>> NtSetContextThread, unlike NtContinue, should not set nonvolatile 
>> registers.
>>
>> Signed-off-by: Jacek Caban <jacek at codeweavers.com>
>> ---
>>    dlls/ntdll/unix/server.c        |  2 +-
>>    dlls/ntdll/unix/signal_arm.c    | 18 +++++++++++++-----
>>    dlls/ntdll/unix/signal_arm64.c  | 20 ++++++++++++++------
>>    dlls/ntdll/unix/signal_i386.c   | 14 +++++++++++++-
>>    dlls/ntdll/unix/signal_x86_64.c | 15 ++++++++++++++-
>>    dlls/ntdll/unix/unix_private.h  |  1 +
>>    6 files changed, 56 insertions(+), 14 deletions(-)
>>
>>
>
> Just to clarify—our NtSetContextThread does set nonvolatile registers, 
> and continues to do so after this series. Is that incorrect behaviour?


No, it does not continue to do so after this series. This is covered by 
the test at the end of the series. NtSetContextThread does set 
nonvolatile registers in syscall frame so that if you set them and then 
query them before syscall returns, you will get modified values. 
However, syscall dispatcher does not restore them from syscall frame. It 
means that if you call NtSetContectThread on current thread, it will not 
set nonvolatile registers to passed values.


> Also:
>
>> diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c
>> index db09d7759da..fe581cca7b2 100644
>> --- a/dlls/ntdll/unix/server.c
>> +++ b/dlls/ntdll/unix/server.c
>> @@ -726,7 +726,7 @@ NTSTATUS WINAPI NtContinue( CONTEXT *context, 
>> BOOLEAN alertable )
>>          status = server_select( NULL, 0, SELECT_INTERRUPTIBLE | 
>> SELECT_ALERTABLE, 0, NULL, NULL, &apc );
>>          if (status == STATUS_USER_APC) invoke_apc( context, &apc );
>>      }
>> -    return NtSetContextThread( GetCurrentThread(), context );
>> +    signal_set_cpu_context( context );
>>  }
>>
>>
>
> Would it be simpler just to move NtContinue() to signal_*.c instead?


We'd then need to expose invoke_apc somehow, so I'm not sure if that 
would be better.


> One further question regarding this patch series / patch 0017: I 
> assume you're also planning to also change NtContinue() to use the 
> syscall exit code instead of set_full_cpu_context()?


I considered it. My very first prototype worked like that because it 
always restored all registers, except for rax/eax which becomes a bit 
tricky for that use case. I was thinking about providing an alternative 
syscall leave implementation (NtContinue could replace its own return 
address to opt-in to this), which would do full restore. I didn't try 
implementing that variant yet to see if it's worth it.


For potential future plans, there are also call_user_apc_dispatcher and 
call_user_exception_dispatcher. They could potentially use the same 
mechanism as well instead of manually implementing crossing syscall 
barrier. They are more tricky to change, because we'd need to allow 
using stack is a more similar way to actual syscalls first. A simple 
solution would be to preserve enough unused stack on syscall entry, but 
a more complete solution like separated stacks could be even better.


BTW, as far as this series is considered, I think we will want a few 
specialized implementations for different CPU generations, so I plan to 
look at moving syscall dispatcher code from winebuild to ntdll first. 
ntdll.so would then set proper dispatcher depending on CPU capabilities. 
That requires changes to the way we access syscall table. I didn't do 
the actual implementation yet.


Jacek