[PATCH] Insert poisoned paddings between fields in C++ classes.

[Richard Smith]

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1346-1347
@@ +1345,4 @@
+// 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,
----------------
Typo. "Later in CodeGen, extra code will [...]"

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1348
@@ +1347,3 @@
+// extra paddings.
+bool RecordLayoutBuilder::MayInsertExtraPaddingAfterField(const RecordDecl *RD,
+                                                          const FieldDecl *FD) {
----------------
This does not depend on the value of `FD`; perhaps remove `FD` and just call it once per class?

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1362-1367
@@ +1361,8 @@
+    ReasonToReject = "union";
+  else if (CXXRD->isTriviallyCopyable())
+    ReasonToReject = "trivially copyable";
+  else if (CXXRD->hasSimpleDestructor())
+    ReasonToReject = "has simple DTOR";
+  else if (CXXRD->isStandardLayout())
+    ReasonToReject = "standard layout";
+
----------------
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?

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1364-1365
@@ +1363,4 @@
+    ReasonToReject = "trivially copyable";
+  else if (CXXRD->hasSimpleDestructor())
+    ReasonToReject = "has simple DTOR";
+  else if (CXXRD->isStandardLayout())
----------------
This 'simple destructor' check doesn't seem right. What properties do you intend to check for here?

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1369-1379
@@ +1368,13 @@
+
+  // 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";
+
----------------
Please fix =)

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

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1381-1387
@@ +1380,9 @@
+
+  // 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;
+}
----------------
If you're going to keep this, it should be put behind a remark flag, and should use the normal diagnostics machinery.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1394-1395
@@ +1393,4 @@
+  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++)
----------------
This might be easier if you insert padding before fields rather than after them.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1397-1398
@@ -1346,1 +1396,4 @@
+  for (size_t i = 0, e = Fields.size(); i < e; i++)
+    LayoutField(Fields[i],
+                i + 1 != e && MayInsertExtraPaddingAfterField(D, Fields[i]));
 }
----------------
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.

================
Comment at: lib/CodeGen/CGClass.cpp:695
@@ -694,1 +694,3 @@
 
+// Emit code in CTOR (Prologue==true) or DTOR(Prologue==false)
+// to poison the extra field paddings inserted under
----------------
"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?

================
Comment at: lib/CodeGen/CGClass.cpp:703-704
@@ +702,4 @@
+               : cast<CXXDestructorDecl>(CurGD.getDecl())->getParent();
+  if (ClassDecl->hasAttr<PackedAttr>() || ClassDecl->isStandardLayout() ||
+      ClassDecl->isTriviallyCopyable())
+    return;
----------------
This doesn't match the set of checks you did in `MayInsertExtraPaddingAfterField`; in particular, do you need to consider unions here?

================
Comment at: lib/CodeGen/CGClass.cpp:708-709
@@ +707,4 @@
+  struct SizeAndOffset {
+    uint64_t size;
+    uint64_t offset;
+  };
----------------
Field names should be capitalized.

================
Comment at: lib/CodeGen/CGClass.cpp:719
@@ +718,3 @@
+  for (unsigned i = 0, e = Info.getFieldCount(); i != e; ++i)
+    SSV[i].offset = Info.getFieldOffset(i) / 8;
+
----------------
Please don't hard-code `8` here; use `ASTContext::toCharUnitsFromBits` instead.

================
Comment at: lib/CodeGen/CGClass.cpp:737
@@ +736,3 @@
+  // LLVM AddressSanitizer pass may decide to inline them later.
+  llvm::Type *PtrIntTy = Builder.getIntNTy(PtrSize);
+  llvm::Type *Args[2] = {PtrIntTy, PtrIntTy};
----------------
This is `IntPtrTy` (a member of `CodeGenFunction`).

http://reviews.llvm.org/D5687