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

[Peter Collingbourne]

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:340
@@ +339,3 @@
+    // throughout the function. For now we store it in an alloca.
+    DynamicTop = IRB.CreateAlloca(StackPtrTy, 0, "unsafe_stack_dynamic_ptr");
+
----------------
jfb wrote:
> `/* ArraySize= */ nullptr`
Done

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:426
@@ +425,3 @@
+    StaticOffset += Size;
+    StaticOffset = (StaticOffset + Align - 1) / Align * Align;
+
----------------
jfb wrote:
> Use `alignAddr` from `MathExtras.h` instead.
`alignAddr` takes a pointer; used `RoundUpToAlignment`.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:444
@@ +443,3 @@
+  StaticOffset =
+      (StaticOffset + StackAlignment - 1) / StackAlignment * StackAlignment;
+
----------------
jfb wrote:
> `alignAddr`
Likewise

================
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;
----------------
jfb wrote:
> pcc wrote:
> > jfb wrote:
> > > 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.
> > > Are there any read-only functions with no return value?
> > 
> > `void noop() {}` ? :)
> > 
> > > I don't see a test fo it.
> > 
> > Added.
> I asked the wrong question :-)
> 
> Are there any read-only functions with no return value which also capture pointers? The example you added looks like what I expected... but I don't get what such a function could do. It seems pretty useless! If so then it doesn't seem worth handling it specially here (why optimize something nobody in their right mind would do?).
Good point. I don't think there are, which pretty much makes this test redundant (or at least it ought to be redundant provided that we're inferring attributes correctly). Removed along with the test.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:147
@@ +146,3 @@
+
+      default:
+        // The object is unsafe if it is used in any other way.
----------------
jfb wrote:
> pcc wrote:
> > jfb wrote:
> > > 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.
> > Yes, but let's add support for these intrinsics in a separate change.
> Sounds good. Could you add a FIXME for this?
Done

http://reviews.llvm.org/D6094

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