[PATCH] Run the callback on a separate stack in StopTheWorld.

[Sergey Matveev]

Hi kcc, glider, samsonov,

Currently the callback runs on the caller's stack. If this stack
contains values that have gone out of scope, and we are not super careful, those
values can propagate into global variables (the libc sigaction() in particular
has a side effect that can lead to this). This has caused false negatives in
leak checking code.

Changes: map a separate stack space for the tracer thread. Also, move some
globals into local scope (they had no business being global anyway).

http://llvm-reviews.chandlerc.com/D592

Files:
  lib/sanitizer_common/sanitizer_stoptheworld_linux.cc

Index: lib/sanitizer_common/sanitizer_stoptheworld_linux.cc
===================================================================
--- lib/sanitizer_common/sanitizer_stoptheworld_linux.cc
+++ lib/sanitizer_common/sanitizer_stoptheworld_linux.cc
@@ -71,6 +71,8 @@
 COMPILER_CHECK(sizeof(SuspendedThreadID) == sizeof(pid_t));
 
 namespace __sanitizer {
+static const uptr kTracerStackSize = 2 * 1024 * 1024;
+
 // This class handles thread suspending/unsuspending in the tracer thread.
 class ThreadSuspender {
  public:
@@ -247,16 +249,38 @@
   return exit_code;
 }
 
-static sigset_t blocked_sigset;
-static sigset_t old_sigset;
-static struct sigaction old_sigactions[ARRAY_SIZE(kUnblockedSignals)];
+class ScopedStackSpaceWithGuard {
+ public:
+  ScopedStackSpaceWithGuard(uptr stack_size) {
+    stack_size_ = stack_size;
+    guard_size_ = GetPageSizeCached();
+    guard_start_ = MmapOrDie(stack_size_ + guard_size_,
+                             "ScopedStackWithGuard");
+    void *res = Mprotect((uptr)guard_start_, guard_size_);
+    CHECK_EQ(res, guard_start_);
+  }
+  ~ScopedStackSpaceWithGuard() {
+    UnmapOrDie(guard_start_, stack_size_ + guard_size_);
+  }
+  void *Bottom() {
+    return (void *)((uptr)guard_start_ + stack_size_ + guard_size_);
+  }
+
+ private:
+  uptr stack_size_;
+  uptr guard_size_;
+  void* guard_start_;
+};
 
 void StopTheWorld(StopTheWorldCallback callback, void *argument) {
   // Block all signals that can be blocked safely, and install default handlers
   // for the remaining signals.
   // We cannot allow user-defined handlers to run while the ThreadSuspender
   // thread is active, because they could conceivably call some libc functions
   // which modify errno (which is shared between the two threads).
+  sigset_t blocked_sigset;
+  sigset_t old_sigset;
+  struct sigaction old_sigactions[ARRAY_SIZE(kUnblockedSignals)];
   sigfillset(&blocked_sigset);
   for (uptr signal_index = 0; signal_index < ARRAY_SIZE(kUnblockedSignals);
        signal_index++) {
@@ -281,16 +305,11 @@
   struct TracerThreadArgument tracer_thread_argument;
   tracer_thread_argument.callback = callback;
   tracer_thread_argument.callback_argument = argument;
+  ScopedStackSpaceWithGuard tracer_stack(kTracerStackSize);
   // Block the execution of TracerThread until after we have set ptrace
   // permissions.
   tracer_thread_argument.mutex.Lock();
-  // The tracer thread will run on the same stack, so we must reserve some
-  // stack space for the caller thread to run in as it waits on the tracer.
-  const uptr kReservedStackSize = 4096;
-  // Get a 16-byte aligned pointer for stack.
-  int a_local_variable __attribute__((__aligned__(16)));
-  pid_t tracer_pid = clone(TracerThread,
-                          (char *)&a_local_variable - kReservedStackSize,
+  pid_t tracer_pid = clone(TracerThread, tracer_stack.Bottom(),
                           CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_UNTRACED,
                           &tracer_thread_argument, 0, 0, 0);
   if (tracer_pid < 0) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D592.1.patch
Type: text/x-patch
Size: 3030 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130328/ef824cb6/attachment.bin>