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.
http://reviews.llvm.org/D5687
Files:
include/clang/AST/Decl.h
include/clang/Basic/DiagnosticFrontendKinds.td
include/clang/Basic/DiagnosticGroups.td
lib/AST/Decl.cpp
lib/AST/RecordLayoutBuilder.cpp
lib/CodeGen/CGClass.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/sanitize-address-field-padding.cpp
Index: include/clang/AST/Decl.h
===================================================================
--- include/clang/AST/Decl.h
+++ include/clang/AST/Decl.h
@@ -3263,6 +3263,11 @@
/// commandline option.
bool isMsStruct(const ASTContext &C) const;
+ /// MayInsertExtraPadding -- 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;
Index: include/clang/Basic/DiagnosticFrontendKinds.td
===================================================================
--- include/clang/Basic/DiagnosticFrontendKinds.td
+++ include/clang/Basic/DiagnosticFrontendKinds.td
@@ -46,6 +46,10 @@
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 : Remark<
+ "MayInsertExtraPadding %0: %1">, BackendInfo,
+ 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">,
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -727,6 +727,9 @@
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">;
Index: lib/AST/Decl.cpp
===================================================================
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -29,7 +29,9 @@
#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 "llvm/Support/SpecialCaseList.h"
#include <algorithm>
using namespace clang;
@@ -3615,6 +3617,45 @@
/*FieldsAlreadyLoaded=*/false);
}
+bool RecordDecl::MayInsertExtraPadding(bool EmitRemark) const {
+ ASTContext &Context = getASTContext();
+ if (!Context.getLangOpts().Sanitize.Address ||
+ !Context.getLangOpts().Sanitize.SanitizeAddressFieldPadding)
+ return false;
+ // FIXME: (before commit) use the blacklist passed by fsanitize=blacklist.
+ // Pending a CL by Alexey.
+ auto SCL = llvm::SpecialCaseList::createOrDie(
+ "/tmp/blacklist.txt");
+ const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(this);
+ StringRef Filename = Context.getSourceManager().getFilename(getLocation());
+ IdentifierInfo *II = getIdentifier();
+ // 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";
+ else if (SCL->inSection("field-padding-src", Filename))
+ ReasonToReject = "blacklisted by src";
+ else if (II && SCL->inSection("field-padding-type", II->getName()))
+ ReasonToReject = "blacklisted by type";
+
+ if (EmitRemark)
+ Context.getDiagnostics().Report(
+ getLocation(), diag::remark_sanitize_address_insert_extra_padding)
+ << (II ? II->getName() : "<unnamed>")
+ << (ReasonToReject ? ReasonToReject : "ACCEPTED");
+ return ReasonToReject == 0;
+}
+
//===----------------------------------------------------------------------===//
// BlockDecl Implementation
//===----------------------------------------------------------------------===//
Index: lib/AST/RecordLayoutBuilder.cpp
===================================================================
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -652,7 +652,7 @@
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 @@
// 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::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 @@
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 @@
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)
Index: lib/CodeGen/CGClass.cpp
===================================================================
--- lib/CodeGen/CGClass.cpp
+++ lib/CodeGen/CGClass.cpp
@@ -703,8 +703,76 @@
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();
@@ -793,7 +861,10 @@
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;
@@ -1305,6 +1376,7 @@
bool isTryBody = (Body && isa<CXXTryStmt>(Body));
if (isTryBody)
EnterCXXTryStmt(*cast<CXXTryStmt>(Body), true);
+ EmitAsanPrologueOrEpilogue(false);
// Enter the epilogue cleanups.
RunCleanupsScope DtorEpilogue(*this);
Index: lib/CodeGen/CodeGenFunction.h
===================================================================
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -1267,6 +1267,7 @@
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.
Index: test/CodeGen/sanitize-address-field-padding.cpp
===================================================================
--- /dev/null
+++ test/CodeGen/sanitize-address-field-padding.cpp
@@ -0,0 +1,104 @@
+// Test -fsanitize-address-field-padding
+// RUN: %clang_cc1 -fsanitize=address -fsanitize-address-field-padding=1 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - %s | FileCheck %s --check-prefix=NO_PADDING
+
+
+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 }>
+
+// 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
+
+// NO_PADDING-NOT: __asan_poison_intra_object_redzone
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
