vsk retitled this revision from "[ubsan] Disable -fsanitize=vptr checks for
devirtualized calls" to "[ubsan] Use the object pointer's type info for
devirtualized calls".
vsk updated the summary for this revision.
vsk added a subscriber: rsmith.
vsk updated this revision to Diff 74916.
vsk added a comment.
Patch update: Pass along the type info of the derived class to the ubsan
runtime when we devirtualize a method call. This squashes the FP. I tested this
with 'check-ubsan' in addition to adding a lit test.
> A pointer to the vtable pointer for Base1 is a pointer to a Derived. You
> have a multiple inheritance bug, or really a general inheritance bug. It's
> being covered up in the case of single, non-virtual inheritance because
> that's the case in which a pointer to a base-class object is the same as a
> pointer to the derived class object.
I imagine that it would be difficult to extend the runtime to handle this case.
I.e, given a pointer to vtable pointer for Base1 and the type info for Base2,
recognize that we may _actually_ be looking at an instance of Derived, and
therefore claim that the types match. I wonder if that would result in a false
negative in this case:
Base1 b1;
reinterpret_cast<Base2 *>(b1)->method_from_base2_only()
We are currently able to diagnose this.
> ... it should also be changing its notion of what class the pointer points to.
I'm taking this to mean that we should pass along the type information for
'Derived'. This is also what @rsmith suggests.
https://reviews.llvm.org/D25448
Files:
lib/CodeGen/CGExprCXX.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGenCXX/ubsan-devirtualized-calls.cpp
Index: test/CodeGenCXX/ubsan-devirtualized-calls.cpp
===================================================================
--- /dev/null
+++ test/CodeGenCXX/ubsan-devirtualized-calls.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 -std=c++11 -triple %itanium_abi_triple -emit-llvm -fsanitize=vptr %s -o - | FileCheck %s
+
+struct Base1 {
+ virtual int f1() { return 1; }
+};
+
+struct Base2 {
+ virtual int f1() { return 2; }
+};
+
+struct Derived1 final : Base1 {
+ int f1() override { return 3; }
+};
+
+struct Derived2 final : Base1, Base2 {
+ int f1() override { return 3; }
+};
+
+// CHECK: [[UBSAN_TI_DERIVED1:@[0-9]+]] = private unnamed_addr global {{.*}} i8* bitcast ({ i8*, i8*, i8* }* @_ZTI8Derived1 to i8*
+// CHECK: [[UBSAN_TI_DERIVED2_1:@[0-9]+]] = private unnamed_addr global {{.*}} i8* bitcast ({ i8*, i8*, i32, i32, i8*, i64, i8*, i64 }* @_ZTI8Derived2 to i8*
+// CHECK: [[UBSAN_TI_DERIVED2_2:@[0-9]+]] = private unnamed_addr global {{.*}} i8* bitcast ({ i8*, i8*, i32, i32, i8*, i64, i8*, i64 }* @_ZTI8Derived2 to i8*
+// CHECK: [[UBSAN_TI_DERIVED2_3:@[0-9]+]] = private unnamed_addr global {{.*}} i8* bitcast ({ i8*, i8*, i32, i32, i8*, i64, i8*, i64 }* @_ZTI8Derived2 to i8*
+
+// CHECK-LABEL: define i32 @_Z2t1v
+int t1() {
+ Derived1 d1;
+ return static_cast<Base1 *>(&d1)->f1();
+ // CHECK: handler.dynamic_type_cache_miss:
+ // CHECK-NEXT: %[[D1:[0-9]+]] = ptrtoint %struct.Derived1* %d1 to i64, !nosanitize
+ // CHECK-NEXT: call void @{{[_a-z]+}}({{.*}} [[UBSAN_TI_DERIVED1]] {{.*}}, i64 %[[D1]]
+}
+
+// CHECK-LABEL: define i32 @_Z2t2v
+int t2() {
+ Derived2 d2;
+ return static_cast<Base1 *>(&d2)->f1();
+ // CHECK: handler.dynamic_type_cache_miss:
+ // CHECK-NEXT: %[[D2_1:[0-9]+]] = ptrtoint %struct.Derived2* %d2 to i64, !nosanitize
+ // CHECK-NEXT: call void @{{[_a-z]+}}({{.*}} [[UBSAN_TI_DERIVED2_1]] {{.*}}, i64 %[[D2_1]]
+}
+
+// CHECK-LABEL: define i32 @_Z2t3v
+int t3() {
+ Derived2 d2;
+ return static_cast<Base2 *>(&d2)->f1();
+ // CHECK: handler.dynamic_type_cache_miss:
+ // CHECK-NEXT: %[[D2_2:[0-9]+]] = ptrtoint %struct.Derived2* %d2 to i64, !nosanitize
+ // CHECK-NEXT: call void @{{[_a-z]+}}({{.*}} [[UBSAN_TI_DERIVED2_2]] {{.*}}, i64 %[[D2_2]]
+}
+
+// CHECK-LABEL: define i32 @_Z2t4v
+int t4() {
+ Derived2 d2;
+ return d2.Derived2::f1();
+ // CHECK: handler.dynamic_type_cache_miss:
+ // CHECK-NEXT: %[[D2_3:[0-9]+]] = ptrtoint %struct.Derived2* %d2 to i64, !nosanitize
+ // CHECK-NEXT: call void @{{[_a-z]+}}({{.*}} [[UBSAN_TI_DERIVED2_3]] {{.*}}, i64 %[[D2_3]]
+}
Index: lib/CodeGen/CodeGenFunction.h
===================================================================
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2877,12 +2877,11 @@
CXXDtorType Type,
const CXXRecordDecl *RD);
- RValue
- EmitCXXMemberOrOperatorCall(const CXXMethodDecl *MD, llvm::Value *Callee,
- ReturnValueSlot ReturnValue, llvm::Value *This,
- llvm::Value *ImplicitParam,
- QualType ImplicitParamTy, const CallExpr *E,
- CallArgList *RtlArgs);
+ RValue EmitCXXMemberOrOperatorCall(
+ const CXXMethodDecl *MD, llvm::Value *Callee, ReturnValueSlot ReturnValue,
+ llvm::Value *This, llvm::Value *ImplicitParam, QualType ImplicitParamTy,
+ const CallExpr *E, CallArgList *RtlArgs,
+ llvm::Optional<QualType> DevirtualizedClassTy = llvm::None);
RValue EmitCXXDestructorCall(const CXXDestructorDecl *DD, llvm::Value *Callee,
llvm::Value *This, llvm::Value *ImplicitParam,
QualType ImplicitParamTy, const CallExpr *E,
Index: lib/CodeGen/CGExprCXX.cpp
===================================================================
--- lib/CodeGen/CGExprCXX.cpp
+++ lib/CodeGen/CGExprCXX.cpp
@@ -24,11 +24,11 @@
using namespace clang;
using namespace CodeGen;
-static RequiredArgs
-commonEmitCXXMemberOrOperatorCall(CodeGenFunction &CGF, const CXXMethodDecl *MD,
- llvm::Value *This, llvm::Value *ImplicitParam,
- QualType ImplicitParamTy, const CallExpr *CE,
- CallArgList &Args, CallArgList *RtlArgs) {
+static RequiredArgs commonEmitCXXMemberOrOperatorCall(
+ CodeGenFunction &CGF, const CXXMethodDecl *MD, llvm::Value *This,
+ llvm::Value *ImplicitParam, QualType ImplicitParamTy, const CallExpr *CE,
+ CallArgList &Args, CallArgList *RtlArgs,
+ llvm::Optional<QualType> DevirtualizedClassTy) {
assert(CE == nullptr || isa<CXXMemberCallExpr>(CE) ||
isa<CXXOperatorCallExpr>(CE));
assert(MD->isInstance() &&
@@ -41,10 +41,14 @@
SourceLocation CallLoc;
if (CE)
CallLoc = CE->getExprLoc();
+ QualType Ty = DevirtualizedClassTy.hasValue()
+ ? DevirtualizedClassTy.getValue()
+ : C.getRecordType(MD->getParent());
CGF.EmitTypeCheck(isa<CXXConstructorDecl>(MD)
? CodeGenFunction::TCK_ConstructorCall
: CodeGenFunction::TCK_MemberCall,
- CallLoc, This, C.getRecordType(MD->getParent()));
+ CallLoc, This, Ty,
+ /*Alignment=*/CharUnits::Zero(), /*SkipNullCheck=*/false);
// Push the this ptr.
const CXXRecordDecl *RD =
@@ -82,11 +86,13 @@
RValue CodeGenFunction::EmitCXXMemberOrOperatorCall(
const CXXMethodDecl *MD, llvm::Value *Callee, ReturnValueSlot ReturnValue,
llvm::Value *This, llvm::Value *ImplicitParam, QualType ImplicitParamTy,
- const CallExpr *CE, CallArgList *RtlArgs) {
+ const CallExpr *CE, CallArgList *RtlArgs,
+ llvm::Optional<QualType> DevirtualizedClassTy) {
const FunctionProtoType *FPT = MD->getType()->castAs<FunctionProtoType>();
CallArgList Args;
RequiredArgs required = commonEmitCXXMemberOrOperatorCall(
- *this, MD, This, ImplicitParam, ImplicitParamTy, CE, Args, RtlArgs);
+ *this, MD, This, ImplicitParam, ImplicitParamTy, CE, Args, RtlArgs,
+ DevirtualizedClassTy);
return EmitCall(CGM.getTypes().arrangeCXXMethodCall(Args, FPT, required),
Callee, ReturnValue, Args, MD);
}
@@ -97,7 +103,8 @@
StructorType Type) {
CallArgList Args;
commonEmitCXXMemberOrOperatorCall(*this, DD, This, ImplicitParam,
- ImplicitParamTy, CE, Args, nullptr);
+ ImplicitParamTy, CE, Args, nullptr,
+ llvm::None);
return EmitCall(CGM.getTypes().arrangeCXXStructorDeclaration(DD, Type),
Callee, ReturnValueSlot(), Args, DD);
}
@@ -148,6 +155,7 @@
bool CanUseVirtualCall = MD->isVirtual() && !HasQualifier;
const CXXMethodDecl *DevirtualizedMethod = nullptr;
+ llvm::Optional<QualType> DevirtualizedClassTy = llvm::None;
if (CanUseVirtualCall && CanDevirtualizeMemberFunctionCall(Base, MD)) {
const CXXRecordDecl *BestDynamicDecl = Base->getBestDynamicClassType();
DevirtualizedMethod = MD->getCorrespondingMethodInClass(BestDynamicDecl);
@@ -162,11 +170,13 @@
// type of MD and has a prefix.
// For now we just avoid devirtualizing these covariant cases.
DevirtualizedMethod = nullptr;
- else if (getCXXRecord(Inner) == DevirtualizedClass)
+ else if (getCXXRecord(Inner) == DevirtualizedClass) {
// If the class of the Inner expression is where the dynamic method
// is defined, build the this pointer from it.
Base = Inner;
- else if (getCXXRecord(Base) != DevirtualizedClass) {
+ DevirtualizedClassTy = QualType(BestDynamicDecl->getTypeForDecl(),
+ Base->getType().getLocalFastQualifiers());
+ } else if (getCXXRecord(Base) != DevirtualizedClass) {
// If the method is defined in a class that is not the best dynamic
// one or the one of the full expression, we would have to build
// a derived-to-base cast to compute the correct this pointer, but
@@ -270,7 +280,7 @@
}
EmitCXXMemberOrOperatorCall(MD, Callee, ReturnValue, This.getPointer(),
/*ImplicitParam=*/nullptr, QualType(), CE,
- nullptr);
+ nullptr, DevirtualizedClassTy);
}
return RValue::get(nullptr);
}
@@ -304,7 +314,7 @@
return EmitCXXMemberOrOperatorCall(MD, Callee, ReturnValue, This.getPointer(),
/*ImplicitParam=*/nullptr, QualType(), CE,
- RtlArgs);
+ RtlArgs, DevirtualizedClassTy);
}
RValue
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
