[ubsan] Use the object pointer's type info for devirtualized calls

[vsk]

Summary

ubsan reports a false positive 'invalid member call' diagnostic on the
following example (PR30478):

struct Base1 {
  virtual int f1() { return 1; }
};

struct Base2 {
  virtual int f1() { return 2; }
};

struct Derived2 final : Base1, Base2 {
  int f1() override { return 3; }
};

int t1() {
  Derived2 d;
  return static_cast<Base2 *>(&d)->f1();
}

Adding the "final" attribute to a most-derived class allows clang to
devirtualize member calls into an instance of that class. We should pass
along the type info of the object pointer to avoid the FP. In this case,
that means passing along the type info for 'Derived2' instead of 'Base2'
when checking the dynamic type of 'static_cast<Base2 *>(&d2)'.

[rjmccall]

lib/CodeGen/CodeGenFunction.h

379 ↗

(On Diff #74164)

Please find some way to do this that doesn't require adding new tracking state like this. It should be quite easy to pass down that a call was devirtualized.

[vsk]

• Remove the set of blacklisted object pointers; pass down a flag which indicates whether or not the member call check should be skipped.

[rjmccall]

Wait, can you talk me through the bug here? Why is final-based devirtualization here different from, say, user-directed devirtualization via a qualified method name?

It sounds to me from your description that you're not sure why this is happening. If this indeed only triggers in the presence of multiple inheritance, it might just be the case that you're doing your object-extents analysis starting from the wrong offset.

[vsk]

Thanks for your feedback so far, and sorry for the delayed response.

In D25448#570014, @rjmccall wrote:

Wait, can you talk me through the bug here?

Derived inherits from Base1 and Base2. We upcast an instance of Derived to Base2, then call a method (f1). Clang figures out that the method has to be Derived::f1. Next, clang passes in a pointer to the vtable pointer for Base1, along with the typeinfo for Base2, into the sanitizer runtime. This confuses the runtime, which reports that the dynamic type of the object is "Base1", and that this does not match the expected type ("Base2").

With the 'final' keyword:

%6 = ptrtoint <2 x i32 (...)**>* %1 to i64
call void @__ubsan_handle_dynamic_type_cache_miss(@1 (TypeInfo for Base2), i64 %6, ...)

Without the 'final' keyword:

%6 = bitcast <2 x i32 (...)**>* %1 to %class.Derived*
%7 = getelementptr inbounds %class.Derived, %class.Derived* %6, i64 0, i32 1
%8 = ptrtoint %class.Base2* %7 to i64, !nosanitize !5
call void @__ubsan_handle_dynamic_type_cache_miss(@1 (TypeInfo for Base2), i64 %8, ...)

Why is final-based devirtualization here different from, say, user-directed devirtualization via a qualified method name?

I'm not sure. I tested this by removing the 'final' specifier from 'Derived' and calling:

obj.Derived::f1()

In this case, clang passes the correct typeinfo (for Derived) in to the runtime.

It sounds to me from your description that you're not sure why this is happening. If this indeed only triggers in the presence of multiple inheritance, it might just be the case that you're doing your object-extents analysis starting from the wrong offset.

It looks like I just haven't done a good job of explaining the issue. The bug really does seem to be that clang isn't passing the right information to the ubsan runtime. However, I'm not sure what the right fix is. Do we disable sanitization in cases where we expect FP's, do we try harder to pass in the right vptr (in this case, the vptr for Base2) into the runtime, or do we try harder to pass in the right typeinfo (in this case, the typeinfo for Derived)?

[rjmccall]

In D25448#571941, @vsk wrote:

Thanks for your feedback so far, and sorry for the delayed response.

In D25448#570014, @rjmccall wrote:

Wait, can you talk me through the bug here?

Derived inherits from Base1 and Base2. We upcast an instance of Derived to Base2, then call a method (f1). Clang figures out that the method has to be Derived::f1. Next, clang passes in a pointer to the vtable pointer for Base1, along with the typeinfo for Base2, into the sanitizer runtime. This confuses the runtime, which reports that the dynamic type of the object is "Base1", and that this does not match the expected type ("Base2").

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.

When the devirtualizer sees what's formally a call to a method on Base2, and it decides that it can instead treat it as a call to a method on Derived, it has to adjust the 'this' pointer to point to a Derived. (I don't know off-hand whether it does this by retroactively adjusting the pointer or whether it just avoids adjusting it in the first place.) When it does this, it should also be changing its notion of what class the pointer points to.

