[asan] fix false dynamic-stack-buffer-overflow report with constantly-sized dynamic allocas

[kubamracek]

Summary

See the bug report at https://github.com/google/sanitizers/issues/691. When a dynamic alloca has a constant size, ASan instrumentation will treat it as a regular dynamic alloca (insert calls to poison and unpoison), but the backend will turn it into a regular stack variable. The poisoning/unpoisoning is then broken. This patch will treat such allocas as static.

[m.ostapenko]

Hm, with this patch I got such a strange error:

$ cat test.c

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

void f1() {
  char *dynamic_buffer = (char *)alloca(200);
  fprintf(stderr, "dynamic_buffer = %p, dynamic_buffer_end = %p\n", dynamic_buffer, dynamic_buffer + 200);
  dynamic_buffer[2] = 1;
  return;
}

void f2() {
  char buf[1024];
  fprintf(stderr, "buf = %p, buf_end = %p\n", buf, buf + 1024);
  memset(buf, 'x', 1024);
}

int main(int argc, const char *argv[]) {
  f1();
  f2();
  fprintf(stderr, "Done.\n");
  return 0;
}

$ clang -fsanitize=address test.c && ./a.out

dynamic_buffer = 0x7ffed7c98a80, dynamic_buffer_end = 0x7ffed7c98b48
=================================================================
==15867==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffed7c98a82 at pc 0x0000004dbaaa bp 0x7ffed7c98a50 sp 0x7ffed7c98a48
WRITE of size 1 at 0x7ffed7c98a82 thread T0
    #0 0x4dbaa9 in f1 (/home/max/build/llvm/a.out+0x4dbaa9)
    #1 0x4dbcda in main (/home/max/build/llvm/a.out+0x4dbcda)
    #2 0x7f1a18464ec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287
    #3 0x418b95 in _start (/home/max/build/llvm/a.out+0x418b95)

Address 0x7ffed7c98a82 is located in stack of thread T0 at offset 34 in frame
    #0 0x4db95f in f1 (/home/max/build/llvm/a.out+0x4db95f)

  This frame has 1 object(s):
    [32, 33) '' <== Memory access at offset 34 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow (/home/max/build/llvm/a.out+0x4dbaa9) in f1
Shadow bytes around the buggy address:
  0x10005af8b100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10005af8b110: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10005af8b120: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10005af8b130: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10005af8b140: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
=>0x10005af8b150:[01]f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
  0x10005af8b160: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10005af8b170: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10005af8b180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10005af8b190: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10005af8b1a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb

It seems that dynamic_buffer wasn't even allocated here.

[kubamracek]

Update patch to correctly calculate the size of the dynamic alloca. Added -fstack-protector to the test cases.

[m.ostapenko]

The change looks sane to me (not sure I can push LGTM button though), just a little nit below.

projects/compiler-rt/test/asan/TestCases/alloca_constant_size.cc

3 ↗

