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

David Chisnall csdavec at swan.ac.uk
Mon Nov 3 10:20:14 PST 2014

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?
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,
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
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
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__
Why is this not on Linux?

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

Comment at: lib/safestack/safestack.cc:327
@@ +326,3 @@
+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.


More information about the cfe-commits mailing list