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

[Peter Collingbourne]

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:507
@@ +506,3 @@
+  /// space, and populates the address space and offset as appropriate.
+  bool getUnsafeStackPtrLocation(unsigned &AddressSpace,
+                                 unsigned &Offset) const;
----------------
kcc wrote:
> Do we need this at all? 
Removed. We can easily restore this hook if we ever need it.

================
Comment at: lib/CodeGen/LLVMBuild.txt:25
@@ -24,2 +24,2 @@
 parent = Libraries
-required_libraries = Analysis Core MC Scalar Support Target TransformUtils
+required_libraries = Analysis Core Instrumentation MC Scalar Support Target TransformUtils
----------------
kcc wrote:
> why do you need this? 
Because the CodeGen library now depends on the `createSafeStackPass` function declared in Instrumentation.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:105
@@ +104,3 @@
+
+        /* fallthough */
+
----------------
kcc wrote:
> Maybe this? 
> http://clang.llvm.org/docs/AttributeReference.html#fallthrough-clang-fallthrough
Done, introducing a macro LLVM_FALLTHROUGH in `llvm/Support/Compiler.h` because of r234788.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:163
@@ +162,3 @@
+/// local variables that are accessed in unsafe ways.
+class SafeStack : public ModulePass {
+  const DataLayout *DL;
----------------
kcc wrote:
> does it have to be a module pass? 
> why not a function pass? 
It can be a function pass; done.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:244
@@ +243,3 @@
+
+  // Check where the unsafe stack pointer is stored on this architecture
+  unsigned AddressSpace, Offset;
----------------
kcc wrote:
> here and elsewhere: period after a comment. 
Done

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:249
@@ +248,3 @@
+    // (usually in the thread control block)
+    Constant *OffsetVal = ConstantInt::get(Int32Ty, Offset);
+    return ConstantExpr::getIntToPtr(
----------------
kcc wrote:
> why int32 here? 
This code is now gone.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:252
@@ +251,3 @@
+        OffsetVal, Int8Ty->getPointerTo()->getPointerTo(AddressSpace));
+  } else {
+    // The unsafe stack pointer is stored in a global variable with a magic name
----------------
kcc wrote:
> which of the ways is used where? do we need both? 
The first code path was associated with the `getUnsafeStackPtrLocation` hook, which is now gone, so I've removed it.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:256
@@ +255,3 @@
+    // FIXME: make the name start with "llvm."
+    static const char* unsafe_stack_ptr_var = "__safestack_unsafe_stack_ptr";
+
----------------
kcc wrote:
> const variables are named like kUnsafeStackPtrVar
Done

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:271
@@ +270,3 @@
+      // The variable exists, check its type and attributes
+      if (UnsafeStackPtr->getValueType() != Int8Ty->getPointerTo()) {
+        report_fatal_error(Twine(unsafe_stack_ptr_var) +
----------------
kcc wrote:
> why not just assert? 
It is possible to fail these checks with "valid" (modulo the use of reserved identifiers) C translation units. Consider for example the following translation unit which uses `__safestack_unsafe_stack_ptr` as both a regular and thread-local variable.

```
void *__safestack_unsafe_stack_ptr = 0;

void f(char *);

void g() {
  char foo[64];
  f(foo);
}
```

Rather than asserting (or crashing horribly later in non-asserts builds), it would be better to abort early with `report_fatal_error`.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:286
@@ +285,3 @@
+
+bool SafeStack::runOnFunction(Function &F) {
+  ++NumFunctions;
----------------
kcc wrote:
> This function is too big. Please split it into as many smaller ones as possible. With comments. 
Done

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:289
@@ +288,3 @@
+
+  unsigned StackAlignment = 16;
+  Constant *UnsafeStackPtr = getOrCreateUnsafeStackPtr(F);
----------------
kcc wrote:
> const?
Now an enum

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:328
@@ +327,3 @@
+      if (CI->getCalledFunction() && CI->canReturnTwice())
+          //CI->getCalledFunction()->getName() == "_setjmp")
+        StackRestorePoints.push_back(CI);
----------------
kcc wrote:
> do you need this comment? 
Removed

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:541
@@ +540,3 @@
+    Value *CurrentTop = DynamicTop ? IRB.CreateLoad(DynamicTop) : StaticTop;
+    IRB.CreateStore(CurrentTop, UnsafeStackPtr);
+  }
----------------
kcc wrote:
> Is this tested in any of the unit tests? 
This is now covered by `setjmp.ll` and `setjmp2.ll`.

================
Comment at: test/Transforms/SafeStack/safestack.ll:1
@@ +1,2 @@
+; RUN: opt -safe-stack -S -mtriple=i386-pc-linux-gnu < %s -o - | FileCheck --check-prefix=CHECK --check-prefix=LINUX %s
+; RUN: opt -safe-stack -S -mtriple=x86_64-pc-linux-gnu < %s -o - | FileCheck --check-prefix=CHECK --check-prefix=LINUX %s
----------------
kcc wrote:
> A comment would be nice. 
> Explain why you have CHECK and LINUX
> Consider splitting this test into several
> Explain why you have CHECK and LINUX

The latter was testing the non-`getUnsafeStackPtrLocation` case, which is now the only case; removed.

> Consider splitting this test into several

Done

http://reviews.llvm.org/D6094

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