[PATCH v2] ntdll: Force align the stack in RtlRaiseException() on i386.

[Chip Davis]

This is an assembly function, so we have to do it manually.

Signed-off-by: Chip Davis <cdavis(a)codeweavers.com>
---

Notes:
    v2: Fix saving EAX. Unfortunately, I need a register because of the 'subl'
        instruction.

 dlls/ntdll/signal_i386.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/dlls/ntdll/signal_i386.c b/dlls/ntdll/signal_i386.c
index e9dd0de2fc3c..099e10a3ebf2 100644
--- a/dlls/ntdll/signal_i386.c
+++ b/dlls/ntdll/signal_i386.c
@@ -2548,14 +2548,22 @@ __ASM_STDCALL_FUNC( RtlRaiseException, 4,
                     "movl %esp,%ebp\n\t"
                     __ASM_CFI(".cfi_def_cfa_register %ebp\n\t")
                     "leal -0x2cc(%esp),%esp\n\t"  /* sizeof(CONTEXT) */
-                    "pushl %esp\n\t"              /* context */
+                    "andl $-16,%esp\n\t"
+                    "subl $16,%esp\n\t"
+                    "movl %eax,(%esp)\n\t"
+                    "movl %esp,%eax\n\t"
+                    "subl $12,%esp\n\t"           /* stack alignment */
+                    "pushl %eax\n\t"              /* context */
+                    "movl 16(%esp),%eax\n\t"
                     "call " __ASM_STDCALL("RtlCaptureContext",4) "\n\t"
+                    "addl $12,%esp\n\t"
                     "movl 4(%ebp),%eax\n\t"       /* return address */
                     "movl 8(%ebp),%ecx\n\t"       /* rec */
                     "movl %eax,12(%ecx)\n\t"      /* rec->ExceptionAddress */
                     "leal 12(%ebp),%eax\n\t"
                     "movl %eax,0xc4(%esp)\n\t"    /* context->Esp */
                     "movl %esp,%eax\n\t"
+                    "subl $8,%esp\n\t"            /* stack alignment */
                     "pushl %eax\n\t"
                     "pushl %ecx\n\t"
                     "call " __ASM_NAME("raise_exception_full_context") "\n\t"
-- 
2.24.0

[Zebediah Figura]

Hello Chip,

On 1/11/20 4:51 PM, Chip Davis wrote:
> This is an assembly function, so we have to do it manually.
>
> Signed-off-by: Chip Davis <cdavis(a)codeweavers.com>
> ---
>
> Notes:
>     v2: Fix saving EAX. Unfortunately, I need a register because of the 'subl'
>         instruction.
>
> dlls/ntdll/signal_i386.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/ntdll/signal_i386.c b/dlls/ntdll/signal_i386.c
> index e9dd0de2fc3c..099e10a3ebf2 100644
> --- a/dlls/ntdll/signal_i386.c
> +++ b/dlls/ntdll/signal_i386.c
> @@ -2548,14 +2548,22 @@ __ASM_STDCALL_FUNC( RtlRaiseException, 4,
>                      "movl %esp,%ebp\n\t"
>                      __ASM_CFI(".cfi_def_cfa_register %ebp\n\t")
>                      "leal -0x2cc(%esp),%esp\n\t"  /* sizeof(CONTEXT) */
> -                    "pushl %esp\n\t"              /* context */
> +                    "andl $-16,%esp\n\t"
> +                    "subl $16,%esp\n\t"
> +                    "movl %eax,(%esp)\n\t"
> +                    "movl %esp,%eax\n\t"
> +                    "subl $12,%esp\n\t"           /* stack alignment */
> +                    "pushl %eax\n\t"              /* context */

Out of curiosity, why do you need to align the stack again before
calling RtlCaptureContext()? I don't see anything in our implementation
that I'm aware relies on an aligned stack.

And if so, perhaps it'd be better to align the stack inside
RtlCaptureContext() instead of here?
> +                    "movl 16(%esp),%eax\n\t"
>                      "call " __ASM_STDCALL("RtlCaptureContext",4) "\n\t"
> +                    "addl $12,%esp\n\t"
>                      "movl 4(%ebp),%eax\n\t"       /* return address */
>                      "movl 8(%ebp),%ecx\n\t"       /* rec */
>                      "movl %eax,12(%ecx)\n\t"      /* rec->ExceptionAddress */
>                      "leal 12(%ebp),%eax\n\t"
>                      "movl %eax,0xc4(%esp)\n\t"    /* context->Esp */
>                      "movl %esp,%eax\n\t"
> +                    "subl $8,%esp\n\t"            /* stack alignment */
>                      "pushl %eax\n\t"
>                      "pushl %ecx\n\t"
>                      "call " __ASM_NAME("raise_exception_full_context") "\n\t"

[Chip Davis]

January 11, 2020 5:34 PM, "Zebediah Figura" <z.figura12(a)gmail.com> wrote:
> Hello Chip,
>
> On 1/11/20 4:51 PM, Chip Davis wrote:
> > This is an assembly function, so we have to do it manually.
> >
> > Signed-off-by: Chip Davis <cdavis(a)codeweavers.com>
> > ---
> >
> > Notes:
> > v2: Fix saving EAX. Unfortunately, I need a register because of the 'subl'
> > instruction.
> >
> > dlls/ntdll/signal_i386.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/dlls/ntdll/signal_i386.c b/dlls/ntdll/signal_i386.c
> > index e9dd0de2fc3c..099e10a3ebf2 100644
> > --- a/dlls/ntdll/signal_i386.c
> > +++ b/dlls/ntdll/signal_i386.c
> > @@ -2548,14 +2548,22 @@ __ASM_STDCALL_FUNC( RtlRaiseException, 4,
> > "movl %esp,%ebp\n\t"
> > __ASM_CFI(".cfi_def_cfa_register %ebp\n\t")
> > "leal -0x2cc(%esp),%esp\n\t" /* sizeof(CONTEXT) */
> > - "pushl %esp\n\t" /* context */
> > + "andl $-16,%esp\n\t"
> > + "subl $16,%esp\n\t"
> > + "movl %eax,(%esp)\n\t"
> > + "movl %esp,%eax\n\t"
> > + "subl $12,%esp\n\t" /* stack alignment */
> > + "pushl %eax\n\t" /* context */
>
> Out of curiosity, why do you need to align the stack again before
> calling RtlCaptureContext()? I don't see anything in our implementation
> that I'm aware relies on an aligned stack.
>
> And if so, perhaps it'd be better to align the stack inside
> RtlCaptureContext() instead of here?

It isn't needed for RtlCaptureContext(). It's hand-hacked assembly and doesn't have any instructions in it that assume a 16-byte-aligned stack.

I was seeing crashes in send_debug_event(), which I tracked down to the compiler generating an aligned SSE instruction (movaps) on the assumption that the stack was 16-byte aligned. For some reason, adding this here made the crashes go away. These embedded aligned SSE instructions are part of why we compile exported functions with force_align_arg_pointer in the first place. Here we have to do it manually, because of the hand-hacked assembly.
> > + "movl 16(%esp),%eax\n\t"
> > "call " __ASM_STDCALL("RtlCaptureContext",4) "\n\t"
> > + "addl $12,%esp\n\t"
> > "movl 4(%ebp),%eax\n\t" /* return address */
> > "movl 8(%ebp),%ecx\n\t" /* rec */
> > "movl %eax,12(%ecx)\n\t" /* rec->ExceptionAddress */
> > "leal 12(%ebp),%eax\n\t"
> > "movl %eax,0xc4(%esp)\n\t" /* context->Esp */
> > "movl %esp,%eax\n\t"
> > + "subl $8,%esp\n\t" /* stack alignment */
> > "pushl %eax\n\t"
> > "pushl %ecx\n\t"
> > "call " __ASM_NAME("raise_exception_full_context") "\n\t"

Chip

[Zebediah Figura]

On 1/11/20 8:38 PM, Chip Davis wrote:
> January 11, 2020 5:34 PM, "Zebediah Figura" <z.figura12(a)gmail.com> wrote:
> > Hello Chip,
> >
> > On 1/11/20 4:51 PM, Chip Davis wrote:
> > > This is an assembly function, so we have to do it manually.
> > >
> > > Signed-off-by: Chip Davis <cdavis(a)codeweavers.com>
> > > ---
> > >
> > > Notes:
> > > v2: Fix saving EAX. Unfortunately, I need a register because of the 'subl'
> > > instruction.
> > >
> > > dlls/ntdll/signal_i386.c | 10 +++++++++-
> > > 1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/dlls/ntdll/signal_i386.c b/dlls/ntdll/signal_i386.c
> > > index e9dd0de2fc3c..099e10a3ebf2 100644
> > > --- a/dlls/ntdll/signal_i386.c
> > > +++ b/dlls/ntdll/signal_i386.c
> > > @@ -2548,14 +2548,22 @@ __ASM_STDCALL_FUNC( RtlRaiseException, 4,
> > > "movl %esp,%ebp\n\t"
> > > __ASM_CFI(".cfi_def_cfa_register %ebp\n\t")
> > > "leal -0x2cc(%esp),%esp\n\t" /* sizeof(CONTEXT) */
> > > - "pushl %esp\n\t" /* context */
> > > + "andl $-16,%esp\n\t"
> > > + "subl $16,%esp\n\t"
> > > + "movl %eax,(%esp)\n\t"
> > > + "movl %esp,%eax\n\t"
> > > + "subl $12,%esp\n\t" /* stack alignment */
> > > + "pushl %eax\n\t" /* context */
> >
> > Out of curiosity, why do you need to align the stack again before
> > calling RtlCaptureContext()? I don't see anything in our implementation
> > that I'm aware relies on an aligned stack.
> >
> > And if so, perhaps it'd be better to align the stack inside
> > RtlCaptureContext() instead of here?
>
> It isn't needed for RtlCaptureContext(). It's hand-hacked assembly and doesn't have any instructions in it that assume a 16-byte-aligned stack.
>
> I was seeing crashes in send_debug_event(), which I tracked down to the compiler generating an aligned SSE instruction (movaps) on the assumption that the stack was 16-byte aligned. For some reason, adding this here made the crashes go away. These embedded aligned SSE instructions are part of why we compile exported functions with force_align_arg_pointer in the first place. Here we have to do it manually, because of the hand-hacked assembly.

Huh, I'm still confused about how we can get from RtlRaiseException() to
send_debug_event() without going through a WINAPI function (which should
end up realigning the stack). Am I missing something?
> > > + "movl 16(%esp),%eax\n\t"
> > > "call " __ASM_STDCALL("RtlCaptureContext",4) "\n\t"
> > > + "addl $12,%esp\n\t"
> > > "movl 4(%ebp),%eax\n\t" /* return address */
> > > "movl 8(%ebp),%ecx\n\t" /* rec */
> > > "movl %eax,12(%ecx)\n\t" /* rec->ExceptionAddress */
> > > "leal 12(%ebp),%eax\n\t"
> > > "movl %eax,0xc4(%esp)\n\t" /* context->Esp */
> > > "movl %esp,%eax\n\t"
> > > + "subl $8,%esp\n\t" /* stack alignment */
> > > "pushl %eax\n\t"
> > > "pushl %ecx\n\t"
> > > "call " __ASM_NAME("raise_exception_full_context") "\n\t"
>
> Chip

[Chip Davis]

January 11, 2020 11:05 PM, "Zebediah Figura" <z.figura12(a)gmail.com> wrote:
> On 1/11/20 8:38 PM, Chip Davis wrote:
> > January 11, 2020 5:34 PM, "Zebediah Figura" <z.figura12(a)gmail.com> wrote:
> > > Hello Chip,
> > >
> > > On 1/11/20 4:51 PM, Chip Davis wrote:
> >
> > This is an assembly function, so we have to do it manually.
> >
> > Signed-off-by: Chip Davis <cdavis(a)codeweavers.com>
> > ---
> >
> > Notes:
> > v2: Fix saving EAX. Unfortunately, I need a register because of the 'subl'
> > instruction.
> >
> > dlls/ntdll/signal_i386.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/dlls/ntdll/signal_i386.c b/dlls/ntdll/signal_i386.c
> > index e9dd0de2fc3c..099e10a3ebf2 100644
> > --- a/dlls/ntdll/signal_i386.c
> > +++ b/dlls/ntdll/signal_i386.c
> > @@ -2548,14 +2548,22 @@ __ASM_STDCALL_FUNC( RtlRaiseException, 4,
> > "movl %esp,%ebp\n\t"
> > __ASM_CFI(".cfi_def_cfa_register %ebp\n\t")
> > "leal -0x2cc(%esp),%esp\n\t" /* sizeof(CONTEXT) */
> > - "pushl %esp\n\t" /* context */
> > + "andl $-16,%esp\n\t"
> > + "subl $16,%esp\n\t"
> > + "movl %eax,(%esp)\n\t"
> > + "movl %esp,%eax\n\t"
> > + "subl $12,%esp\n\t" /* stack alignment */
> > + "pushl %eax\n\t" /* context */
> > > Out of curiosity, why do you need to align the stack again before
> > > calling RtlCaptureContext()? I don't see anything in our implementation
> > > that I'm aware relies on an aligned stack.
> > >
> > > And if so, perhaps it'd be better to align the stack inside
> > > RtlCaptureContext() instead of here?
> >
> > It isn't needed for RtlCaptureContext(). It's hand-hacked assembly and doesn't have any
> > instructions in it that assume a 16-byte-aligned stack.
> >
> > I was seeing crashes in send_debug_event(), which I tracked down to the compiler generating an
> > aligned SSE instruction (movaps) on the assumption that the stack was 16-byte aligned. For some
> > reason, adding this here made the crashes go away. These embedded aligned SSE instructions are part
> > of why we compile exported functions with force_align_arg_pointer in the first place. Here we have
> > to do it manually, because of the hand-hacked assembly.
>
> Huh, I'm still confused about how we can get from RtlRaiseException() to
> send_debug_event() without going through a WINAPI function (which should
> end up realigning the stack). Am I missing something?

You're not missing anything. We have to go through NtRaiseException() to call send_debug_event(). I'm just as mystified as you. But this fixes crashes attempting to run a process under the control of the debugger when an exception occurs.

It might have something to do with the fact that I'm building with Clang.
> > >
> >
> > + "movl 16(%esp),%eax\n\t"
> > "call " __ASM_STDCALL("RtlCaptureContext",4) "\n\t"
> > + "addl $12,%esp\n\t"
> > "movl 4(%ebp),%eax\n\t" /* return address */
> > "movl 8(%ebp),%ecx\n\t" /* rec */
> > "movl %eax,12(%ecx)\n\t" /* rec->ExceptionAddress */
> > "leal 12(%ebp),%eax\n\t"
> > "movl %eax,0xc4(%esp)\n\t" /* context->Esp */
> > "movl %esp,%eax\n\t"
> > + "subl $8,%esp\n\t" /* stack alignment */
> > "pushl %eax\n\t"
> > "pushl %ecx\n\t"
> > "call " __ASM_NAME("raise_exception_full_context") "\n\t"
> >
> > Chip

Chip