[clang/asan] call __asan_poison_cxx_array_cookie after operator new[]

[kcc]

Summary

PR19838
When operator new[] is called and an array cookie is created
we want asan to detect buffer overflow bugs that touch the cookie.
For that we need to

a) poison the shadow for the array cookie (call __asan_poison_cxx_array_cookie).
b) ignore the legal accesses to the cookie generated by clang (add 'nosanitize' metadata)

[kcc]

remove unnecessary file

[kcc]

ping?

[samsonov]

Sorry for delay. Looking at it now.

[samsonov]

lib/CodeGen/ItaniumCXXABI.cpp

1450

Hm, I think we'd need to ask Timur to implement similar logic for Microsoft ABI?

1482

Please start local variable names with capitals

1483

Why don't you use fixed type for __asan_poison_cxx_array_cookie (like IntptrTy)?

[rsmith]

Is this change correct? Suppose I do this:

char Buffer[32];
// ...
new (Buffer) int[4];
// ...
new (Buffer) int(0);

Won't we get a false positive on the last line?

[samsonov]

I thought of one more test case we should add:

__attribute__((no_sanitize_address))
int *createArray(int n) {
  return new int[n];
}

int bad_access() {  
  int *array = createArray(4);
  return array[-1];
}

We certainly want to print an error in this case, even though we have attribute on createArray() function. I believe current code *would* handle this correctly, but let's test this behavior.

[timurrrr]

Am I right that skipping MicrosoftCXXABI for a while won't give us false positives?
If so, I'd wait until a change to ItaniumCXXABI lands and then I can just do that for MicrosoftCXXABI.

[samsonov]

Sure, we can add support for this to MicrosoftCXXABI later.

[kcc]

In D4774#11, @samsonov wrote:

I thought of one more test case we should add:

__attribute__((no_sanitize_address))
int *createArray(int n) {
  return new int[n];
}

int bad_access() {  
  int *array = createArray(4);
  return array[-1];
}

We certainly want to print an error in this case, even though we have attribute on createArray() function. I believe current code *would* handle this correctly, but let's test this behavior.

I'd prefer to add such test to compiler-rt, not to clang, as it is a run-time test.

In D4774#10, @rsmith wrote:

Is this change correct? Suppose I do this:

char Buffer[32];
// ...
new (Buffer) int[4];
// ...
new (Buffer) int(0);

Won't we get a false positive on the last line?

Not sure I understand this test.
First, with arrays of PODs you don't have cookies at all.
Second, do we have the cookie with placement new at all?
Pardon my ignorance here; this is my test case:

#include <stdio.h>
#include <new>
struct C {
  int x;
  ~C() {fprintf(stderr, "ZZZZZZZZ\n");}
};
char Buffer[100];
C *c;
int main(int argc, char **argv) { 
  c = new (Buffer) C[argc];
}

clang++ -O -fsanitize=address ~/tmp/new_ar_placement.cc -S -o - -emit-llvm

define i32 @main(i32 %argc, i8** nocapture readnone %argv) #0 {
entry:

store %struct.C* bitcast ({ [100 x i8], [60 x i8] }* @Buffer to %struct.C*), %struct.C** getelementptr inbounds ({ %struct.C*, [56 x i8] }* @c, i32 0, i32 0), align 8, !tbaa !5
ret i32 0

}

// No cookie

lib/CodeGen/ItaniumCXXABI.cpp

1450

yes, but not in this round

1482

done

1483

What's wrong with i64* ?
Having I64 here will just require one more cast

[kcc]

address some of the comments

[kcc]

Don't insert insrumentation for placement new, add a relevant test.

Richard, PTAL

[rsmith]

lib/CodeGen/ItaniumCXXABI.cpp

1480

Use expr->getOperatorNew()->isReplaceableGlobalAllocationFunction() here, so that you also sanitize new (std::nothrow) X[n].

[kcc]

Use expr->getOperatorNew()->isReplaceableGlobalAllocationFunction()
and add a test to verify that new (std::nothrow) X[n] is instrumented.

PTAL

[rsmith]

LGTM

lib/CodeGen/CodeGenFunction.cpp

