[PATCH 0/1] MR2794: ntdll: add attribute "force_align_arg_pointer" to hanlder of signal on i386 & x86_64
For example, on x86_64, value of sp on begin of callee must be 16n-8, because sp was 16n before instruction "call" on caller. But initilize value of sp on signal stack is 16n instead of 16n-8. It may cause error when store and load xmm register. add "force_align_arg_pointer" to hanlder of signal can avoid potential risks -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2794
From: Fan WenJie <fanwj(a)mail.ustc.edu.cn> For example, on x86_64, value of sp on begin of callee must be 16n-8, because sp was 16n before instruction "call" on caller. But initilize value of sp on signal stack is 16n instead of 16n-8. It may cause error when store and load xmm register. add "force_align_arg_pointer" to hanlder of signal can avoid potential risks --- dlls/ntdll/unix/signal_i386.c | 14 +++++++------- dlls/ntdll/unix/signal_x86_64.c | 14 +++++++------- include/winnt.h | 6 ++++++ 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/dlls/ntdll/unix/signal_i386.c b/dlls/ntdll/unix/signal_i386.c index 89af605443d..cd554fdf985 100644 --- a/dlls/ntdll/unix/signal_i386.c +++ b/dlls/ntdll/unix/signal_i386.c @@ -1853,7 +1853,7 @@ static BOOL handle_syscall_trap( ucontext_t *sigcontext ) * * Handler for SIGSEGV and related errors. */ -static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { EXCEPTION_RECORD rec = { 0 }; struct xcontext xcontext; @@ -1943,7 +1943,7 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) * * Handler for SIGTRAP. */ -static void trap_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void trap_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { EXCEPTION_RECORD rec = { 0 }; struct xcontext xcontext; @@ -1989,7 +1989,7 @@ static void trap_handler( int signal, siginfo_t *siginfo, void *sigcontext ) * * Handler for SIGFPE. */ -static void fpe_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void fpe_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { EXCEPTION_RECORD rec = { 0 }; struct xcontext xcontext; @@ -2034,7 +2034,7 @@ static void fpe_handler( int signal, siginfo_t *siginfo, void *sigcontext ) * * Handler for SIGINT. */ -static void int_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void int_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { HANDLE handle; @@ -2051,7 +2051,7 @@ static void int_handler( int signal, siginfo_t *siginfo, void *sigcontext ) * * Handler for SIGABRT. */ -static void abrt_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void abrt_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { EXCEPTION_RECORD rec = { EXCEPTION_WINE_ASSERTION, EH_NONCONTINUABLE }; @@ -2064,7 +2064,7 @@ static void abrt_handler( int signal, siginfo_t *siginfo, void *sigcontext ) * * Handler for SIGQUIT. */ -static void quit_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void quit_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { init_handler( sigcontext ); abort_thread(0); @@ -2076,7 +2076,7 @@ static void quit_handler( int signal, siginfo_t *siginfo, void *sigcontext ) * * Handler for SIGUSR1, used to signal a thread that it got suspended. */ -static void usr1_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void usr1_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { struct xcontext xcontext; diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 97d3a009c03..eb6ddad3e8c 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -1962,7 +1962,7 @@ static BOOL handle_syscall_trap( ucontext_t *sigcontext ) * * Handler for SIGSEGV and related errors. */ -static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { EXCEPTION_RECORD rec = { 0 }; struct xcontext context; @@ -2049,7 +2049,7 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext ) * * Handler for SIGTRAP. */ -static void trap_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void trap_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { EXCEPTION_RECORD rec = { 0 }; struct xcontext context; @@ -2083,7 +2083,7 @@ static void trap_handler( int signal, siginfo_t *siginfo, void *sigcontext ) * * Handler for SIGFPE. */ -static void fpe_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void fpe_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { EXCEPTION_RECORD rec = { 0 }; ucontext_t *ucontext = init_handler( sigcontext ); @@ -2136,7 +2136,7 @@ static void fpe_handler( int signal, siginfo_t *siginfo, void *sigcontext ) * * Handler for SIGINT. */ -static void int_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void int_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { ucontext_t *ucontext = init_handler( sigcontext ); HANDLE handle; @@ -2156,7 +2156,7 @@ static void int_handler( int signal, siginfo_t *siginfo, void *sigcontext ) * * Handler for SIGABRT. */ -static void abrt_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void abrt_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { ucontext_t *ucontext = init_handler( sigcontext ); EXCEPTION_RECORD rec = { EXCEPTION_WINE_ASSERTION, EH_NONCONTINUABLE }; @@ -2170,7 +2170,7 @@ static void abrt_handler( int signal, siginfo_t *siginfo, void *sigcontext ) * * Handler for SIGQUIT. */ -static void quit_handler( int signal, siginfo_t *siginfo, void *ucontext ) +static DECLSPEC_FORCEALIGN void quit_handler( int signal, siginfo_t *siginfo, void *ucontext ) { init_handler( ucontext ); abort_thread(0); @@ -2182,7 +2182,7 @@ static void quit_handler( int signal, siginfo_t *siginfo, void *ucontext ) * * Handler for SIGUSR1, used to signal a thread that it got suspended. */ -static void usr1_handler( int signal, siginfo_t *siginfo, void *sigcontext ) +static DECLSPEC_FORCEALIGN void usr1_handler( int signal, siginfo_t *siginfo, void *sigcontext ) { ucontext_t *ucontext = init_handler( sigcontext ); struct xcontext context; diff --git a/include/winnt.h b/include/winnt.h index 64408126cba..50c5d24d70e 100644 --- a/include/winnt.h +++ b/include/winnt.h @@ -83,6 +83,12 @@ extern "C" { # endif #endif +#if (defined(__i386__) || defined(__x86_64__)) && defined(__GNUC__) +#ifndef DECLSPEC_FORCEALIGN +#define DECLSPEC_FORCEALIGN __attribute__((force_align_arg_pointer)) +#endif +#endif + #ifndef DECLSPEC_NOTHROW # if defined(_MSC_VER) && (_MSC_VER >= 1200) && !defined(MIDL_PASS) # define DECLSPEC_NOTHROW __declspec(nothrow) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2794
But initilize value of sp on signal stack is 16n instead of 16n-8.
Where are you seeing that? It sounds like a bug in your kernel/emulator. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2794#note_32530
On Thu May 11 16:09:42 2023 +0000, Alexandre Julliard wrote:
But initilize value of sp on signal stack is 16n instead of 16n-8. Where are you seeing that? It sounds like a bug in your kernel/emulator. dlls\ntdll\unix\server.c
static int init_thread_pipe(void)
{
int reply_pipe[2];
stack_t ss;
ss.ss_sp = get_signal_stack();
ss.ss_size = signal_stack_size;
ss.ss_flags = 0;
sigaltstack( &ss, NULL );
if (server_pipe( reply_pipe ) == -1) server_protocol_perror( "pipe" );
if (server_pipe( ntdll_get_thread_data()->wait_fd ) == -1) server_protocol_perror( "pipe" );
wine_server_send_fd( reply_pipe[1] );
wine_server_send_fd( ntdll_get_thread_data()->wait_fd[1] );
ntdll_get_thread_data()->reply_fd = reply_pipe[0];
return reply_pipe[1];
}
get_signal_stack() return a value of 16n signal_stack_size = 0x10000 - 0x3800 is also 16n so value of stack top is 16n but on x86_64, value of sp at begin of signal handler must be 16n-8 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2794#note_32531
get_signal_stack() return a value of 16n
signal_stack_size = 0x10000 - 0x3800 is also 16n
so value of stack top is 16n
but on x86_64, value of sp at begin of signal handler must be 16n-8
The signal is sent by the kernel, with the stack pointer set according to the ABI. Please explain on what exact setup you are seeing the problem. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2794#note_32532
On Thu May 11 16:37:50 2023 +0000, Alexandre Julliard wrote:
get_signal_stack() return a value of 16n
signal_stack_size = 0x10000 - 0x3800 is also 16n
so value of stack top is 16n
but on x86_64, value of sp at begin of signal handler must be 16n-8 The signal is sent by the kernel, with the stack pointer set according to the ABI. Please explain on what exact setup you are seeing the problem. for example, when quit_signal occur, quit_handler be called. but value of sp is 16n instead of 16n-8 at begin of quit_handler. the next instruction is "push %rbp", after the instruction, sp is 16n-8. Currently the quit_handler, there is No problem,
static void quit_handler( int signal, siginfo_t *siginfo, void *ucontext )
{
// push %rbp
init_handler( ucontext ); // sp is 16n-8
abort_thread(0);
}
But if I add a calling of WINAPI function, For Example ``` extern int bar(void); int WINAPI foo(void) { // callee is Linux API, caller is WINAPI, need save xmm register. xmm value must store to memory which aligned of 16 return bar() + 1; } static void quit_handler( int signal, siginfo_t *siginfo, void *ucontext ) { // push %rbp foo(); // sp is 16n-8, expected value 16n, problem may occured init_handler( ucontext ); abort_thread(0); } ``` the problem may be occured. Although no errors will currently occur. If add __attribute__((force_align_arg_pointer)) ``` static DECLSPEC_FORCEALIGN void quit_handler( int signal, siginfo_t *siginfo, void *ucontext ) { // push %rbp // and -16, %rsp init_handler( ucontext ); // sp is 16n abort_thread(0); } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2794#note_32533
I'm asking on what operating system and environment you are seeing this. The standard Linux kernel aligns correctly, otherwise all apps would break. My guess is that you are running this under a CPU emulator that doesn't follow the ABI when delivering signals. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2794#note_32534
On Fri May 12 03:28:12 2023 +0000, Alexandre Julliard wrote:
I'm asking on what operating system and environment you are seeing this. The standard Linux kernel aligns correctly, otherwise all apps would break. My guess is that you are running this under a CPU emulator that doesn't follow the ABI when delivering signals. The bug is caused by a third-party libc.so. Use glibc can avoid the bug, I Close the merge
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2794#note_32562
This merge request was closed by Fan WenJie. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2794
participants (3)
-
Alexandre Julliard (＠julliard) -
Fan WenJie
-
Fan WenJie (＠fanwenjie)