Re: [v4 3/4] ntdll: Save rdi and rsi in raise_func_trampoline.

[Sebastian Lackner]

On 12.02.2017 23:41, Andrew Wesie wrote:
> On Windows, RDI and RSI are callee-saved registers, but on Linux
> they are caller-saved registers. As such, raise_func_trampoline
> needs to explicitly unwind them.
> Signed-off-by: Andrew Wesie
>
> dlls/ntdll/signal_x86_64.c   | 12 +++++++++---
>  dlls/ntdll/tests/exception.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/ntdll/signal_x86_64.c b/dlls/ntdll/signal_x86_64.c
> index d2b040e..2370cf4 100644
> --- a/dlls/ntdll/signal_x86_64.c
> +++ b/dlls/ntdll/signal_x86_64.c
> @@ -2092,9 +2092,11 @@ NTSTATUS context_from_server( CONTEXT *to, const context_t *from )
>  extern void raise_func_trampoline( EXCEPTION_RECORD *rec, CONTEXT *context, raise_func func );
>  __ASM_GLOBAL_FUNC( raise_func_trampoline,
>                     __ASM_CFI(".cfi_signal_frame\n\t")
> -
>               __ASM_CFI(".cfi_def_cfa %rbp,144\n\t")  /* red zone + rip + rbp */
> -
>               __ASM_CFI(".cfi_rel_offset %rip,8\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %rbp,0\n\t")
> -
>               __ASM_CFI(".cfi_def_cfa %rbp,160\n\t")  /* red zone + rip + rbp + rdi + rsi */
> -
>               __ASM_CFI(".cfi_rel_offset %rip,24\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %rbp,16\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %rdi,8\n\t")
> -
>               __ASM_CFI(".cfi_rel_offset %rsi,0\n\t")
>               "call *%rdx\n\t"
>               "int $3")
>
> @@ -2111,6 +2113,8 @@ static EXCEPTION_RECORD *setup_exception( ucontext_t *sigcontext, raise_func fun
>      {
>          CONTEXT           context;
>          EXCEPTION_RECORD  rec;
> -
>    ULONG64           rsi;
> -
>    ULONG64           rdi;
>    ULONG64           rbp;
>    ULONG64           rip;
>    ULONG64           red_zone[16];
>
> @@ -2180,6 +2184,8 @@ static EXCEPTION_RECORD *setup_exception( ucontext_t *sigcontext, raise_func fun
>      rsp_ptr = (ULONG64 *)RSP_sig(sigcontext) - 16;
>      *(--rsp_ptr) = RIP_sig(sigcontext);
>      *(--rsp_ptr) = RBP_sig(sigcontext);
> -
>
> *(--rsp_ptr) = RDI_sig(sigcontext);
> -
>
> *(--rsp_ptr) = RSI_sig(sigcontext);
>
> /* now modify the sigcontext to return to the raise function */
> RIP_sig(sigcontext) = (ULONG_PTR)raise_func_trampoline;
>
> diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c
> index 183cf18..15973ce 100644
> --- a/dlls/ntdll/tests/exception.c
> +++ b/dlls/ntdll/tests/exception.c
> @@ -2073,6 +2073,47 @@ static DWORD run_exception_test(void *handler, const void* context,
>      return result;
>  }
>
> +static DWORD WINAPI unwind_rdi_rsi_handler( EXCEPTION_RECORD *rec, ULONG64 frame,
> -
>                                        CONTEXT *context, DISPATCHER_CONTEXT *dispatcher )
>
> +{
> - if (rec->ExceptionCode != STATUS_BREAKPOINT)
> -
>    return ExceptionContinueSearch;
> -
> - RtlUnwind( (PVOID)context->Rsp, (PVOID)(context->Rip + 1), rec, NULL );

Doesn't this suffer from the same problem as before? We might not have unwinding information
for the unwind_rdi_rsi_handler function if it is not supported by all x86_64 compilers.
Actually, I believe the change itself would also be acceptable even without tests.
> - return ExceptionContinueSearch;
>
> +}
> -
>
> +static const BYTE unwind_rdi_test_code[] = {
> -
>    0x57,                                           /* push %rdi */
> -
>    0x48, 0xc7, 0xc7, 0x55, 0x55, 0x55, 0x55,       /* mov $0x55555555, %rdi */
> -
>    0xcc,                                           /* int3 */
> -
>    0x48, 0x89, 0xf8,                               /* mov %rdi, %rax */
> -
>    0x5f,                                           /* pop %rdi */
> -
>    0xc3,                                           /* ret */
>
> +};
> -
>
> +static const BYTE unwind_rsi_test_code[] = {
> -
>    0x56,                                           /* push %rsi */
> -
>    0x48, 0xc7, 0xc6, 0x33, 0x33, 0x33, 0x33,       /* mov $0x33333333, %rsi */
> -
>    0xcc,                                           /* int3 */
> -
>    0x48, 0x89, 0xf0,                               /* mov %rsi, %rax */
> -
>    0x5e,                                           /* pop %rsi */
> -
>    0xc3,                                           /* ret */
>
> +};
> -
>
> +static void test_unwind_rdi_rsi(void)
> +{
> - DWORD result;
> -
> - result = run_exception_test(unwind_rdi_rsi_handler, NULL, unwind_rdi_test_code,
> -
>        sizeof(unwind_rdi_test_code), 0);
> - ok( result == 0x55555555, "expected %x, got %x\n", 0x55555555, result );
> -
> - result = run_exception_test(unwind_rdi_rsi_handler, NULL, unwind_rsi_test_code,
> -
>        sizeof(unwind_rsi_test_code), 0);
> - ok( result == 0x33333333, "expected %x, got %x\n", 0x33333333, result );
>
> +}
> -
>
> #endif  /* __x86_64__ */
>
> #if defined(__i386__) || defined(__x86_64__)
> @@ -2653,6 +2694,7 @@ START_TEST(exception)
>      test_virtual_unwind();
>      test___C_specific_handler();
>      test_restore_context();
> -
>
> test_unwind_rdi_rsi();
>
> if (pRtlAddFunctionTable && pRtlDeleteFunctionTable && pRtlInstallFunctionTableCallback && pRtlLookupFunctionEntry)
>   test_dynamic_unwind();
>
> -- 2.7.4

[v4 3/4] ntdll: Save rdi and rsi in raise_func_trampoline.

[Andrew Wesie]

On Mon, Feb 13, 2017 at 10:47 AM, Sebastian Lackner 
wrote:
> Doesn't this suffer from the same problem as before? We might not have
> unwinding information
> for the unwind_rdi_rsi_handler function if it is not supported by all
> x86_64 compilers.
> Actually, I believe the change itself would also be acceptable even
> without tests.

Correct, I was resubmitting the patch set primarily for the debug registers
patch. I didn't change this patch, because I wasn't sure how to avoid the
issue since the whole point of the test is to do unwinding of the stack.

When I resubmit the patch set, I will exclude this test. The primary
purpose was just to show that this is an actual bug and the patch fixes it.

-Andrew