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

[JF Bastien]

lgtm after the two comments are addressed.


================
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)
----------------
pcc wrote:
> jfb wrote:
> > 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.
> > What's missing for Windows support? Thread and TLS support?
> 
> LLVM already supports TLS on Windows (I believe), but we will have to find some way to intercept thread creation to allocate both stacks.
> 
> > This can be a different patch, but I think it's important.
> 
> Agree.
Tagging @rnk FYI.

================
Comment at: lib/safestack/safestack.cc:61
@@ +60,3 @@
+static inline void unsafe_stack_setup(void *start, size_t size, size_t guard) {
+  void* stack_ptr = (char*) start + size;
+  CHECK_EQ((((size_t)stack_ptr) & (kStackAlign-1)), 0);
----------------
Overflow check here too, and with guard.

================
Comment at: lib/safestack/safestack.cc:174
@@ +173,3 @@
+  if (initialized)
+    return;
+
----------------
ksvladimir wrote:
> pcc wrote:
> > jfb wrote:
> > > 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.
> > > Is this initialization check useful if __attribute__((constructor(0))) was used?
> > 
> > I don't think it is. Removed.
> > 
> > (Regarding Vova's comment, we don't currently support linking SafeStack into DSOs.)
> The primary reason for this check is to handle cases when libsafestack is linked into multiple DSOs and __safestack_init ends up being on multiple constructor lists. It can only be called concurrently if multiple DSOs are being dlopen'ed in parallel. I think that dlopen itself isn't thread-safe and shouldn't be used that way but, perhaps, for extra safety it's useful to make this code non-racy using atomics.
Gotcha. Would it be useful to have nothing here for now (@pcc just deleted the code), but add a comment explaining @ksvladimir's point?

http://reviews.llvm.org/D6096

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