Fix 343173 - helgrind crash during stack unwind

This fixes a helgrind crash detected on android.
Android bionic pthread lib unmaps the stack for detached threads
before exiting.
Helgrind tries to unwind the stack to record a 'read' after
the stack unmap, just before the exit syscall.
The unwind then causes a SEGV.

The solution consists in tightening the calculation of
the stack limits, so as to stop unwinding when no valid stack
can be found.
Regression test reproduces the same problem by simulating the
bionic behaviour on linux, using asm similar to bionic lib.



git-svn-id: svn://svn.valgrind.org/valgrind/trunk@14976

NEWS

@@ -88,6 +88,7 @@ where XXXXXX is the bug number as listed below.
 342795  Internal glibc __GI_mempcpy call should be intercepted
 343012  Unhandled syscall 319 (memfd_create)
 343069  Patch updating v4l2 API support
+343173  helgrind crash during stack unwind
 343303  Fix known deliberate memory leak in setenv() on Mac OS X 10.10
 343306  OS X 10.10: UNKNOWN mach_msg unhandled MACH_SEND_TRAILER option
 343332  Unhandled instruction 0x9E310021 (fcvtmu) on aarch64

coregrind/m_stacks.c

@@ -33,6 +33,7 @@
 #include "pub_core_libcassert.h"
 #include "pub_core_libcprint.h"
 #include "pub_core_mallocfree.h"
+#include "pub_core_aspacemgr.h"
 #include "pub_core_options.h"
 #include "pub_core_stacks.h"
 #include "pub_core_tooliface.h"
@@ -202,7 +203,7 @@ UWord VG_(register_stack)(Addr start, Addr end)
       current_stack = i;
    }
 