CodeGenFunction::SanitizerScope::~SanitizerScope() {
   CGF->IsSanitizerScope = false;
 }
 
 void CodeGenFunction::InsertHelper(llvm::Instruction *I,
                                    const llvm::Twine &Name,
                                    llvm::BasicBlock *BB,
                                    llvm::BasicBlock::iterator InsertPt) const {
   LoopStack.InsertHelper(I);
-  if (IsSanitizerScope) {
-    I->setMetadata(
-        CGM.getModule().getMDKindID("nosanitize"),
-        llvm::MDNode::get(CGM.getLLVMContext(), ArrayRef<llvm::Value *>()));
-  }
+  if (IsSanitizerScope)
+    CGM.getSanitizerMetadata()->disableSanitizerForInstruction(I);
 }
 
 template <bool PreserveNames>
 void CGBuilderInserter<PreserveNames>::InsertHelper(
     llvm::Instruction *I, const llvm::Twine &Name, llvm::BasicBlock *BB,
     llvm::BasicBlock::iterator InsertPt) const {
   llvm::IRBuilderDefaultInserter<PreserveNames>::InsertHelper(I, Name, BB,
                                                               InsertPt);

lib/CodeGen/ItaniumCXXABI.cpp

 
 CharUnits ItaniumCXXABI::getArrayCookieSizeImpl(QualType elementType) {
   // The array cookie is a size_t; pad that up to the element alignment.
   // The cookie is actually right-justified in that space.
   return std::max(CharUnits::fromQuantity(CGM.SizeSizeInBytes),
                   CGM.getContext().getTypeAlignInChars(elementType));
 }
 
 llvm::Value *ItaniumCXXABI::InitializeArrayCookie(CodeGenFunction &CGF,
                                                   llvm::Value *NewPtr,
                                                   llvm::Value *NumElements,
                                                   const CXXNewExpr *expr,
                                                   QualType ElementType) {
   assert(requiresArrayCookie(expr));
 
   unsigned AS = NewPtr->getType()->getPointerAddressSpace();
 
llvm::Value *ItaniumCXXABI::InitializeArrayCookie(CodeGenFunction &CGF,
   // Compute an offset to the cookie.
   llvm::Value *CookiePtr = NewPtr;
   CharUnits CookieOffset = CookieSize - SizeSize;
   if (!CookieOffset.isZero())
     CookiePtr = CGF.Builder.CreateConstInBoundsGEP1_64(CookiePtr,
                                                  CookieOffset.getQuantity());
 
   // Write the number of elements into the appropriate slot.
-  llvm::Value *NumElementsPtr
-    = CGF.Builder.CreateBitCast(CookiePtr,
-                                CGF.ConvertType(SizeTy)->getPointerTo(AS));
-  CGF.Builder.CreateStore(NumElements, NumElementsPtr);
+  llvm::Type *NumElementsTy = CGF.ConvertType(SizeTy)->getPointerTo(AS);
+  llvm::Value *NumElementsPtr =
+      CGF.Builder.CreateBitCast(CookiePtr, NumElementsTy);
+  llvm::Instruction *SI = CGF.Builder.CreateStore(NumElements, NumElementsPtr);
+  if (CGM.getLangOpts().Sanitize.Address) {
+    CGM.getSanitizerMetadata()->disableSanitizerForInstruction(SI);
+    llvm::FunctionType *FTy =
+        llvm::FunctionType::get(CGM.VoidTy, NumElementsTy, false);
+    llvm::Constant *F =
+        CGM.CreateRuntimeFunction(FTy, "__asan_poison_cxx_array_cookie");
+    CGF.Builder.CreateCall(F, NumElementsPtr);
+  }
 
   // Finally, compute a pointer to the actual data buffer by skipping
   // over the cookie completely.
   return CGF.Builder.CreateConstInBoundsGEP1_64(NewPtr,
                                                 CookieSize.getQuantity());  
 }
 
 llvm::Value *ItaniumCXXABI::readArrayCookieImpl(CodeGenFunction &CGF,
                                                 llvm::Value *allocPtr,
                                                 CharUnits cookieSize) {
   // The element size is right-justified in the cookie.
   llvm::Value *numElementsPtr = allocPtr;
   CharUnits numElementsOffset =
     cookieSize - CharUnits::fromQuantity(CGF.SizeSizeInBytes);
   if (!numElementsOffset.isZero())
     numElementsPtr =
       CGF.Builder.CreateConstInBoundsGEP1_64(numElementsPtr,
                                              numElementsOffset.getQuantity());
 
   unsigned AS = allocPtr->getType()->getPointerAddressSpace();
   numElementsPtr = 
     CGF.Builder.CreateBitCast(numElementsPtr, CGF.SizeTy->getPointerTo(AS));
-  return CGF.Builder.CreateLoad(numElementsPtr);
+  llvm::Instruction *LI = CGF.Builder.CreateLoad(numElementsPtr);
+  if (CGM.getLangOpts().Sanitize.Address)
+    CGM.getSanitizerMetadata()->disableSanitizerForInstruction(LI);
+  return LI;
 }
 
 CharUnits ARMCXXABI::getArrayCookieSizeImpl(QualType elementType) {
   // ARM says that the cookie is always:
   //   struct array_cookie {
   //     std::size_t element_size; // element_size != 0
   //     std::size_t element_count;
   //   };

lib/CodeGen/SanitizerMetadata.h

 #ifndef LLVM_CLANG_LIB_CODEGEN_SANITIZERMETADATA_H
 #define LLVM_CLANG_LIB_CODEGEN_SANITIZERMETADATA_H
 
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 
 namespace llvm {
 class GlobalVariable;
+class Instruction;
 class MDNode;
 }
 
 namespace clang {
 class VarDecl;
 
 namespace CodeGen {
 
 class CodeGenModule;
 
 class SanitizerMetadata {
   SanitizerMetadata(const SanitizerMetadata &) LLVM_DELETED_FUNCTION;
   void operator=(const SanitizerMetadata &) LLVM_DELETED_FUNCTION;
 
   CodeGenModule &CGM;
 public:
   SanitizerMetadata(CodeGenModule &CGM);
   void reportGlobalToASan(llvm::GlobalVariable *GV, const VarDecl &D,
                           bool IsDynInit = false);
   void reportGlobalToASan(llvm::GlobalVariable *GV, SourceLocation Loc,
                           StringRef Name, bool IsDynInit = false,
                           bool IsBlacklisted = false);
   void disableSanitizerForGlobal(llvm::GlobalVariable *GV);
+  void disableSanitizerForInstruction(llvm::Instruction *I);
 private:
   llvm::MDNode *getLocationMetadata(SourceLocation Loc);
 };
 }  // end namespace CodeGen
 }  // end namespace clang
 
 #endif

lib/CodeGen/SanitizerMetadata.cpp

 
 void SanitizerMetadata::disableSanitizerForGlobal(llvm::GlobalVariable *GV) {
   // For now, just make sure the global is not modified by the ASan
   // instrumentation.
   if (CGM.getLangOpts().Sanitize.Address)
     reportGlobalToASan(GV, SourceLocation(), "", false, true);
 }
 
+void SanitizerMetadata::disableSanitizerForInstruction(llvm::Instruction *I) {
+  I->setMetadata(
+      CGM.getModule().getMDKindID("nosanitize"),
+      llvm::MDNode::get(CGM.getLLVMContext(), ArrayRef<llvm::Value *>()));
+}
+
 llvm::MDNode *SanitizerMetadata::getLocationMetadata(SourceLocation Loc) {
   PresumedLoc PLoc = CGM.getContext().getSourceManager().getPresumedLoc(Loc);
   if (!PLoc.isValid())
     return nullptr;
   llvm::LLVMContext &VMContext = CGM.getLLVMContext();
   llvm::Value *LocMetadata[] = {
       llvm::MDString::get(VMContext, PLoc.getFilename()),
       llvm::ConstantInt::get(llvm::Type::getInt32Ty(VMContext), PLoc.getLine()),
       llvm::ConstantInt::get(llvm::Type::getInt32Ty(VMContext),
                              PLoc.getColumn()),
   };
   return llvm::MDNode::get(VMContext, LocMetadata);
 }

test/CodeGen/address-sanitizer-and-array-cookie.cpp

+// RUN: %clang_cc1 -triple x86_64-gnu-linux -emit-llvm -o - %s | FileCheck %s -check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-gnu-linux -emit-llvm -o - -fsanitize=address %s | FileCheck %s -check-prefix=ASAN
+
+struct C {
+  int x;
+  ~C();
+};
+
+C *CallNew() {
+  return new C[10];
+}
+// PLAIN-LABEL: CallNew
+// PLAIN-NOT: nosanitize
+// PLAIN-NOT: __asan_poison_cxx_array_cookie
+// ASAN-LABEL: CallNew
+// ASAN: store{{.*}}nosanitize
+// ASAN-NOT: nosanitize
+// ASAN: call void @__asan_poison_cxx_array_cookie
+
+void CallDelete(C *c) {
+  delete [] c;
+}
+
+// PLAIN-LABEL: CallDelete
+// PLAIN-NOT: nosanitize
+// ASAN-LABEL: CallDelete
+// ASAN: load{{.*}}!nosanitize
+// ASAN-NOT: nosanitize