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

[kcc]

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

[rsmith]

lib/AST/RecordLayoutBuilder.cpp

1346–1347

Typo. "Later in CodeGen, extra code will [...]"

1348

This does not depend on the value of FD; perhaps remove FD and just call it once per class?

1362–1367

What's the reason behind picking these rules? It seems like we could be more aggressive here and still follow the C++ standard. Is this for C compatibility, or people who roll their own memcpy, or something else? I guess the ASan-instrumented memcpy can be taught that it's OK to copy intra-object redzone?

1364–1365

This 'simple destructor' check doesn't seem right. What properties do you intend to check for here?

1369–1379

Please fix =)

I think reusing the same -fsanitize-blacklist flag makes sense.

1381–1387

If you're going to keep this, it should be put behind a remark flag, and should use the normal diagnostics machinery.

1394–1395

This might be easier if you insert padding before fields rather than after them.

1397–1398

Maybe factor out adding the padding from adding the field? I don't think they need to be done together, do they?

Also, you shouldn't add padding at the end of the class if it ends in a flexible array member.

lib/CodeGen/CGClass.cpp

695

"ctor" and "dtor" in lowercase please.

Stripping the poison in the dtor doesn't match C++ semantics: you aren't required to run the dtor to reuse the storage as something else, but conversely you can't reuse the storage as something else unless you use placement-new to construct a new object.

As a compromise to be nicer to existing code, maybe we should strip poison both in the dtor and in a new-expression?

703–704

This doesn't match the set of checks you did in MayInsertExtraPaddingAfterField; in particular, do you need to consider unions here?

708–709

Field names should be capitalized.

719

Please don't hard-code 8 here; use ASTContext::toCharUnitsFromBits instead.

737

This is IntPtrTy (a member of CodeGenFunction).

[majnemer]

lib/AST/RecordLayoutBuilder.cpp

1374–1375

This doesn't look windows friendly.

[kcc]

Addressed most of the comments in the review, added tests,
moved the check for validity of the instrumentation to RecordDecl::MayInsertExtraPadding,
added a remark flag -Rsanitize-address.

PTAL

This CL still has one FIXME pending Alexey's changes for -fsanitize-blacklist flag.

lib/AST/RecordLayoutBuilder.cpp

1346–1347

done

1348

Agree. Moved to RecordDecl::MayInsertExtraPadding
In future I may need a separate function to check a particular field (e.g. if we want paddings only after arrays).
But not in this patch

1362–1367

It's quite hard to teach asan memcpy to avoid intra object redzones w/o extra false negatives and performance penalty.
But I do want to relax the requirement of non-standard layout, just not in this CL

1364–1365

I wanted to limit to classes that have a user defined DTOR, at least in one of the members, subclasses, etc.
As I say in the comment above, we'll try to relax some of these requirements.
This and other limitations are something that I was able to debug so far, I'll be removing/relaxing these requirements one by one later.

1369–1379

Alexey will commit a separate patch to move fsanitize=blacklist to langopts.
Clearly, I will wait for that and update this CL

1381–1387

done (-Rsanitize-address)

1394–1395

I've reverted the loop to its previous state because I actually decided to instrument the last field as well.
This handles cases where we have an array of objects and an overflow from the last field of one objects touches the other object.

1397–1398

Maybe factor out adding the padding from adding the field? I don't think they need to be done together, do they?

Err. Not sure here. It looks like it fits here naturally because we change the FieldSize which then affect the computation if the type's size, etc

Also, you shouldn't add padding at the end of the class if it ends in a flexible array member.

Added the check for !FieldSize.isZero()

lib/CodeGen/CGClass.cpp

695

We may be able to relax the requirement of unpoisoning in dtor, but not in this CL.
I've seen at least few places (on in LLVM itself) where we should not instrument a class w/o dtor.

striping poisin in new-expression is not a solution as it may lead to false negatives (e.g. when a new-expression is called on an illegal memory)

703–704

Agree. I've moved all this logic into RecordDecl::MayInsertExtraPadding and call it twice, once from RecordLayoutBuilder::LayoutFields and once from CodeGenFunction::EmitAsanPrologueOrEpilogue

708–709

done

719

done

737

done

[samsonov]

lib/AST/RecordLayoutBuilder.cpp

1369–1379

r219842 moves `-fsanitize-blacklist` is in LangOpts. You've probably thought about it, but I will warn you anyway: recompiling the library with the same compiler and compile flags, but different blacklist file will make it ABI-incompatible. So, we should be really careful in using it.

[kcc]

Use proper blacklist machinery.
PTAL

[samsonov]

lib/AST/Decl.cpp

