[PATCH] TidySigaction in sanitizer_linux

[Sergey Matveev]

Hi kcc, glider, samsonov,

Add a wrapper for sigaction() that prevents it from copying garbage
stack values into unused portions of oldact.

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

Files:
  lib/sanitizer_common/sanitizer_linux.cc
  lib/sanitizer_common/sanitizer_linux.h
  lib/sanitizer_common/sanitizer_stoptheworld_linux.cc

Index: lib/sanitizer_common/sanitizer_linux.cc
===================================================================
--- lib/sanitizer_common/sanitizer_linux.cc
+++ lib/sanitizer_common/sanitizer_linux.cc
@@ -647,6 +647,19 @@
   return syscall(__NR_sigaltstack, ss, oss);
 }
 
+int TidySigaction(int signum, const struct sigaction *act,
+                  struct sigaction *oldact) {
+  // XXX: This assumes the number of signals is 64, which is currently true on
+  // platforms we care about.
+  const uptr kKernelSigsetSize = 8;
+  int res = sigaction(signum, act, oldact);
+  if (oldact) {
+    internal_memset((void *)((uptr)&oldact->sa_mask + kKernelSigsetSize), 0,
+                    sizeof(oldact->sa_mask) - kKernelSigsetSize);
+  }
+  return res;
+}
+
 // ThreadLister implementation.
 ThreadLister::ThreadLister(int pid)
   : pid_(pid),
Index: lib/sanitizer_common/sanitizer_linux.h
===================================================================
--- lib/sanitizer_common/sanitizer_linux.h
+++ lib/sanitizer_common/sanitizer_linux.h
@@ -16,6 +16,7 @@
 #include "sanitizer_internal_defs.h"
 
 struct sigaltstack;
+struct sigaction;
 
 namespace __sanitizer {
 // Dirent structure for getdents(). Note that this structure is different from
@@ -27,6 +28,16 @@
 int internal_prctl(int option, uptr arg2, uptr arg3, uptr arg4, uptr arg5);
 int internal_sigaltstack(const struct sigaltstack *ss, struct sigaltstack *oss);
 
+// At least in some libc implementations, sigaction() has a side-effect where
+// it copies 120 bytes from stack directly into oldact without ever touching
+// them. This happens because libc allocates a 128-byte signal mask for POSIX
+// compliance, but the syscall only writes to the first 8 bytes. The net effect
+// is that out-of-scope stack values end up in oldact, which can cause problems
+// in LSan. We fix this by wrapping sigaction() calls in a function that wipes
+// the unused portion of the signal mask.
+int TidySigaction(int signum, const struct sigaction *act,
+                  struct sigaction *oldact);
+
 // This class reads thread IDs from /proc/<pid>/task using only syscalls.
 class ThreadLister {
  public:
Index: lib/sanitizer_common/sanitizer_stoptheworld_linux.cc
===================================================================
--- lib/sanitizer_common/sanitizer_stoptheworld_linux.cc
+++ lib/sanitizer_common/sanitizer_stoptheworld_linux.cc
@@ -292,11 +292,10 @@
     internal_memset(&new_sigaction, 0, sizeof(new_sigaction));
     new_sigaction.sa_handler = SIG_DFL;
     sigfillset(&new_sigaction.sa_mask);
-    sigaction(kUnblockedSignals[signal_index], &new_sigaction,
-                    &old_sigactions[signal_index]);
+    CHECK_EQ(0, TidySigaction(kUnblockedSignals[signal_index], &new_sigaction,
+                              &old_sigactions[signal_index]));
   }
-  int sigprocmask_status = sigprocmask(SIG_BLOCK, &blocked_sigset, &old_sigset);
-  CHECK_EQ(sigprocmask_status, 0); // sigprocmask should never fail
+  CHECK_EQ(0, sigprocmask(SIG_BLOCK, &blocked_sigset, &old_sigset));
   // Make this process dumpable. Processes that are not dumpable cannot be
   // attached to.
   int process_was_dumpable = internal_prctl(PR_GET_DUMPABLE, 0, 0, 0, 0);
@@ -339,10 +338,10 @@
   // Restore the signal handlers.
   for (uptr signal_index = 0; signal_index < ARRAY_SIZE(kUnblockedSignals);
        signal_index++) {
-    sigaction(kUnblockedSignals[signal_index],
-              &old_sigactions[signal_index], NULL);
+    CHECK_EQ(0, TidySigaction(kUnblockedSignals[signal_index],
+                              &old_sigactions[signal_index], NULL));
   }
-  sigprocmask(SIG_SETMASK, &old_sigset, &old_sigset);
+  CHECK_EQ(0, sigprocmask(SIG_SETMASK, &old_sigset, &old_sigset));
 }
 
 // Platform-specific methods from SuspendedThreadsList.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D603.1.patch
Type: text/x-patch
Size: 3821 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130401/c0707f88/attachment.bin>