Insert poisoned paddings between fields in C++ classes so that AddressSanitizer can find intra-object-overflow bugs

Summary:
The general approach is to add extra paddings after every field
in AST/RecordLayoutBuilder.cpp, then add code to CTORs/DTORs that poisons the paddings
(CodeGen/CGClass.cpp).

Everything is done under the flag -fsanitize-address-field-padding. 
The blacklist file (-fsanitize-blacklist) allows to avoid the transformation 
for given classes or source files. 

See also https://code.google.com/p/address-sanitizer/wiki/IntraObjectOverflow

Test Plan: run SPEC2006 and some of the Chromium tests with  -fsanitize-address-field-padding

Reviewers: samsonov, rnk, rsmith

Reviewed By: rsmith

Subscribers: majnemer, cfe-commits

Differential Revision: http://reviews.llvm.org/D5687

llvm-svn: 219961

clang/include/clang/AST/Decl.h

@@ -3263,6 +3263,11 @@ public:
   /// commandline option.
   bool isMsStruct(const ASTContext &C) const;
 
+  /// \brief Whether we are allowed to insert extra padding between fields.
+  /// These padding are added to help AddressSanitizer detect
+  /// intra-object-overflow bugs.
+  bool mayInsertExtraPadding(bool EmitRemark = false) const;
+
 private:
   /// \brief Deserialize just the fields.
   void LoadFieldsFromExternalStorage() const;

clang/include/clang/Basic/DiagnosticFrontendKinds.td

@@ -46,6 +46,16 @@ def warn_fe_backend_optimization_failure : Warning<"%0">, BackendInfo,
 def note_fe_backend_optimization_remark_invalid_loc : Note<"could "
   "not determine the original source location for %0:%1:%2">;
 
+def remark_sanitize_address_insert_extra_padding_accepted : Remark<
+    "-fsanitize-address-field-padding applied to %0">, ShowInSystemHeader,
+    InGroup<SanitizeAddressRemarks>;
+def remark_sanitize_address_insert_extra_padding_rejected : Remark<
+    "-fsanitize-address-field-padding ignored for %0 because it "
+    "%select{is not C++|is packed|is a union|is trivially copyable|"
+    "has trivial destructor|is standard layout|is in a blacklisted file|"
+    "is blacklisted}1">, ShowInSystemHeader,
+    InGroup<SanitizeAddressRemarks>;
+
 def err_fe_invalid_code_complete_file : Error<
     "cannot locate code-completion file %0">, DefaultFatal;
 def err_fe_stdout_binary : Error<"unable to change standard output to binary">,

clang/include/clang/Basic/DiagnosticGroups.td

@@ -727,6 +727,9 @@ def BackendOptimizationFailure : DiagGroup<"pass-failed">;
 def ProfileInstrOutOfDate : DiagGroup<"profile-instr-out-of-date">;
 def ProfileInstrUnprofiled : DiagGroup<"profile-instr-unprofiled">;
 
+// AddressSanitizer frontent instrumentation remarks.
+def SanitizeAddressRemarks : DiagGroup<"sanitize-address">;
+
 // A warning group for warnings about code that clang accepts when
 // compiling CUDA C/C++ but which is not compatible with the CUDA spec.
 def CudaCompat : DiagGroup<"cuda-compat">;

clang/include/clang/Basic/SanitizerBlacklist.h

@@ -35,7 +35,8 @@ public:
   bool isIn(const llvm::Function &F) const;
   bool isIn(const llvm::GlobalVariable &G,
             StringRef Category = StringRef()) const;
-  bool isBlacklistedType(StringRef MangledTypeName) const;
+  bool isBlacklistedType(StringRef MangledTypeName,
+                         StringRef Category = StringRef()) const;
   bool isBlacklistedFunction(StringRef FunctionName) const;
   bool isBlacklistedFile(StringRef FileName,
                          StringRef Category = StringRef()) const;

clang/lib/AST/Decl.cpp

@@ -29,6 +29,7 @@
 #include "clang/Basic/Module.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TargetInfo.h"
+#include "clang/Frontend/FrontendDiagnostic.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <algorithm>
 
@@ -3615,6 +3616,49 @@ void RecordDecl::LoadFieldsFromExternalStorage() const {
                                                  /*FieldsAlreadyLoaded=*/false);
 }
 
