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.
From 646ec4c2c1bc89c4243b69643f172eb20d5876c1 Mon Sep 17 00:00:00 2001
From: Jon TURNEY <[email protected]>
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
