[PATCH] Protection against stack-based memory corruption errors using SafeStack

[JF Bastien]

Some early comments, I haven't finished going through the changes but this should be a good start!


================
Comment at: lib/Transforms/IPO/Inliner.cpp:97
@@ -97,1 +96,3 @@
+    .addAttribute(Attribute::StackProtectStrong)
+    .addAttribute(Attribute::StackProtectReq);
   AttributeSet OldSSPAttr = AttributeSet::get(Caller->getContext(),
----------------
Was `StackProtectReq` simply missing?

It looks like there's a lattice of stack protection mechanisms, but it's not documented AFAICT. Could you document it somewhere?

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:12
@@ +11,3 @@
+// and the unsafe stack (explicitly allocated and managed through the runtime
+// support library).
+//
----------------
Add link to safe stack docs.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:16
@@ +15,3 @@
+
+#define DEBUG_TYPE "safestack"
+#include "llvm/Transforms/Instrumentation.h"
----------------
`DEBUG_TYPE` has to be after includes to avoid nasty ODR violations.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:94
@@ +93,3 @@
+        if (!cast<const GetElementPtrInst>(I)->hasAllConstantIndices())
+          // GEP with non-constant indices can lead to memory errors.
+          return false;
----------------
I'm assuming that's also true if the GEP is `inbounds`? Could the comment also state this.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:124
@@ +123,3 @@
+        // the object is considered safe if a pointer to it is passed to a
+        // function that only reads memory nor returns any value. This function
+        // can neither do unsafe writes itself nor capture the pointer (or
----------------
"a function that only reads memory nor returns any value" The "nor" seems wrong here.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:129
@@ +128,3 @@
+        // exception or not depending on the input value).
+        if (CS.onlyReadsMemory() && I->getType()->isVoidTy())
+          continue;
----------------
Are there any read-only functions with no return value? I'm not sure that makes much sense, and I don't see a test fo it.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:147
@@ +146,3 @@
+
+      default:
+        // The object is unsafe if it is used in any other way.
----------------
Don't you end up seeing uses of alloca through intrinsics such as `memset` and `memcpy`? They can be added if needed, but since these two act the same as load/store I'd think that they are safe.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:172
@@ +171,3 @@
+
+  enum { StackAlignment = 16 };
+
----------------
Why 16? Could you also document how this is used, and how it takes into account target alignment?

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:254
@@ +253,3 @@
+        /*InsertBefore=*/nullptr,
+        /*ThreadLocalMode=*/GlobalValue::InitialExecTLSModel);
+  } else {
----------------
This TLS model is for variables in modules that will not be loaded dynamically. This sanitizer can be used in executables, and with shared libraries that don't have safestack, but not with executables that used shared libraries with safestack?

Shouldn't this use GeneralDynamicTLSModel instead?

I'd like a comment that explains the choice.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:301
@@ +300,3 @@
+  }
+}
+
----------------
Do the `llvm.stacksave`/`llvm.stackrestore`, `gcroot`, `frameaddress` intrinsics need to be handled?

http://reviews.llvm.org/D6094

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