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

[David Chisnall]

I've done a quick review, I'll do a more detailed follow-up once the comments inline have been addressed.

================
Comment at: lib/safestack/safestack.cc:34
@@ +33,3 @@
+// Should we make the following configurable?
+#define __SAFESTACK_DEFAULT_STACK_SIZE 0x2800000
+
----------------
Yes.  It must also be dynamic to respect pthread stack limits and system-wide rlimits.

================
Comment at: lib/safestack/safestack.cc:75
@@ +74,3 @@
+
+// The following locations are platform-specific
+# define __GET_UNSAFE_STACK_PTR()         (void*) __THREAD_GETMEM_L(0x280)
----------------
There are magic numbers here again.  Please reference either an ABI document or equivalent that indicates that these are safe locations to use.

================
Comment at: lib/safestack/safestack.cc:145
@@ +144,3 @@
+#if defined(__linux__)
+    (void*) syscall(SYS_mmap,
+#else
----------------
Why is this Linux only?  If the goal is to avoid LD_PRELOAD and similar interceptions, then it should be done everywhere (possibly with the exception of Darwin)

================
Comment at: lib/safestack/safestack.cc:152
@@ +151,3 @@
+#if defined(__linux__)
+              | MAP_STACK | MAP_GROWSDOWN
+#endif
----------------
On FreeBSD, MAP_STACK should also be passed.  Not sure about other BSDs.  In general, however, these checks should be of the form:
#ifdef MAP_STACK
 | MAP_STACK
#endif
Not #if defined(linux)

================
Comment at: lib/safestack/safestack.cc:255
@@ +254,3 @@
+void thread_cleanup_handler(void* _iter) {
+  // We want to free the unsafe stack only after all other destructors
+  // have already run. We force this function to be called multiple times.
----------------
This seems somewhat fragile.  It doesn't guarantee that the other destructors won't be run after this, only that they get several opportunities to run before.

================
Comment at: lib/safestack/safestack.cc:271
@@ +270,3 @@
+#ifndef __linux__
+__attribute__((constructor(0)))
+#endif
----------------
Why is this not on Linux?

================
Comment at: lib/safestack/safestack.cc:280
@@ +279,3 @@
+  // Allocate unsafe stack for main thread
+  size_t size = __SAFESTACK_DEFAULT_STACK_SIZE;
+  size_t guard = 4096;
----------------
This should check RLIMIT_STACK.

================
Comment at: lib/safestack/safestack.cc:327
@@ +326,3 @@
+__attribute__((noinline))
+void *__safestack_get_unsafe_stack_ptr() {
+  return __GET_UNSAFE_STACK_PTR();
----------------
Why noinline?  It is externally visible, so a non-inline version will be produced anyway.  Why not let LTO builds inline it?  Or is the noinline attribute because this function must be compiled *without* the safe stack support itself?  If so, document it (or perhaps add the no_safestack attribute instead, to prevent it from being inlined in unsafe contexts).

================
Comment at: lib/safestack/safestack.cc:340
@@ +339,3 @@
+// Run safestack initialization before any other constructors
+// FIXME: can we do something similar on Mac or FreeBSD?
+extern "C" {
----------------
I believe that this should work on most ELF platforms.

http://reviews.llvm.org/D6096