[PATCH] D22714: stop short-circuiting the SSP code for sspstrong

[Daniel Micay via llvm-commits]

strcat created this revision.
strcat added reviewers: hfinkel, probinson, void.
strcat added a subscriber: llvm-commits.

The StackProtector::RequiresStackProtector method is supposed to add layout information for alloca instructions that need to be protected by the canary. This is supposed to protect normal local variables (including function pointers, etc.) from linear overflows.

However, this method contains an early return for sspstrong and sspreq in the code for handling calls to alloca and variable length arrays (not regular arrays, with the IR Clang generates):

          // SSP-Strong: Enable protectors for any call to alloca, regardless
          // of size.
          if (Strong)
            return true;

The method has special handling for sspstrong/sspreq following this early return, but it's not being used. It ends up returning early, resulting in the function being protected with a canary but without marking the arrays it's trying to protect (not only the alloca/VLA triggering the issue) so they get treated as normal local variables.

Sample code:

  #include <string.h>
  #include <alloca.h>

  int foo(char *bar) {
      char *buf = alloca(20);
      strcpy(buf, bar);
      return strlen(buf);
  }

Compiler output at -O0:


```
--- old_x86.s	2016-07-22 08:44:37.534862251 -0400
+++ new_x86.s	2016-07-22 08:44:18.778486803 -0400
@@ -17,12 +17,12 @@
 	subq	$48, %rsp
 	movq	%fs:40, %rax
 	movq	%rax, -8(%rbp)
-	movq	%rdi, -24(%rbp)
-	leaq	-44(%rbp), %rdi
-	movq	%rdi, -16(%rbp)
-	movq	-24(%rbp), %rsi
+	movq	%rdi, -48(%rbp)
+	leaq	-28(%rbp), %rdi
+	movq	%rdi, -40(%rbp)
+	movq	-48(%rbp), %rsi
 	callq	strcpy
-	movq	-16(%rbp), %rdi
+	movq	-40(%rbp), %rdi
 	callq	strlen
 	movq	%fs:40, %rcx
 	cmpq	-8(%rbp), %rcx
```

https://reviews.llvm.org/D22714

Files:
  lib/CodeGen/StackProtector.cpp

Index: lib/CodeGen/StackProtector.cpp
===================================================================
--- lib/CodeGen/StackProtector.cpp
+++ lib/CodeGen/StackProtector.cpp
@@ -236,11 +236,6 @@
     for (const Instruction &I : BB) {
       if (const AllocaInst *AI = dyn_cast<AllocaInst>(&I)) {
         if (AI->isArrayAllocation()) {
-          // SSP-Strong: Enable protectors for any call to alloca, regardless
-          // of size.
-          if (Strong)
-            return true;
-
           if (const auto *CI = dyn_cast<ConstantInt>(AI->getArraySize())) {
             if (CI->getLimitedValue(SSPBufferSize) >= SSPBufferSize) {
               // A call to alloca with size >= SSPBufferSize requires


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D22714.65202.patch
Type: text/x-patch
Size: 711 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160723/6dcc8c32/attachment.bin>