+bool RecordDecl::mayInsertExtraPadding(bool EmitRemark) const {
+  ASTContext &Context = getASTContext();
+  if (!Context.getLangOpts().Sanitize.Address ||
+      !Context.getLangOpts().Sanitize.SanitizeAddressFieldPadding)
+    return false;
+  auto &Blacklist = Context.getSanitizerBlacklist();
+  const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(this);
+  StringRef Filename = Context.getSourceManager().getFilename(getLocation());
+  // We may be able to relax some of these requirements.
+  int ReasonToReject = -1;
+  if (!CXXRD || CXXRD->isExternCContext())
+    ReasonToReject = 0;  // is not C++.
+  else if (CXXRD->hasAttr<PackedAttr>())
+    ReasonToReject = 1;  // is packed.
+  else if (CXXRD->isUnion())
+    ReasonToReject = 2;  // is a union.
+  else if (CXXRD->isTriviallyCopyable())
+    ReasonToReject = 3;  // is trivially copyable.
+  else if (CXXRD->hasTrivialDestructor())
+    ReasonToReject = 4;  // has trivial destructor.
+  else if (CXXRD->isStandardLayout())
+    ReasonToReject = 5;  // is standard layout.
+  else if (Blacklist.isBlacklistedFile(Filename, "field-padding"))
+    ReasonToReject = 6;  // is in a blacklisted file.
+  else if (Blacklist.isBlacklistedType(getQualifiedNameAsString(),
+                                       "field-padding"))
+    ReasonToReject = 7;  // is blacklisted.
+
+  if (EmitRemark) {
+    if (ReasonToReject >= 0)
+      Context.getDiagnostics().Report(
+          getLocation(),
+          diag::remark_sanitize_address_insert_extra_padding_rejected)
+          << getQualifiedNameAsString() << ReasonToReject;
+    else
+      Context.getDiagnostics().Report(
+          getLocation(),
+          diag::remark_sanitize_address_insert_extra_padding_accepted)
+          << getQualifiedNameAsString();
+  }
+  return ReasonToReject < 0;
+}
+
 //===----------------------------------------------------------------------===//
 // BlockDecl Implementation
 //===----------------------------------------------------------------------===//

clang/lib/AST/RecordLayoutBuilder.cpp

@@ -652,7 +652,7 @@ protected:
   void Layout(const ObjCInterfaceDecl *D);
 
   void LayoutFields(const RecordDecl *D);
-  void LayoutField(const FieldDecl *D);
+  void LayoutField(const FieldDecl *D, bool InsertExtraPadding);
   void LayoutWideBitField(uint64_t FieldSize, uint64_t TypeSize,
                           bool FieldPacked, const FieldDecl *D);
   void LayoutBitField(const FieldDecl *D);