With the 'final' keyword:

%6 = ptrtoint <2 x i32 (...)**>* %1 to i64
call void @__ubsan_handle_dynamic_type_cache_miss(@1 (TypeInfo for Base2), i64 %6, ...)

Without the 'final' keyword:

%6 = bitcast <2 x i32 (...)**>* %1 to %class.Derived*
%7 = getelementptr inbounds %class.Derived, %class.Derived* %6, i64 0, i32 1
%8 = ptrtoint %class.Base2* %7 to i64, !nosanitize !5
call void @__ubsan_handle_dynamic_type_cache_miss(@1 (TypeInfo for Base2), i64 %8, ...)

Why is final-based devirtualization here different from, say, user-directed devirtualization via a qualified method name?

I'm not sure. I tested this by removing the 'final' specifier from 'Derived' and calling:

obj.Derived::f1()

In this case, clang passes the correct typeinfo (for Derived) in to the runtime.

It sounds to me from your description that you're not sure why this is happening. If this indeed only triggers in the presence of multiple inheritance, it might just be the case that you're doing your object-extents analysis starting from the wrong offset.

It looks like I just haven't done a good job of explaining the issue. The bug really does seem to be that clang isn't passing the right information to the ubsan runtime. However, I'm not sure what the right fix is. Do we disable sanitization in cases where we expect FP's, do we try harder to pass in the right vptr (in this case, the vptr for Base2) into the runtime, or do we try harder to pass in the right typeinfo (in this case, the typeinfo for Derived)?

[vsk]

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.

[vsk]

• Remove some default arguments left over from an older revision of this patch.
• Simplify the test.

[rjmccall]

In D25448#572245, @vsk wrote:

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 don't know how the runtime parts of this check are implemented. It is certainly true that an object that is a Base1 might also be a Base2 due to multiple inheritance. But given a pointer that's statically typed to be a Base2, I believe you want to be checking not "am I pointing at an object whose dynamic type derives from Base2" but "am I pointing *at the Base2 subobject* of an object whose dynamic type derives from Base1", which is a very different check and doesn't necessarily require the same runtime complexity (or might need more, depending on what you're doing).

But anyway, the pointer you're passing does not point to the Base2 subobject anymore, which is clearly a frontend bug, not a runtime bug.

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.

I can't imagine what on earth you would do in the runtime that would have a false-negative problem here without basically disabling the check, but let's leave that aside.

... 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.

Yes, this is what you should do. Your runtime logic is, somehow, checking whether the memory contains an object of some type that you know statically. Devirt is potentially changing where your pointer points and thus also changing the expected static type of the memory you're pointing to. Properly responding to that change will not only fix the logic here, it will strengthen your check so that you also detect the bug here:

Base1 p;
Derived *badp = static_cast<Derived*>(&p);
static_cast<Base1*>(badp)->method_that_is_final_in_derived();

[rjmccall]

lib/CodeGen/CGExprCXX.cpp

31 ↗

