[PATCH 0/1] MR2794: ntdll: add attribute "force_align_arg_pointer" to hanlder of signal on i386 & x86_64

[Fan WenJie (＠fanwenjie)]

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

[PATCH 1/1] ntdll: add attribute "force_align_arg_pointer" to hanlder of signal on i386 & x86_64

[Fan WenJie]

From: Fan WenJie

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)

[Alexandre Julliard (＠julliard)]

> 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.

[Fan WenJie (＠fanwenjie)]

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

[Alexandre Julliard (＠julliard)]

> 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.

[Fan WenJie (＠fanwenjie)]

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);
}
```

[Alexandre Julliard (＠julliard)]

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.

[Fan WenJie (＠fanwenjie)]

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

[PATCH 0/1] MR2794: ntdll: add attribute "force_align_arg_pointer" to hanlder of signal on i386 & x86_64 - closed

[Fan WenJie (＠fanwenjie)]

This merge request was closed by Fan WenJie.