(On Diff #61248)

Do we need to check if the target supports "-fprotector" ?

[kubamracek]

projects/compiler-rt/test/asan/TestCases/alloca_constant_size.cc

3 ↗

(On Diff #61248)

AFAIK it should be available in all ASan-supported targets.

[zaks.anna]

Since this is an IR-pass change, could you add a regression test checking the changes made by the -asan pass.

lib/Transforms/Instrumentation/AddressSanitizer.cpp

468 ↗

(On Diff #61248)

You probably can get rid of the helper function now.

906 ↗

(On Diff #61248)

Why is this needed?

[kubamracek]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

906 ↗

(On Diff #61248)

The change above makes getAllocaSizeInBytes valid only for static allocas (otherwise it asserts). This avoids calling getAllocaSizeInBytes for dynamic allocas.

[zaks.anna]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

468 ↗

(On Diff #61248)

I was trying to figure out why we have the check for AI.isArrayAllocation() here and I cannot.. Looks like it was added here:

http://reviews.llvm.org/D6055

906 ↗

(On Diff #61248)

Ok, Maybe we could get rid of the helper and place this check next to the getAllocaSizeInBytes to make it clear why we call this.

[m.ostapenko]

I think you don't need two test files, could you combine them to one source file (you'll probably want use ifdefs)?

lib/Transforms/Instrumentation/AddressSanitizer.cpp

468 ↗

(On Diff #61248)

I was trying to figure out why we have the check for AI.isArrayAllocation() here and I cannot.. Looks like it was added here:

http://reviews.llvm.org/D6055

Yeah, this is a artifact from initial implementation. I don't remember exact reason why I placed this check here, but this probably was a mistake.

[kubamracek]

Updating patch.

[m.ostapenko]

Ok, thank you for fixing this!

[kubamracek]

Landed:
LLVM part: r273888
compiler-rt part: r273889

llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp

struct AddressSanitizer : public FunctionPass {
   const char *getPassName() const override {
     return "AddressSanitizerFunctionPass";
   }
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addRequired<DominatorTreeWrapperPass>();
     AU.addRequired<TargetLibraryInfoWrapperPass>();
   }
   uint64_t getAllocaSizeInBytes(AllocaInst *AI) const {
+    uint64_t ArraySize = 1;
+    if (AI->isArrayAllocation()) {
+      ConstantInt *CI = dyn_cast<ConstantInt>(AI->getArraySize());
+      assert(CI && "non-constant array size");
+      ArraySize = CI->getZExtValue();
+    }
     Type *Ty = AI->getAllocatedType();
     uint64_t SizeInBytes =
         AI->getModule()->getDataLayout().getTypeAllocSize(Ty);
-    return SizeInBytes;
+    return SizeInBytes * ArraySize;
   }
   /// Check if we want (and can) handle this alloca.
   bool isInterestingAlloca(AllocaInst &AI);
 
-  // Check if we have dynamic alloca.
-  bool isDynamicAlloca(AllocaInst &AI) const {
-    return AI.isArrayAllocation() || !AI.isStaticAlloca();
-  }
-
   /// If it is an interesting memory access, return the PointerOperand
   /// and set IsWrite/Alignment. Otherwise return nullptr.
   Value *isInterestingMemoryAccess(Instruction *I, bool *IsWrite,
                                    uint64_t *TypeSize, unsigned *Alignment);
   void instrumentMop(ObjectSizeOffsetVisitor &ObjSizeVis, Instruction *I,
                      bool UseCalls, const DataLayout &DL);
   void instrumentPointerComparisonOrSubtraction(Instruction *I);
   void instrumentAddress(Instruction *OrigIns, Instruction *InsertBefore,
struct FunctionStackPoisoner : public InstVisitor<FunctionStackPoisoner> {
   /// \brief Collect Alloca instructions we want (and can) handle.
   void visitAllocaInst(AllocaInst &AI) {
     if (!ASan.isInterestingAlloca(AI)) {
       if (AI.isStaticAlloca()) NonInstrumentedStaticAllocaVec.insert(&AI);
       return;
     }
 
     StackAlignment = std::max(StackAlignment, AI.getAlignment());
-    if (ASan.isDynamicAlloca(AI))
+    if (!AI.isStaticAlloca())
       DynamicAllocaVec.push_back(&AI);
     else
       AllocaVec.push_back(&AI);
   }
 
   /// \brief Collect lifetime intrinsic calls to check for use-after-scope
   /// errors.
   void visitIntrinsicInst(IntrinsicInst &II) {
bool AddressSanitizer::isInterestingAlloca(AllocaInst &AI) {
   auto PreviouslySeenAllocaInfo = ProcessedAllocas.find(&AI);
 
   if (PreviouslySeenAllocaInfo != ProcessedAllocas.end())
     return PreviouslySeenAllocaInfo->getSecond();
 
   bool IsInteresting =
       (AI.getAllocatedType()->isSized() &&
        // alloca() may be called with 0 size, ignore it.
-       getAllocaSizeInBytes(&AI) > 0 &&
+       ((!AI.isStaticAlloca()) || getAllocaSizeInBytes(&AI) > 0) &&
        // We are only interested in allocas not promotable to registers.
        // Promotable allocas are common under -O0.
        (!ClSkipPromotableAllocas || !isAllocaPromotable(&AI)) &&
        // inalloca allocas are not treated as static, and we don't want
        // dynamic alloca instrumentation for them as well.
        !AI.isUsedWithInAlloca());
 
   ProcessedAllocas[&AI] = IsInteresting;
void FunctionStackPoisoner::poisonStack() {
   assert(AllocaVec.size() > 0 || DynamicAllocaVec.size() > 0);
 
   // Insert poison calls for lifetime intrinsics for alloca.
   bool HavePoisonedStaticAllocas = false;
   for (const auto &APC : AllocaPoisonCallVec) {
     assert(APC.InsBefore);
     assert(APC.AI);
     assert(ASan.isInterestingAlloca(*APC.AI));
-    bool IsDynamicAlloca = ASan.isDynamicAlloca(*APC.AI);
+    bool IsDynamicAlloca = !(*APC.AI).isStaticAlloca();
     if (!ClInstrumentAllocas && IsDynamicAlloca)
       continue;
 
     IRBuilder<> IRB(APC.InsBefore);
     poisonAlloca(APC.AI, APC.Size, IRB, APC.DoPoison);
     // Dynamic allocas will be unpoisoned unconditionally below in
     // unpoisonDynamicAllocas.
     // Flag that we need unpoison static allocas.