3642 ↗

(On Diff #14971)

This looks weird: II->getName() is neither mangled nor qualified (what would it return for the record declared inside the namespaces)? Consider testing smth. like printQualifiedName().

[rsmith]

include/clang/AST/Decl.h

3266 ↗

(On Diff #14907)

In new code, please use \brief instead of repeating the function name.

3269 ↗

(On Diff #14907)

Please use a leading lowercase letter for this function name for consistency with the rest of the class.

include/clang/Basic/DiagnosticFrontendKinds.td

49–51 ↗

(On Diff #14907)

Remarks are end-user-visible, so should be a bit more friendly than this. Also, this diagnostic should use "%select{list|of|strings}1" rather than putting user-visible strings in the code in Decl.cpp. Something like:

"-fsanitize-address-field-padding ignored for %0 because it %select{is packed|is a union|is trivially copyable|has a trivial destructor|is standard layout|is in a blacklisted file|is blacklisted}1"

... would seem reasonable. (I don't think it's worth remarking once per class if the reason we're not sanitizing is because the current language is C; in that case, we should warn once for the entire compilation.)

lib/AST/Decl.cpp

3633 ↗

(On Diff #14907)

Please use nullptr rather than 0.

3642 ↗

(On Diff #14907)

hasSimpleDestructor still does not seem right; it's a notion internal to how we manage implicit declaration and definition for destructors and shouldn't be used for anything else. Does hasTrivialDestructor work for you?

3648 ↗

(On Diff #14907)

Blacklisting by identifier doesn't seem ideal; there should probably be some way of specifying a namespace in the blacklist file.

[kcc]

Addressed all comments except for one

include/clang/AST/Decl.h

3266 ↗

(On Diff #14907)

done

3269 ↗

(On Diff #14907)

done

include/clang/Basic/DiagnosticFrontendKinds.td

49–51 ↗

(On Diff #14907)

Done except for "we should warn once for the entire compilation for C"
What is the kosher way to achieve that in clang?
(I assume that a function-scope static boolean is not kosher)

lib/AST/Decl.cpp

3633 ↗

(On Diff #14907)

Var changed to int.

3642 ↗

(On Diff #14907)

Yes, this seems to work,

3648 ↗

(On Diff #14907)

done (using getQualifiedNameAsString())

[kcc]

Addressed all comments (but one), extended the test to check the blacklist functionality and the remarks

PTAL

[rsmith]

LGTM

include/clang/Basic/DiagnosticFrontendKinds.td

50–56 ↗

(On Diff #15031)

I don't think these should be BackendInfo, because they're reflecting frontend semantics changes. They should probably be just ShowInSystemHeader.

lib/AST/Decl.cpp

3629–3630 ↗

(On Diff #15031)

It would seem prudent to also test whether the type is in an extern "C" linkage specification in C++ here:

if (!CXXRD || CXXRD->isExternCContext())

[kcc]

Addressed last two Richard's comments.
Added a test for an extern "C" class.

lib/AST/RecordLayoutBuilder.cpp

 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/Expr.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/SemaDiagnostic.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/MathExtras.h"
+#include "llvm/Support/SpecialCaseList.h"
 
 using namespace clang;
 
 namespace {
 
 /// BaseSubobjectInfo - Represents a single base subobject in a complete class.
 /// For a class hierarchy like
 ///
void resetWithTargetAlignment(CharUnits TargetAlignment) {
     new (this) RecordLayoutBuilder(Context, EmptySubobjects);
     Alignment = UnpackedAlignment = TargetAlignment;
   }
 
   void Layout(const RecordDecl *D);
   void Layout(const CXXRecordDecl *D);
   void Layout(const ObjCInterfaceDecl *D);
 
+  bool MayInsertExtraPaddingAfterField(const RecordDecl *RD,
+                                       const FieldDecl *FD);
   void LayoutFields(const RecordDecl *D);
-  void LayoutField(const FieldDecl *D);
+  void LayoutField(const FieldDecl *D, bool InsertPoisonedRedzone);
   void LayoutWideBitField(uint64_t FieldSize, uint64_t TypeSize,
                           bool FieldPacked, const FieldDecl *D);
   void LayoutBitField(const FieldDecl *D);
 
   TargetCXXABI getCXXABI() const {
     return Context.getTargetInfo().getCXXABI();
   }
 
if (ObjCInterfaceDecl *SD = D->getSuperClass()) {
     setSize(SL.getDataSize());
     setDataSize(getSize());
   }
 
   InitializeLayout(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.
   FinishLayout(D);
 }
 
+// Returns true if we want to insert an extra padding in RD after FD.
+// These padding are added to help AddressSanitizer detect intra-object-overflow
+// bugs. Later in CogeGen an extra code will be emitted to poison these
+// extra paddings.
+bool RecordLayoutBuilder::MayInsertExtraPaddingAfterField(const RecordDecl *RD,
+                                                          const FieldDecl *FD) {
+  if (!Context.getLangOpts().Sanitize.Address ||
+      !Context.getLangOpts().Sanitize.AddressFieldPadding)
+    return false;
+  const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD);
+  // We may be able to relax some of these requirements.
+  const char *ReasonToReject = 0;
+  if (!CXXRD)
+    ReasonToReject = "not C++";
+  else if (CXXRD->hasAttr<PackedAttr>())
+    ReasonToReject = "packed";
+  else if (CXXRD->isUnion())
+    ReasonToReject = "union";
+  else if (CXXRD->isTriviallyCopyable())
+    ReasonToReject = "trivially copyable";
+  else if (CXXRD->hasSimpleDestructor())
+    ReasonToReject = "has simple DTOR";
+  else if (CXXRD->isStandardLayout())
+    ReasonToReject = "standard layout";
+
+  // FIXME (before commit): where should this SpecialCaseList reside?
+  // In RecordLayoutBuilder? In global scope?
+  // Also, can we reuse the existing -fsanitize-blacklist flag to pass
+  // the file path (i.e. the same blacklist file will be used in AST and
+  // CogeGen), or we should have another flag?
+  auto SCL = llvm::SpecialCaseList::createOrDie(
+      "/tmp/blacklist.txt");
+  StringRef Filename =
+      Context.getSourceManager().getFilename(RD->getLocation());
+  if (SCL->inSection("src", Filename))
+    ReasonToReject = "blacklisted by src";
+
+  // FIXME (before commit): what flag to use for debug output?
+  if (0) {
+    const char *ToPrint = ReasonToReject ? ReasonToReject : "ACCEPTED";
+    llvm::errs() << "ASAN_FIELD_PADDING: " << Filename << " " << *RD
+                 << "::" << *FD << " : " << ToPrint << "\n";
+  }
+  return ReasonToReject == 0;
+}
+
 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.
-  for (const auto *Field : D->fields())
-    LayoutField(Field);
+  SmallVector<FieldDecl *, 16> Fields(D->field_begin(), D->field_end());
+  // FIXME (before commit): is there a way to know that the field is last
+  // w/o this temporary vector?
+  for (size_t i = 0, e = Fields.size(); i < e; i++)
+    LayoutField(Fields[i],
+                i + 1 != e && MayInsertExtraPaddingAfterField(D, Fields[i]));
 }
 
 void RecordLayoutBuilder::LayoutWideBitField(uint64_t FieldSize,
                                              uint64_t TypeSize,
                                              bool FieldPacked,
                                              const FieldDecl *D) {
   assert(Context.getLangOpts().CPlusPlus &&
          "Can only have wide bit-fields in C++!");
void RecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
   // Update the size.
   setSize(std::max(getSizeInBits(), getDataSizeInBits()));
 
   // Remember max struct/class alignment.
   UpdateAlignment(Context.toCharUnitsFromBits(FieldAlign), 
                   Context.toCharUnitsFromBits(UnpackedFieldAlign));
 }
 
-void RecordLayoutBuilder::LayoutField(const FieldDecl *D) {  
+void RecordLayoutBuilder::LayoutField(const FieldDecl *D,
+                                      bool InsertPoisonedRedzone) {
   if (D->isBitField()) {
     LayoutBitField(D);
     return;
   }
 
   uint64_t UnpaddedFieldOffset = getDataSizeInBits() - UnfilledBitsInLastUnit;
 
   // Reset the unfilled bits.
void RecordLayoutBuilder::LayoutField(const FieldDecl *D,
   // Place this field at the current location.
   FieldOffsets.push_back(Context.toBits(FieldOffset));
 
   if (!ExternalLayout)
     CheckFieldPadding(Context.toBits(FieldOffset), UnpaddedFieldOffset, 
                       Context.toBits(UnpackedFieldOffset),
                       Context.toBits(UnpackedFieldAlign), FieldPacked, D);
 
+  if (InsertPoisonedRedzone) {
+    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)
     setDataSize(std::max(getDataSizeInBits(), FieldSizeInBits));
   else
     setDataSize(FieldOffset + FieldSize);
 
   // Update the size.

lib/CodeGen/CGClass.cpp

static bool IsConstructorDelegationValid(const CXXConstructorDecl *Ctor) {
 
   // FIXME: Decide if we can do a delegation of a delegating constructor.
   if (Ctor->isDelegatingConstructor())
     return false;
 
   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) {
+  if (!getLangOpts().Sanitize.AddressFieldPadding) return;
+  const CXXRecordDecl *ClassDecl =
+      Prologue ? cast<CXXConstructorDecl>(CurGD.getDecl())->getParent()
+               : cast<CXXDestructorDecl>(CurGD.getDecl())->getParent();
+  if (ClassDecl->hasAttr<PackedAttr>() || ClassDecl->isStandardLayout() ||
+      ClassDecl->isTriviallyCopyable())
+    return;
+
+  struct SizeAndOffset {
+    uint64_t size;
+    uint64_t offset;
+  };
+
+  unsigned PtrSize = CGM.getDataLayout().getPointerSizeInBits();
+  const ASTRecordLayout &Info = getContext().getASTRecordLayout(ClassDecl);
+
+  // Populate sizes and offsets of fields.
+  // FIXME: can we get those from a single source?
+  SmallVector<SizeAndOffset, 16> SSV(Info.getFieldCount());
+  for (unsigned i = 0, e = Info.getFieldCount(); i != e; ++i)
+    SSV[i].offset = Info.getFieldOffset(i) / 8;
+
+
+  size_t n_fields = 0;
+  for (const auto *Field : ClassDecl->fields()) {
+    const FieldDecl *D = Field;
+    std::pair<CharUnits, CharUnits> FieldInfo =
+      getContext().getTypeInfoInChars(D->getType());
+    CharUnits FieldSize = FieldInfo.first;
+    assert(n_fields < SSV.size());
+    SSV[n_fields].size = D->isBitField() ? 0 : FieldSize.getQuantity();
+    n_fields++;
+  }
+  assert(n_fields == 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 *PtrIntTy = Builder.getIntNTy(PtrSize);
+  llvm::Type *Args[2] = {PtrIntTy, PtrIntTy};
+  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, PtrIntTy);
+
+  // For each field (but the last) check if it has sufficient padding,
+  // if so (un)poison it with a call.
+  for (size_t i = 0; i < SSV.size() - 1; i++) {
+    uint64_t PoisonSize = SSV[i+1].offset - SSV[i].offset - SSV[i].size;
+    uint64_t EndOffset = SSV[i].offset + SSV[i].size;
+    if (PoisonSize < 8 || !SSV[i].size || (SSV[i+1].offset % 8) != 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();
 
   assert((CGM.getTarget().getCXXABI().hasConstructorVariants() ||
           CtorType == Ctor_Complete) &&
          "can only generate complete ctor for this ABI");
 
   // Before we go any further, try the complete->base constructor
namespace {
   public:
     FieldMemcpyizer(CodeGenFunction &CGF, const CXXRecordDecl *ClassDecl,
                     const VarDecl *SrcRec)
       : CGF(CGF), ClassDecl(ClassDecl), SrcRec(SrcRec), 
         RecLayout(CGF.getContext().getASTRecordLayout(ClassDecl)),
         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.AddressFieldPadding)
+        return false;
       Qualifiers Qual = F->getType().getQualifiers();
       if (Qual.hasVolatile() || Qual.hasObjCLifetime())
         return false;
       return true;
     }
 
     void addMemcpyableField(FieldDecl *F) {
       if (!FirstField)
void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
 
   Stmt *Body = Dtor->getBody();
 
   // If the body is a function-try-block, enter the try before
   // anything else.
   bool isTryBody = (Body && isa<CXXTryStmt>(Body));
   if (isTryBody)
     EnterCXXTryStmt(*cast<CXXTryStmt>(Body), true);
+  EmitAsanPrologueOrEpilogue(false);
 
   // Enter the epilogue cleanups.
   RunCleanupsScope DtorEpilogue(*this);
   
   // If this is the complete variant, just invoke the base variant;
   // the epilogue will destruct the virtual bases.  But we can't do
   // this optimization if the body is a function-try-block, because
   // we'd introduce *two* handler blocks.  In the Microsoft ABI, we 

lib/CodeGen/CodeGenFunction.h

public:
   void EmitBlockWithFallThrough(llvm::BasicBlock *BB, RegionCounter &Cnt);
 
   void EmitForwardingCallToLambda(const CXXMethodDecl *LambdaCallOperator,
                                   CallArgList &CallArgs);
   void EmitLambdaToBlockPointerBody(FunctionArgList &Args);
   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.
   void EmitReturnBlock();
 
   /// FinishFunction - Complete IR generation of the current function. It is
   /// legal to call this function even if there is no current insertion point.
   void FinishFunction(SourceLocation EndLoc=SourceLocation());