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

[JF Bastien]

Could you also run `clang-format` on the code? Preferably separate from other changes to the diff history is easier to read.


================
Comment at: cmake/config-ix.cmake:259
@@ -257,2 +258,3 @@
   mipsel mips64 mips64el powerpc64 powerpc64le)
+filter_available_targets(SAFESTACK_SUPPORTED_ARCH x86_64 i386 i686)
 
----------------
I haven't been through the LLVM-side code yet, but what prevents other architectures from being supported? IIUC nothing is x86 specific since it's in ``lib/Transforms``, right?

================
Comment at: cmake/config-ix.cmake:340
@@ +339,3 @@
+if (SAFESTACK_SUPPORTED_ARCH AND
+    OS_NAME MATCHES "Darwin|Linux|FreeBSD")
+  set(COMPILER_RT_HAS_SAFESTACK TRUE)
----------------
What's missing for Windows support? Thread and TLS support? It's the most significant platform for Chrome (well, and Android), and we're looking forward to LLVM support on Windows. Not having SafeStack would be sad. This can be a different patch, but I think it's important.

================
Comment at: lib/safestack/safestack.cc:44
@@ +43,3 @@
+  __attribute__((visibility ("default")))
+  __thread void  *__safestack_unsafe_stack_ptr = 0;
+}
----------------
`nullptr` here and other pointers below.

================
Comment at: lib/safestack/safestack.cc:54
@@ +53,3 @@
+static inline void *unsafe_stack_alloc(size_t size, size_t guard) {
+  void *addr = MmapOrDie(size + guard, "unsafe_stack_alloc");
+  MprotectNoAccess((uptr)addr, (uptr)guard);
----------------
It's probably not a huge issue for the unsafe stack, but I'd rather check that `size + guard` doesn't overflow `uptr` before calling `MmapOrDie`. The same applies in other parts of the code below.

================
Comment at: lib/safestack/safestack.cc:71
@@ +70,3 @@
+  if (unsafe_stack_start) {
+    (void)UnmapOrDie(
+      (char*) unsafe_stack_start - unsafe_stack_guard,
----------------
Drop the `(void)` case.

================
Comment at: lib/safestack/safestack.cc:106
@@ +105,3 @@
+  // FIXME: we can do this only any other specific key is set by
+  // intersepting the pthread_setspecific function itself
+  pthread_setspecific(thread_cleanup_key, (void*) 1);
----------------
s/intersepting/intercepting/

================
Comment at: lib/safestack/safestack.cc:109
@@ +108,3 @@
+
+  // Start the original thread rutine
+  return start_routine(start_routine_arg);
----------------
s/rutine/routine/

Though I'm not sure the comment is really helping me understand much here, I'd just drop it.

================
Comment at: lib/safestack/safestack.cc:174
@@ +173,3 @@
+  if (initialized)
+    return;
+
----------------
Is this initialization check useful if `__attribute__((constructor(0)))` was used? Can this be done concurrently, or is it just called multiple times from the same thread of execution? If concurrent then initialization is racy.

================
Comment at: lib/safestack/safestack.cc:225
@@ +224,3 @@
+void *__get_safe_stack_ptr() {
+  return (char*) __builtin_frame_address(0) + 2*sizeof(void*);
+}
----------------
Please add a comment on why `2*sizeof(void*)` is correct.

================
Comment at: test/safestack/CMakeLists.txt:9
@@ +8,3 @@
+    list(APPEND SAFESTACK_TEST_DEPS
+      LLVMgold
+    )
----------------
Does this force-build gold for this test? Or does it refuse to build the test if gold wasn't built? Also, comment on why PIC and+binutils_incdir require gold for testing.

================
Comment at: test/safestack/CMakeLists.txt:14
@@ +13,3 @@
+    list(APPEND SAFESTACK_TEST_DEPS
+      LTO
+    )
----------------
Why does Apple require LTO for testing?

================
Comment at: test/safestack/check-buffer-copy.c:9
@@ +8,3 @@
+  int i;
+  char buffer[128];
+
----------------
Could you do the same test with a VLA?

================
Comment at: test/safestack/check-buffer-copy.c:14
@@ +13,3 @@
+    buffer[i] = argv[0][i];
+  buffer[i] = '\0';
+
----------------
Writing and then immediately reading back is pretty brittle if the optimizer decides to kick in and prove the code trivially dead. Could you add some optimizer smarts defeat mechanism here? I'm not sure if compiler-rt has a preferred method of doing this (asm volatile, barrier, volatile, separate noinline call, ...).

================
Comment at: test/safestack/check-overflow.c:10
@@ +9,3 @@
+
+void fct(int *buffer)
+{
----------------
noinline would be appropriate, though IPO could still kick in. Maybe slap a volatile on there too?

================
Comment at: test/safestack/check-overflow.c:13
@@ +12,3 @@
+  buffer[-1] = 36;
+  buffer[6] = 36;
+}
----------------
I'd rather memset a bigger range which includes the buffer itself than just changing two values.

================
Comment at: test/safestack/check-overflow.c:22
@@ +21,3 @@
+  fct(buffer);
+  return value1 != 42 || value2 != 42;
+}
----------------
The optimizer knows that `value1` and `value2` don't escape `main`. This check is trivially true.

================
Comment at: test/safestack/check-pthread.c:22
@@ +21,3 @@
+  char buffer[8096]; // two pages
+  memset(buffer, val, sizeof (buffer));
+
----------------
The buffer allocation and memset are dead code and can be eliminated.

http://reviews.llvm.org/D6096

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