Segfault in _Unwind_* code called from pthread_exit

[Andreas Tobler]

On 29.10.17 20:13, Konstantin Belousov wrote:
> On Sun, Oct 29, 2017 at 06:23:51PM +0100, Tijl Coosemans wrote:
>> On Sat, 26 Aug 2017 21:40:34 +0300 Konstantin Belousov <kostikbel at gmail.com> wrote:
>>> On Sat, Aug 26, 2017 at 08:28:13PM +0200, Tijl Coosemans wrote:
>>>> I did consider using
>>>> a CFI directive (see patch below) and it works, but it's architecture
>>>> specific and it's inserted after the function prologue so there's still
>>>> a window of a few instructions where a stack unwinder will try to use
>>>> the return address.
>>>>
>>>> Index: lib/libthr/thread/thr_create.c
>>>> ===================================================================
>>>> --- lib/libthr/thread/thr_create.c      (revision 322802)
>>>> +++ lib/libthr/thread/thr_create.c      (working copy)
>>>> @@ -251,6 +251,7 @@ create_stack(struct pthread_attr *pattr)
>>>>   static void
>>>>   thread_start(struct pthread *curthread)
>>>>   {
>>>> +       __asm(".cfi_undefined %rip");
>>>>          sigset_t set;
>>>>   
>>>>          if (curthread->attr.suspend == THR_CREATE_SUSPENDED)
>>>
>>> I like this approach much more than the previous patch.  What can be
>>> done is to provide asm trampoline which calls thread_start().  There you
>>> can add the .cfi_undefined right at the entry.
>>>
>>> It is somewhat more work than just setting the return address on the
>>> kernel-constructed pseudo stack frame, but I believe this is ultimately
>>> correct way.  You still can do it only on some arches, if you do not
>>> have incentive to code asm for all of them.
>>
>> Ok, but then there are two ways to implement the trampoline:
>>
>> 1)
>> 	movq $0,(%rsp)
>> 	jmp thread_start
>> 2)
>> 	subq $8,%rsp
>> 	call thread_start
>> 	/* NOTREACHED */
>>
>> With 1) you're setting the return address to zero anyway, so you might
>> as well do that in the kernel like my first patch.  With 2) you're
>> setting up a new call frame, basically redoing what the kernel already
>> did and on i386 this also means copying the function argument.
> I do not quite understand the second variant, because the stack is not
> guaranteed to be zeroed, and it is often not if reused after the previously
> exited thread.
> 
> The first variant is what I like, but perhaps we need to emulate the
> frame as well, i.e. push two zero longs.
> 
> Currently kernel does not access the usermode stack for the new thread
> unless dictated by ABI (i.e. it does not touch it for 64bit process
> on amd64, but have to for 32bit).  I like this property.  Also, the
> previous paragraph is indicative: we do not really know in kernel
> what ABI the userspace follows.  It might want frame, may be it does
> not need it.  It could use other register than %rbp as the frame base,
> etc.
> 
>>
>> Do you have any preference (or better alternatives), because I think I
>> still prefer my first patch.  It's the caller's job to set up the call
>> frame, in this case the kernel.  And if the kernel handles it then it
>> also works with (hypothetical) implementations other than libthr.
>>
>>> Also crt1 probably should get the same treatment, despite we already set
>>> %rbp to zero AFAIR.
>>
>> I haven't checked but I imagine the return address of the process entry
>> point is always zero because the stack is all zeros.
> Stack is not zero. The environment and argument strings and auxv are copied
> at top, and at the bottom the ps_strings structure is located, so it
> is not.
> 
> If you commit your existing patch as is, I will not resent.  But I do think
> that stuff that can be done in usermode, should be done in usermode, esp.
> when the amount of efforts is same.

Attached what I have for libgcc. It can be applied to gcc5-8, should 
give no issues. The mentioned tc from this thread and mine, 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82635 do pass.

What do you think?

Andreas

-------------- next part --------------
Index: libgcc/config/i386/freebsd-unwind.h
===================================================================
--- libgcc/config/i386/freebsd-unwind.h	(revision 254205)
+++ libgcc/config/i386/freebsd-unwind.h	(working copy)
@@ -28,7 +28,10 @@
 
 #include <sys/types.h>
 #include <signal.h>
+#include <unistd.h>
+#include <sys/sysctl.h>
 #include <sys/ucontext.h>
+#include <sys/user.h>
 #include <machine/sigframe.h>
 
 #define REG_NAME(reg)	sf_uc.uc_mcontext.mc_## reg
@@ -36,38 +39,46 @@
 #ifdef __x86_64__
 #define MD_FALLBACK_FRAME_STATE_FOR x86_64_freebsd_fallback_frame_state
 
+static int
+x86_64_outside_sigtramp_range (unsigned char *pc)
+{
+  static int sigtramp_range_determined = 0;
+  static unsigned char *sigtramp_start, *sigtramp_end;
+
+  if (sigtramp_range_determined == 0)
+    {
+      struct kinfo_sigtramp kst = {0};
+      size_t len = sizeof (kst);
+      int mib[4] = { CTL_KERN, KERN_PROC, KERN_PROC_SIGTRAMP, getpid() };
+
+      sigtramp_range_determined = 1;
+      if (sysctl (mib, 4, &kst, &len, NULL, 0) == 0)
+      {
+        sigtramp_range_determined = 2;
+        sigtramp_start = kst.ksigtramp_start;
+        sigtramp_end   = kst.ksigtramp_end;
+      }
+    }
+  if (sigtramp_range_determined < 2)  /* sysctl failed if < 2 */
+    return 1;
+
+  return (pc < sigtramp_start || pc >= sigtramp_end);
+}
+
 static _Unwind_Reason_Code
 x86_64_freebsd_fallback_frame_state
 (struct _Unwind_Context *context, _Unwind_FrameState *fs)
 {
   struct sigframe *sf;
-  long new_cfa;
+  _Unwind_Ptr new_cfa;
 
-  /* Prior to FreeBSD 9, the signal trampoline was located immediately
-     before the ps_strings.  To support non-executable stacks on AMD64,
-     the sigtramp was moved to a shared page for FreeBSD 9.  Unfortunately
-     this means looking frame patterns again (sys/amd64/amd64/sigtramp.S)
-     rather than using the robust and convenient KERN_PS_STRINGS trick.
-
-     <pc + 00>:  lea     0x10(%rsp),%rdi
-     <pc + 05>:  pushq   $0x0
-     <pc + 17>:  mov     $0x1a1,%rax
-     <pc + 14>:  syscall
-
-     If we can't find this pattern, we're at the end of the stack.
-  */
-
-  if (!(   *(unsigned int *)(context->ra)      == 0x247c8d48
-        && *(unsigned int *)(context->ra +  4) == 0x48006a10
-        && *(unsigned int *)(context->ra +  8) == 0x01a1c0c7
-        && *(unsigned int *)(context->ra + 12) == 0x050f0000 ))
+  if (x86_64_outside_sigtramp_range(context->ra))
     return _URC_END_OF_STACK;
 
   sf = (struct sigframe *) context->cfa;
   new_cfa = sf->REG_NAME(rsp);
   fs->regs.cfa_how = CFA_REG_OFFSET;
-  /* Register 7 is rsp  */
-  fs->regs.cfa_reg = 7;
+  fs->regs.cfa_reg =  __LIBGCC_STACK_POINTER_REGNUM__;
   fs->regs.cfa_offset = new_cfa - (long) context->cfa;
 
   /* The SVR4 register numbering macros aren't usable in libgcc.  */