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

[Kostya Serebryany]

================
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)
----------------
is this still valid? 
Don't we use -fsanitize= syntax? 

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

================
Comment at: lib/safestack/safestack.cc:45
@@ +44,3 @@
+
+// define MAP_STACK if undefined, this flag is used as a hint
+#ifndef MAP_STACK
----------------
do you really need this? 

================
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;
+
----------------
Mmm. I don't like that. 
Why can't we just ensure that pthread is linked? 

================
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)
----------------
no macros, please, unless you have too, in which case describe in comments why. 

================
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,
----------------
I would use MmapOrDie (or create a similar one in sanitizer_common/sanitizer_posix.cc) 
and get rid of mmap includes in this file. 

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

================
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(),
----------------
UnmapOrDie

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

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

http://reviews.llvm.org/D6096

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