(On Diff #74943)

Shouldn't MD just be the devirtualized method? That should avoid the need to pass down something different for the devirt case here.

[vsk]

Thanks for the feedback!

Patch update:

• Pass in the right CalleeDecl to EmitCXXMemberOrOperatorCall. This lets us drop the unnecessary 'DevirtualizedClassTy' optional parameter.
• Extend the test case with John's example (devirt for methods marked 'final'). Since this hasn't yet been implemented in clang, make the checks depend on the relevant bug (PR13127).

Tested with check-ubsan and check-clang.

[rjmccall]

Looks great, thanks.

cfe/trunk/lib/CodeGen/CGExprCXX.cpp

RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr(
   else if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(CalleeDecl))
     FInfo = &CGM.getTypes().arrangeCXXStructorDeclaration(
         Ctor, StructorType::Complete);
   else
     FInfo = &CGM.getTypes().arrangeCXXMethodDeclaration(CalleeDecl);
 
   llvm::FunctionType *Ty = CGM.getTypes().GetFunctionType(*FInfo);
 
+  // FIXME: Uses of 'MD' past this point need to be audited. We may need to use
+  // 'CalleeDecl' instead.
+
   // C++ [class.virtual]p12:
   //   Explicit qualification with the scope operator (5.1) suppresses the
   //   virtual call mechanism.
   //
   // We also don't emit a virtual call if the base expression has a record type
   // because then we know what the type is.
   bool UseVirtualCall = CanUseVirtualCall && !DevirtualizedMethod;
   llvm::Value *Callee;
if (UseVirtualCall) {
       else if (!DevirtualizedMethod)
         Callee =
             CGM.getAddrOfCXXStructor(Dtor, StructorType::Complete, FInfo, Ty);
       else {
         const CXXDestructorDecl *DDtor =
           cast<CXXDestructorDecl>(DevirtualizedMethod);
         Callee = CGM.GetAddrOfFunction(GlobalDecl(DDtor, Dtor_Complete), Ty);
       }
-      EmitCXXMemberOrOperatorCall(MD, Callee, ReturnValue, This.getPointer(),
-                                  /*ImplicitParam=*/nullptr, QualType(), CE,
-                                  nullptr);
+      EmitCXXMemberOrOperatorCall(
+          CalleeDecl, Callee, ReturnValue, This.getPointer(),
+          /*ImplicitParam=*/nullptr, QualType(), CE, nullptr);
     }
     return RValue::get(nullptr);
   }
   
   if (const CXXConstructorDecl *Ctor = dyn_cast<CXXConstructorDecl>(MD)) {
     Callee = CGM.GetAddrOfFunction(GlobalDecl(Ctor, Ctor_Complete), Ty);
   } else if (UseVirtualCall) {
     Callee = CGM.getCXXABI().getVirtualFunctionPointer(*this, MD, This, Ty,
if (const CXXConstructorDecl *Ctor = dyn_cast<CXXConstructorDecl>(MD)) {
     }
   }
 
   if (MD->isVirtual()) {
     This = CGM.getCXXABI().adjustThisArgumentForVirtualFunctionCall(
         *this, CalleeDecl, This, UseVirtualCall);
   }
 
-  return EmitCXXMemberOrOperatorCall(MD, Callee, ReturnValue, This.getPointer(),
-                                     /*ImplicitParam=*/nullptr, QualType(), CE,
-                                     RtlArgs);
+  return EmitCXXMemberOrOperatorCall(
+      CalleeDecl, Callee, ReturnValue, This.getPointer(),
+      /*ImplicitParam=*/nullptr, QualType(), CE, RtlArgs);
 }
 
 RValue
 CodeGenFunction::EmitCXXMemberPointerCallExpr(const CXXMemberCallExpr *E,
                                               ReturnValueSlot ReturnValue) {
   const BinaryOperator *BO =
       cast<BinaryOperator>(E->getCallee()->IgnoreParens());
   const Expr *BaseExpr = BO->getLHS();

cfe/trunk/test/CodeGenCXX/ubsan-devirtualized-calls.cpp

+// RUN: %clang_cc1 -std=c++11 -triple %itanium_abi_triple -emit-llvm -fsanitize=vptr %s -o - | FileCheck %s
+
+struct Base1 {
+  virtual void f1() {}
+};
+
+struct Base2 {
+  virtual void f1() {}
+};
+
+struct Derived1 final : Base1 {
+  void f1() override {}
+};
+
+struct Derived2 final : Base1, Base2 {
+  void f1() override {}
+};
+
+// PR13127 documents some missed devirtualization opportunities, including
+// devirt for methods marked 'final'. We can enable the checks marked 'PR13127'
+// if we implement this in the frontend.
+struct Derived3 : Base1 {
+  void f1() override /* nofinal */ {}
+};
+
+struct Derived4 final : Base1 {
+  void f1() override final {}
+};
+
+// CHECK: [[UBSAN_TI_DERIVED1_1:@[0-9]+]] = private unnamed_addr global {{.*}} i8* bitcast {{.*}} @_ZTI8Derived1 to i8*
+// CHECK: [[UBSAN_TI_DERIVED2_1:@[0-9]+]] = private unnamed_addr global {{.*}} i8* bitcast {{.*}} @_ZTI8Derived2 to i8*
+// CHECK: [[UBSAN_TI_DERIVED2_2:@[0-9]+]] = private unnamed_addr global {{.*}} i8* bitcast {{.*}} @_ZTI8Derived2 to i8*
+// PR13127: [[UBSAN_TI_DERIVED3:@[0-9]+]] = private unnamed_addr global {{.*}} i8* bitcast {{.*}} @_ZTI8Derived3 to i8*
+// PR13127: [[UBSAN_TI_BASE1:@[0-9]+]] = private unnamed_addr global {{.*}} i8* bitcast {{.*}} @_ZTI5Base1 to i8*
+// PR13127: [[UBSAN_TI_DERIVED4_1:@[0-9]+]] = private unnamed_addr global {{.*}} i8* bitcast {{.*}} @_ZTI8Derived4 to i8*
+// PR13127: [[UBSAN_TI_DERIVED4_2:@[0-9]+]] = private unnamed_addr global {{.*}} i8* bitcast {{.*}} @_ZTI8Derived4 to i8*
+
+// CHECK-LABEL: define void @_Z2t1v
+void t1() {
+  Derived1 d1;
+  static_cast<Base1 *>(&d1)->f1(); //< Devirt Base1::f1 to Derived1::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_1]] {{.*}}, i64 %[[D1]]
+}
+
+// CHECK-LABEL: define void @_Z2t2v
+void t2() {
+  Derived2 d2;
+  static_cast<Base1 *>(&d2)->f1(); //< Devirt Base1::f1 to Derived2::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 void @_Z2t3v
+void t3() {
+  Derived2 d2;
+  static_cast<Base2 *>(&d2)->f1(); //< Devirt Base2::f1 to Derived2::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]]
+}
+
+// PR13127-LABEL: define void @_Z2t4v
+void t4() {
+  Base1 p;
+  Derived3 *badp = static_cast<Derived3 *>(&p); //< Check that &p isa Derived3.
+  // PR13127: handler.dynamic_type_cache_miss
+  // PR13127: %[[P1:[0-9]+]] = ptrtoint %struct.Derived3* {{%[0-9]+}} to i64, !nosanitize
+  // PR13127-NEXT: call void @{{[_a-z]+}}({{.*}} [[UBSAN_TI_DERIVED3]] {{.*}}, i64 %[[P1]]
+
+  static_cast<Base1 *>(badp)->f1(); //< No devirt, test 'badp isa Base1'.
+  // PR13127: handler.dynamic_type_cache_miss
+  // PR13127: %[[BADP1:[0-9]+]] = ptrtoint %struct.Base1* {{%[0-9]+}} to i64, !nosanitize
+  // PR13127-NEXT: call void @{{[_a-z]+}}({{.*}} [[UBSAN_TI_BASE1]] {{.*}}, i64 %[[BADP1]]
+}
+
+// PR13127-LABEL: define void @_Z2t5v
+void t5() {
+  Base1 p;
+  Derived4 *badp = static_cast<Derived4 *>(&p); //< Check that &p isa Derived4.
+  // PR13127: handler.dynamic_type_cache_miss:
+  // PR13127: %[[P1:[0-9]+]] = ptrtoint %struct.Derived4* {{%[0-9]+}} to i64, !nosanitize
+  // PR13127-NEXT: call void @{{[_a-z]+}}({{.*}} [[UBSAN_TI_DERIVED4_1]] {{.*}}, i64 %[[P1]]
+
+  static_cast<Base1 *>(badp)->f1(); //< Devirt Base1::f1 to Derived4::f1.
+  // PR13127: handler.dynamic_type_cache_miss1:
+  // PR13127: %[[BADP1:[0-9]+]] = ptrtoint %struct.Derived4* {{%[0-9]+}} to i64, !nosanitize
+  // PR13127-NEXT: call void @{{[_a-z]+}}({{.*}} [[UBSAN_TI_DERIVED4_2]] {{.*}}, i64 %[[BADP1]]
+}