@@ -1331,7 +1331,7 @@ void RecordLayoutBuilder::Layout(const ObjCInterfaceDecl *D) {
   // Layout each ivar sequentially.
   for (const ObjCIvarDecl *IVD = D->all_declared_ivar_begin(); IVD;
        IVD = IVD->getNextIvar())
-    LayoutField(IVD);
+    LayoutField(IVD, false);
 
   // Finally, round the size of the total struct up to the alignment of the
   // struct itself.
@@ -1341,8 +1341,9 @@ void RecordLayoutBuilder::Layout(const ObjCInterfaceDecl *D) {
 void RecordLayoutBuilder::LayoutFields(const RecordDecl *D) {
   // Layout each field, for now, just sequentially, respecting alignment.  In
   // the future, this will need to be tweakable by targets.
+  bool InsertExtraPadding = D->mayInsertExtraPadding(/*EmitRemark=*/true);
   for (const auto *Field : D->fields())
-    LayoutField(Field);
+    LayoutField(Field, InsertExtraPadding);
 }
 
 void RecordLayoutBuilder::LayoutWideBitField(uint64_t FieldSize,
@@ -1645,7 +1646,8 @@ void RecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
                   Context.toCharUnitsFromBits(UnpackedFieldAlign));
 }
 
-void RecordLayoutBuilder::LayoutField(const FieldDecl *D) {  
+void RecordLayoutBuilder::LayoutField(const FieldDecl *D,
+                                      bool InsertExtraPadding) {
   if (D->isBitField()) {
     LayoutBitField(D);
     return;
@@ -1749,6 +1751,15 @@ void RecordLayoutBuilder::LayoutField(const FieldDecl *D) {
                       Context.toBits(UnpackedFieldOffset),
                       Context.toBits(UnpackedFieldAlign), FieldPacked, D);
 
+  if (InsertExtraPadding && !FieldSize.isZero()) {
+    CharUnits ASanAlignment = CharUnits::fromQuantity(8);
+    CharUnits ExtraSizeForAsan = ASanAlignment;
+    if (FieldSize % ASanAlignment)
+      ExtraSizeForAsan +=
+          ASanAlignment - CharUnits::fromQuantity(FieldSize % ASanAlignment);
+    FieldSize += ExtraSizeForAsan;
+  }
+
   // Reserve space for this field.
   uint64_t FieldSizeInBits = Context.toBits(FieldSize);
   if (IsUnion)

clang/lib/Basic/SanitizerBlacklist.cpp

@@ -44,8 +44,9 @@ bool SanitizerBlacklist::isIn(const llvm::GlobalVariable &G,
          SCL->inSection("type", GetGlobalTypeString(G), Category);
 }
 
-bool SanitizerBlacklist::isBlacklistedType(StringRef MangledTypeName) const {
-  return SCL->inSection("type", MangledTypeName);
+bool SanitizerBlacklist::isBlacklistedType(StringRef MangledTypeName,
+                                           StringRef Category) const {
+  return SCL->inSection("type", MangledTypeName, Category);
 }
 
 bool SanitizerBlacklist::isBlacklistedFunction(StringRef FunctionName) const {

clang/lib/CodeGen/CGClass.cpp

@@ -702,8 +702,76 @@ static bool IsConstructorDelegationValid(const CXXConstructorDecl *Ctor) {
   return true;
 }
 
+// Emit code in ctor (Prologue==true) or dtor (Prologue==false)
+// to poison the extra field paddings inserted under
+// -fsanitize-address-field-padding=1|2.
+void CodeGenFunction::EmitAsanPrologueOrEpilogue(bool Prologue) {
+  ASTContext &Context = getContext();
+  const CXXRecordDecl *ClassDecl =
+      Prologue ? cast<CXXConstructorDecl>(CurGD.getDecl())->getParent()
+               : cast<CXXDestructorDecl>(CurGD.getDecl())->getParent();
+  if (!ClassDecl->mayInsertExtraPadding()) return;
+
+  struct SizeAndOffset {
+    uint64_t Size;
+    uint64_t Offset;
+  };
+
+  unsigned PtrSize = CGM.getDataLayout().getPointerSizeInBits();
+  const ASTRecordLayout &Info = Context.getASTRecordLayout(ClassDecl);
+
+  // Populate sizes and offsets of fields.
+  SmallVector<SizeAndOffset, 16> SSV(Info.getFieldCount());
+  for (unsigned i = 0, e = Info.getFieldCount(); i != e; ++i)
+    SSV[i].Offset =
+        Context.toCharUnitsFromBits(Info.getFieldOffset(i)).getQuantity();
+
+  size_t NumFields = 0;
+  for (const auto *Field : ClassDecl->fields()) {
+    const FieldDecl *D = Field;
+    std::pair<CharUnits, CharUnits> FieldInfo =
+        Context.getTypeInfoInChars(D->getType());
+    CharUnits FieldSize = FieldInfo.first;
+    assert(NumFields < SSV.size());
+    SSV[NumFields].Size = D->isBitField() ? 0 : FieldSize.getQuantity();
+    NumFields++;
+  }
+  assert(NumFields == SSV.size());
+  if (SSV.size() <= 1) return;
+
+  // We will insert calls to __asan_* run-time functions.
+  // LLVM AddressSanitizer pass may decide to inline them later.
+  llvm::Type *Args[2] = {IntPtrTy, IntPtrTy};
+  llvm::FunctionType *FTy =
+      llvm::FunctionType::get(CGM.VoidTy, Args, false);
+  llvm::Constant *F = CGM.CreateRuntimeFunction(
+      FTy, Prologue ? "__asan_poison_intra_object_redzone"
+                    : "__asan_unpoison_intra_object_redzone");
+
+  llvm::Value *ThisPtr = LoadCXXThis();
+  ThisPtr = Builder.CreatePtrToInt(ThisPtr, IntPtrTy);
+  QualType RecordTy = Context.getTypeDeclType(ClassDecl);
+  uint64_t TypeSize = Context.getTypeSizeInChars(RecordTy).getQuantity();
+
+  // For each field check if it has sufficient padding,
+  // if so (un)poison it with a call.
+  for (size_t i = 0; i < SSV.size(); i++) {
+    uint64_t AsanAlignment = 8;
+    uint64_t NextField = i == SSV.size() - 1 ? TypeSize : SSV[i + 1].Offset;
+    uint64_t PoisonSize = NextField - SSV[i].Offset - SSV[i].Size;
+    uint64_t EndOffset = SSV[i].Offset + SSV[i].Size;
+    if (PoisonSize < AsanAlignment || !SSV[i].Size ||
+        (NextField % AsanAlignment) != 0)
+      continue;
+    Builder.CreateCall2(
+        F, Builder.CreateAdd(ThisPtr, Builder.getIntN(PtrSize, EndOffset)),
+        Builder.getIntN(PtrSize, PoisonSize));
+  }
+}
+
 /// EmitConstructorBody - Emits the body of the current constructor.
 void CodeGenFunction::EmitConstructorBody(FunctionArgList &Args) {
+  EmitAsanPrologueOrEpilogue(true);
   const CXXConstructorDecl *Ctor = cast<CXXConstructorDecl>(CurGD.getDecl());
   CXXCtorType CtorType = CurGD.getCtorType();
 
@@ -792,7 +860,10 @@ namespace {
         FirstField(nullptr), LastField(nullptr), FirstFieldOffset(0),
         LastFieldOffset(0), LastAddedFieldIndex(0) {}
 
-    static bool isMemcpyableField(FieldDecl *F) {
+    bool isMemcpyableField(FieldDecl *F) const {
+      // Never memcpy fields when we are adding poisoned paddings.
+      if (CGF.getContext().getLangOpts().Sanitize.SanitizeAddressFieldPadding)
+        return false;
       Qualifiers Qual = F->getType().getQualifiers();
       if (Qual.hasVolatile() || Qual.hasObjCLifetime())
         return false;
@@ -1304,6 +1375,7 @@ void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
   bool isTryBody = (Body && isa<CXXTryStmt>(Body));
   if (isTryBody)
     EnterCXXTryStmt(*cast<CXXTryStmt>(Body), true);
+  EmitAsanPrologueOrEpilogue(false);
 
   // Enter the epilogue cleanups.
   RunCleanupsScope DtorEpilogue(*this);

clang/lib/CodeGen/CodeGenFunction.h

@@ -1267,6 +1267,7 @@ public:
   void EmitLambdaBlockInvokeBody();
   void EmitLambdaDelegatingInvokeBody(const CXXMethodDecl *MD);
   void EmitLambdaStaticInvokeFunction(const CXXMethodDecl *MD);
+  void EmitAsanPrologueOrEpilogue(bool Prologue);
 
   /// EmitReturnBlock - Emit the unified return block, trying to avoid its
   /// emission when possible.

clang/test/CodeGen/sanitize-address-field-padding.cpp

@@ -0,0 +1,152 @@
+// Test -fsanitize-address-field-padding
+// RUN: echo 'type:SomeNamespace::BlacklistedByName=field-padding' > %t.type.blacklist
+// RUN: echo 'src:*sanitize-address-field-padding.cpp=field-padding' > %t.file.blacklist
+// RUN: %clang_cc1 -fsanitize=address -fsanitize-address-field-padding=1 -fsanitize-blacklist=%t.type.blacklist -Rsanitize-address -emit-llvm -o - %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=address -fsanitize-address-field-padding=1 -fsanitize-blacklist=%t.file.blacklist -Rsanitize-address -emit-llvm -o - %s 2>&1 | FileCheck %s --check-prefix=FILE_BLACKLIST
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - %s 2>&1 | FileCheck %s --check-prefix=NO_PADDING
+// REQUIRES: shell
+//
+
+// The reasons to ignore a particular class are not set in stone and will change.
+//
+// CHECK: -fsanitize-address-field-padding applied to Positive1
+// CHECK: -fsanitize-address-field-padding ignored for Negative1 because it is trivially copyable
+// CHECK: -fsanitize-address-field-padding ignored for Negative2 because it is trivially copyable
+// CHECK: -fsanitize-address-field-padding ignored for Negative3 because it is a union
+// CHECK: -fsanitize-address-field-padding ignored for Negative4 because it is trivially copyable
+// CHECK: -fsanitize-address-field-padding ignored for Negative5 because it is packed
+// CHECK: -fsanitize-address-field-padding ignored for SomeNamespace::BlacklistedByName because it is blacklisted
+// CHECK:  -fsanitize-address-field-padding ignored for ExternCStruct because it is not C++
+//
+// FILE_BLACKLIST: -fsanitize-address-field-padding ignored for Positive1 because it is in a blacklisted file
+// FILE_BLACKLIST-NOT: __asan_poison_intra_object_redzone
+// NO_PADDING-NOT: __asan_poison_intra_object_redzone
+
+
+class Positive1 {
+ public:
+  Positive1() {}
+  ~Positive1() {}
+  int make_it_non_standard_layout;
+ private:
+  char private1;
+  int private2;
+  short private_array[6];
+  long long private3;
+};
+
+Positive1 positive1;
+// Positive1 with extra paddings
+// CHECK: type { i32, [12 x i8], i8, [15 x i8], i32, [12 x i8], [6 x i16], [12 x i8], i64, [8 x i8] }
+
+class Negative1 {
+ public:
+  Negative1() {}
+  int public1, public2;
+};
+Negative1 negative1;
+// CHECK: type { i32, i32 }
+
+class Negative2 {
+ public:
+  Negative2() {}
+ private:
+  int private1, private2;
+};
+Negative2 negative2;
+// CHECK: type { i32, i32 }
+
+union Negative3 {
+  char m1[8];
+  long long m2;
+};
+
+Negative3 negative3;
+// CHECK: type { i64 }
+
+class Negative4 {
+ public:
+  Negative4() {}
+  // No DTOR
+  int make_it_non_standard_layout;
+ private:
+  char private1;
+  int private2;
+};
+
+Negative4 negative4;
+// CHECK: type { i32, i8, i32 }
+
+class __attribute__((packed)) Negative5 {
+ public:
+  Negative5() {}
+  ~Negative5() {}
+  int make_it_non_standard_layout;
+ private:
+  char private1;
+  int private2;
+};
+
+Negative5 negative5;
+// CHECK: type <{ i32, i8, i32 }>
+
+
+namespace SomeNamespace {
+class BlacklistedByName {
+ public:
+  BlacklistedByName() {}
+  ~BlacklistedByName() {}
+  int make_it_non_standard_layout;
+ private:
+  char private1;
+  int private2;
+};
+}  // SomeNamespace
+
+SomeNamespace::BlacklistedByName blacklisted_by_name;
+
+extern "C" {
+class ExternCStruct {
+ public:
+  ExternCStruct() {}
+  ~ExternCStruct() {}
+  int make_it_non_standard_layout;
+ private:
+  char private1;
+  int private2;
+};
+}  // extern "C"
+
+ExternCStruct extern_C_struct;
+
+// CTOR
+// CHECK-LABEL: define linkonce_odr void @_ZN9Positive1C1Ev
+// CHECK: call void @__asan_poison_intra_object_redzone({{.*}}, i64 12)
+// CHECK: call void @__asan_poison_intra_object_redzone({{.*}}, i64 15)
+// CHECK: call void @__asan_poison_intra_object_redzone({{.*}}, i64 12)
+// CHECK: call void @__asan_poison_intra_object_redzone({{.*}}, i64 12)
+// CHECK: call void @__asan_poison_intra_object_redzone({{.*}}, i64 8)
+// CHECK-NOT: __asan_poison_intra_object_redzone
+// CHECK: ret void
+// DTOR
+// CHECK-LABEL: define linkonce_odr void @_ZN9Positive1D1Ev
+// CHECK: call void @__asan_unpoison_intra_object_redzone({{.*}}, i64 12)
+// CHECK: call void @__asan_unpoison_intra_object_redzone({{.*}}, i64 15)
+// CHECK: call void @__asan_unpoison_intra_object_redzone({{.*}}, i64 12)
+// CHECK: call void @__asan_unpoison_intra_object_redzone({{.*}}, i64 12)
+// CHECK: call void @__asan_unpoison_intra_object_redzone({{.*}}, i64 8)
+// CHECK-NOT: __asan_unpoison_intra_object_redzone
+// CHECK: ret void
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN9Negative1C1Ev
+// CHECK-NOT: call void @__asan_poison_intra_object_redzone
+// CHECK: ret void
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN9Negative2C1Ev
+// CHECK-NOT: call void @__asan_poison_intra_object_redzone
+// CHECK: ret void
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN9Negative4C1Ev
+// CHECK-NOT: call void @__asan_poison_intra_object_redzone
+// CHECK: ret void
+