-   VG_(debugLog)(2, "stacks", "register [%p-%p] as stack %lu\n",
+   VG_(debugLog)(2, "stacks", "register [start-end] [%p-%p] as stack %lu\n",
                     (void*)start, (void*)end, i->id);
 
    return i->id;
@@ -269,11 +270,56 @@ void VG_(change_stack)(UWord id, Addr start, Addr end)
 void VG_(stack_limits)(Addr SP, Addr *start, Addr *end )
 {
    Stack* stack = find_stack_by_addr(SP);
+   NSegment const *stackseg = VG_(am_find_nsegment) (SP);
 
    if (stack) {
       *start = stack->start;
       *end = stack->end;
    }
+
+   /* SP is assumed to be in a RW segment.
+      If no such segment is found, assume we have no valid
+      stack for SP, and set *start and *end to 0.
+      Otherwise, possibly reduce the stack limits to the boundaries of the
+      RW segment containing SP. */
+   if (stackseg == NULL) {
+      VG_(debugLog)(2, "stacks", 
+                    "no addressable segment for SP %p\n", 
+                    (void*)SP);
+      *start = 0;
+      *end = 0;
+   } else if (!stackseg->hasR || !stackseg->hasW) {
+      VG_(debugLog)(2, "stacks", 
+                    "segment for SP %p is not Readable and/or not Writable\n",
+                    (void*)SP);
+      *start = 0;
+      *end = 0;
+   } else {
+      if (*start < stackseg->start) {
+         VG_(debugLog)(2, "stacks", 
+                       "segment for SP %p changed stack start limit"
+                       " from %p to %p\n",
+                       (void*)SP, (void*)*start, (void*)stackseg->start);
+         *start = stackseg->start;
+      }
+      if (*end > stackseg->end) {
+         VG_(debugLog)(2, "stacks", 
+                       "segment for SP %p changed stack end limit"
+                       " from %p to %p\n",
+                       (void*)SP, (void*)*end, (void*)stackseg->end);
+         *end = stackseg->end;
+      }
+
+      /* If reducing start and/or end to the SP segment gives an
+         empty range, return 'empty' limits */
+      if (*start > *end) {
+         VG_(debugLog)(2, "stacks", 
+                       "stack for SP %p start %p after end %p\n",
+                       (void*)SP, (void*)*start, (void*)end);
+         *start = 0;
+         *end = 0;
+      }
+   }
 }
 
 /* complaints_stack_switch reports that SP has changed by more than some

helgrind/tests/Makefile.am

@@ -49,6 +49,7 @@ EXTRA_DIST = \
 	pth_spinlock.vgtest pth_spinlock.stdout.exp pth_spinlock.stderr.exp \
 	rwlock_race.vgtest rwlock_race.stdout.exp rwlock_race.stderr.exp \
 	rwlock_test.vgtest rwlock_test.stdout.exp rwlock_test.stderr.exp \
+	stackteardown.vgtest stackteardown.stdout.exp stackteardown.stderr.exp \
 	t2t_laog.vgtest t2t_laog.stdout.exp t2t_laog.stderr.exp \
 	tc01_simple_race.vgtest tc01_simple_race.stdout.exp \
 		tc01_simple_race.stderr.exp \
@@ -124,6 +125,7 @@ check_PROGRAMS = \
 	locked_vs_unlocked2 \
 	locked_vs_unlocked3 \
 	pth_destroy_cond \
+	stackteardown \
 	t2t \
 	tc01_simple_race \
 	tc02_simple_tls \

helgrind/tests/stackteardown.c

@@ -0,0 +1,104 @@
+#include <sys/mman.h>
+#include <pthread.h>
+#include <unistd.h>
+#include <assert.h>
+#include <unistd.h>
+#include <sys/syscall.h>
+#include "../../config.h"
+
+#define VG_STRINGIFZ(__str)  #__str
+#define VG_STRINGIFY(__str)  VG_STRINGIFZ(__str)
+
+extern void _exit_with_stack_teardown(void*, size_t);
+
+/* Below code is modified version of android bionic
+   pthread_exit: when a detached thread exits: it munmaps
+   its stack and then exits. We cannot do that in C,
+   as we cannot touch the stack after the munmap
+   and before the exit. */
+
+#if defined(VGP_x86_linux)
+asm("\n"
+    ".text\n"
+    "\t.globl _exit_with_stack_teardown\n"
+    "\t.type  _exit_with_stack_teardown,@function\n"
+    "_exit_with_stack_teardown:\n"
+    // We can trash registers because this function never returns.
+    "\tmov 4(%esp), %ebx\n"             // stackBase
+    "\tmov 8(%esp), %ecx\n"             // stackSize
+    "\tmov $"VG_STRINGIFY(__NR_munmap)", %eax\n"
+    "\tint $0x80\n"
+    // If munmap failed, we ignore the failure and exit anyway.
+
+    "\tmov $0, %ebx\n"                  // status
+    "\tmovl $"VG_STRINGIFY(__NR_exit)", %eax\n"
+    "\tint $0x80\n");
+    // The exit syscall does not return.
+
+#elif defined(VGP_amd64_linux)
+asm("\n"
+    ".text\n"
+    "\t.globl _exit_with_stack_teardown\n"
+    "\t.type  _exit_with_stack_teardown,@function\n"
+    "_exit_with_stack_teardown:\n"
+    "\tmov $"VG_STRINGIFY(__NR_munmap)", %eax\n"
+    "\tsyscall\n"
+    // If munmap failed, we ignore the failure and exit anyway.
+    "\tmov $0, %rdi\n"
+    "\tmov $"VG_STRINGIFY(__NR_exit)", %eax\n"
+    "\tsyscall\n");
+  // The exit syscall does not return.
+
+#elif defined(VGP_arm_linux)
+asm("\n"
+    ".text\n"
+    "\t.globl _exit_with_stack_teardown\n"
+    "\t.type  _exit_with_stack_teardown,%function\n"
+    "_exit_with_stack_teardown:\n"
+    "\tldr r7, ="VG_STRINGIFY(__NR_munmap)"\n"
+    "\tswi #0\n"
+    // If munmap failed, we ignore the failure and exit anyway.
+
+    "\tmov r0, #0\n"
+    "\tldr r7, ="VG_STRINGIFY(__NR_exit)"\n"
+    "\tswi #0\n");
+  // The exit syscall does not return.
+
+#else
+void _exit_with_stack_teardown(void*stack, size_t sz)
+{
+   // asm code not done for this platform.
+   // Do nothing, just return. The thread will exit spontaneously
+}
+
+#endif
+static void *stack;
+static size_t sz = 64 * 1024;
+
+/* This one detaches, does its own thing. */
+void* child_fn ( void* arg )
+{
+  int r;
+  r= pthread_detach( pthread_self() ); assert(!r);
+  _exit_with_stack_teardown(stack, sz);
+  return NULL;
+}
+
+/* Parent creates 1 child, that will detach, and exit after destroying
+   its own stack. */
+int main ( void )
+{
+   int r;
+   pthread_attr_t attr;
+   pthread_t child;
+
+   r = pthread_attr_init(&attr); assert(!r);
+   stack = mmap(NULL, sz, PROT_READ|PROT_WRITE,  MAP_PRIVATE | MAP_ANONYMOUS,
+                -1, 0);
+   assert(stack != (void *)-1);
+   r = pthread_attr_setstack(&attr, stack, sz);
+   r = pthread_create(&child, &attr, child_fn, NULL); assert(!r);
+   sleep(1);
+
+   return 0;
+}

helgrind/tests/stackteardown.stderr.exp

@@ -0,0 +1,3 @@
+
+
+ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

helgrind/tests/stackteardown.vgtest

@@ -0,0 +1 @@
+prog: stackteardown