[PATCH] Protection against stack-based memory corruption errors using SafeStack: compiler-rt runtime support library

[Peter Collingbourne]

================
Comment at: CMakeLists.txt:210
@@ -209,2 +209,3 @@
 append_list_if(COMPILER_RT_HAS_FNO_STACK_PROTECTOR_FLAG -fno-stack-protector SANITIZER_COMMON_CFLAGS)
+append_list_if(COMPILER_RT_HAS_FNO_SAFE_STACK_FLAG -fno-safe-stack SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_FVISIBILITY_HIDDEN_FLAG -fvisibility=hidden SANITIZER_COMMON_CFLAGS)
----------------
kcc wrote:
> is this still valid? 
> Don't we use -fsanitize= syntax? 
Updated.

================
Comment at: lib/safestack/safestack.cc:38
@@ +37,3 @@
+/// FIXME: is this in some header?
+#define STACK_ALIGN 16
+
----------------
kcc wrote:
> please no macros where constants work
Done

================
Comment at: lib/safestack/safestack.cc:45
@@ +44,3 @@
+
+// define MAP_STACK if undefined, this flag is used as a hint
+#ifndef MAP_STACK
----------------
kcc wrote:
> do you really need this? 
Removed; they are hints and are not required for correctness. We can bring them back if benchmarks show a clear benefit.

================
Comment at: lib/safestack/safestack.cc:57
@@ +56,3 @@
+// all symbols from pthread that we use dynamically
+#define __DECLARE_WRAPPER(fn) static __typeof__(fn)* __d_ ## fn = NULL;
+
----------------
kcc wrote:
> Mmm. I don't like that. 
> Why can't we just ensure that pthread is linked? 
My understanding is that on some platforms (FreeBSD?) linking the pthread library has a performance impact.

We could probably consider moving pthread stuff to a separate library which is only linked when pthread is linked. I guess for now we can use the pthread symbols unconditionally. Then any later change will mostly be a matter of moving code around.

================
Comment at: lib/safestack/safestack.cc:80
@@ +79,3 @@
+
+#define __GET_UNSAFE_STACK_PTR()         __safestack_unsafe_stack_ptr
+#define __SET_UNSAFE_STACK_PTR(value)    __safestack_unsafe_stack_ptr = (value)
----------------
kcc wrote:
> no macros, please, unless you have too, in which case describe in comments why. 
Removed

================
Comment at: lib/safestack/safestack.cc:101
@@ +100,3 @@
+  // (such overrides might crash is they use the unsafe stack themselves)
+  void *addr = (void *)internal_mmap(
+      NULL, size + guard, PROT_WRITE | PROT_READ,
----------------
kcc wrote:
> I would use MmapOrDie (or create a similar one in sanitizer_common/sanitizer_posix.cc) 
> and get rid of mmap includes in this file. 
Done

================
Comment at: lib/safestack/safestack.cc:114
@@ +113,3 @@
+  void* stack_ptr = (char*) start + size;
+  assert((((size_t)stack_ptr) & (STACK_ALIGN-1)) == 0);
+
----------------
kcc wrote:
> use CHECK_EQ
Done

================
Comment at: lib/safestack/safestack.cc:126
@@ +125,3 @@
+    // (such overrides might crash is they use the unsafe stack themselves)
+    (void)internal_munmap(
+      (char*) __GET_UNSAFE_STACK_START() - __GET_UNSAFE_STACK_GUARD(),
----------------
kcc wrote:
> UnmapOrDie
Done

================
Comment at: lib/safestack/safestack.cc:204
@@ +203,3 @@
+
+  assert(size != 0);
+  assert((size & (STACK_ALIGN-1)) == 0);
----------------
kcc wrote:
> use CHECK
Done

================
Comment at: lib/safestack/safestack.cc:276
@@ +275,3 @@
+
+#ifdef __ELF__
+// Run safestack initialization before any other constructors.
----------------
kcc wrote:
> please make this similar to other sanitizers (SANITIZER_CAN_USE_PREINIT_ARRAY).
Done

================
Comment at: test/safestack/CMakeLists.txt:6
@@ +5,3 @@
+if(NOT COMPILER_RT_STANDALONE_BUILD)
+  list(APPEND SAFESTACK_TEST_DEPS safestack clang)
+endif()
----------------
samsonov wrote:
> Looks like clang is already contained in SANITIZER_COMMON_LIT_TEST_DEPS
Done

http://reviews.llvm.org/D6096

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/