[PATCH] Insert poisoned paddings between fields in C++ classes.
Kostya Serebryany
kcc at google.com
Tue Oct 14 18:24:03 PDT 2014
================
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,
----------------
rsmith wrote:
> Typo. "Later in CodeGen, extra code will [...]"
done
================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1348
@@ +1347,3 @@
+// extra paddings.
+bool RecordLayoutBuilder::MayInsertExtraPaddingAfterField(const RecordDecl *RD,
+ const FieldDecl *FD) {
----------------
rsmith wrote:
> This does not depend on the value of `FD`; perhaps remove `FD` and just call it once per class?
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
================
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";
+
----------------
rsmith wrote:
> 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?
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
================
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())
----------------
rsmith wrote:
> This 'simple destructor' check doesn't seem right. What properties do you intend to check for here?
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.
================
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";
+
----------------
rsmith wrote:
> Please fix =)
>
> I think reusing the same `-fsanitize-blacklist` flag makes sense.
Alexey will commit a separate patch to move fsanitize=blacklist to langopts.
Clearly, I will wait for that and update this CL
================
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;
+}
----------------
rsmith wrote:
> If you're going to keep this, it should be put behind a remark flag, and should use the normal diagnostics machinery.
done (-Rsanitize-address)
================
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++)
----------------
rsmith wrote:
> This might be easier if you insert padding before fields rather than after them.
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.
================
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]));
}
----------------
rsmith wrote:
> 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.
>> 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()
================
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
----------------
rsmith wrote:
> "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?
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)
================
Comment at: lib/CodeGen/CGClass.cpp:703-704
@@ +702,4 @@
+ : cast<CXXDestructorDecl>(CurGD.getDecl())->getParent();
+ if (ClassDecl->hasAttr<PackedAttr>() || ClassDecl->isStandardLayout() ||
+ ClassDecl->isTriviallyCopyable())
+ return;
----------------
rsmith wrote:
> This doesn't match the set of checks you did in `MayInsertExtraPaddingAfterField`; in particular, do you need to consider unions here?
Agree. I've moved all this logic into RecordDecl::MayInsertExtraPadding and call it twice, once from RecordLayoutBuilder::LayoutFields and once from CodeGenFunction::EmitAsanPrologueOrEpilogue
================
Comment at: lib/CodeGen/CGClass.cpp:708-709
@@ +707,4 @@
+ struct SizeAndOffset {
+ uint64_t size;
+ uint64_t offset;
+ };
----------------
rsmith wrote:
> Field names should be capitalized.
done
================
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;
+
----------------
rsmith wrote:
> Please don't hard-code `8` here; use `ASTContext::toCharUnitsFromBits` instead.
done
================
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};
----------------
rsmith wrote:
> This is `IntPtrTy` (a member of `CodeGenFunction`).
done
http://reviews.llvm.org/D5687
More information about the cfe-commits
mailing list