[PATCH 2/3] Provide ucontext to signal handlers

[Jon TURNEY]

On 01/04/2015 15:22, Corinna Vinschen wrote:
> On Apr  1 14:19, Jon TURNEY wrote:
>> Add ucontext.h header, defining ucontext_t and mcontext_t types.
>>
>> Provide sigaction sighandlers with a ucontext_t parameter, containing stack and
>> context information.
>>
>> 	* include/sys/ucontext.h : New header.
>> 	* include/ucontext.h : Ditto.
>> 	* exceptions.cc (call_signal_handler): Provide ucontext_t
>> 	parameter to signal handler function.
>
> Patch is ok with a single change:  Please add a "FIXME?" comment to:
>
>    else
>      RtlCaptureContext();
>
> On second thought, calling RtlCaptureContext here is probably wrong.

Wrong and also dangerous.

This causes random crashes on x86.

It seems that RtlCaptureContext requires the framepointer of the calling 
function in ebp, which it uses to report the rip and rsp of it's caller.

It also seems that gcc can decide to optimize the setting of the 
framepointer away, irrespective of the fact that -fomit-frame-pointer is 
not used when building exceptions.cc

If _cygtls::call_signal_handler() happens to get called with ebp 
pointing to an invalid memory address, as seems to happen occasionally, 
we will fault in RtlCaptureContext.  (in all cases, the eip and ebp in 
the returned context are incorrect)

I wrote the attached patch, which fakes a callframe for 
RtlCaptureContext to avoid these possible crashes, but this needs more 
work to correctly report eip and ebp

However, I'm not sure that is worthwhile effort as it's heading in the 
wrong direction, because ....

> What we really need is the context of the thread when calling
> call_signal_handler I think.  It would be better to call RtlCaptureContext
> before calling call_signal_handler.  But this requires a change in how
> call_signal_handler is called.

-------------- next part --------------
From 646ec4c2c1bc89c4243b69643f172eb20d5876c1 Mon Sep 17 00:00:00 2001
From: Jon TURNEY <jon.turney@dronecode.org.uk>
Date: Fri, 3 Apr 2015 22:26:21 +0100
Subject: [PATCH] Avoid random crashes in RtlCaptureContext on x86

RtlCaptureContext requires the framepointer of the calling function in ebp,
which it uses to report the rip and rsp of it's caller. But it seems that gcc
may decide to optimize the setting of the framepointer away, irrespective of
-fomit-frame-pointer.

If _cygtls::call_signal_handler() is called with ebp pointing to an invalid
memory address, as seems to happen occasionally, we will fault in
RtlCaptureContext.

This patch manually creates a stcall callframe around RtlCaptureContext to be
safe, but I'm not sure that's the best approach.

RtlCaptureContext does so little, it's probably just as effective to write our
own version.

This is heading further in the wrong direction, as the context that is wanted
isn't the current context, but the context at the time of the signal.

There are a couple of other uses of RtlCaptureContext(), are they ok?
---
 winsup/cygwin/exceptions.cc | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 0d1f36d..0ca6dd8 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -1496,8 +1496,24 @@ _cygtls::call_signal_handler ()
       if (thissi.si_cyg)
         memcpy (&thiscontext.uc_mcontext, ((cygwin_exception *)thissi.si_cyg)->context(), sizeof(CONTEXT));
       else
-        RtlCaptureContext ((CONTEXT *)&thiscontext.uc_mcontext);
-        /* FIXME: Really this should be the context which the signal interrupted? */
+        {
+          /* FIXME: Really this should be the context which the signal interrupted? */
+#ifdef __x86_64__
+          RtlCaptureContext ((CONTEXT *)&thiscontext.uc_mcontext);
+#else
+          memset(&thiscontext.uc_mcontext, 0, sizeof(struct __mcontext));
+          thiscontext.uc_mcontext.ctxflags = CONTEXT_FULL;
+          /* RtlCaptureContext requires the framepointer of the calling function
+             in ebp, which it uses to report the rip and rsp of it's caller.
+             But it seems that gcc may decide to optimize the setting of the
+             framepointer away, irrespective of -fomit-frame-pointer.  Manually
+             create a stcall callframe to be safe. */
+          __asm__ ( "mov %%esp,%%ebp\n"
+                    "push %0\n"
+                    "call _RtlCaptureContext@4\n"
+                    : : "r" (&thiscontext.uc_mcontext) : "%ebp", "%eax", "%ecx", "%edx" );
+#endif
+        }
 
       /* FIXME: If/when sigaltstack is implemented, this will need to do
          something more complicated */
-- 
2.1.4