Insert a type check before reading vtable.

[krasin]

Summary

this is to prevent a situation when a pointer is invalid or null,
but we get to reading from vtable before we can check that
(possibly causing a segfault without a good diagnostics).

[krasin]

Restore the type check before calling a destructor.

[pcc]

Test case?

lib/CodeGen/CGExprCXX.cpp

93 ↗

(On Diff #77660)

Is it correct to emit a type check at this point? Looking at [0] it looks like this function is only called from the Microsoft C++ ABI after we have already resolved the virtual function pointer.

[0] http://llvm-cs.pcc.me.uk/tools/clang/lib/CodeGen/CGExprCXX.cpp/rEmitCXXDestructorCall

[krasin]

All UBSan test cases happen to live in compiler-rt. I have added a couple of test cases in https://reviews.llvm.org/D26560.

Also, I am not against adding Clang counterparts for them which would test IL instead of the output from running. But it probably deserves a separate CL, as it has nothing to do with fixing this particular bug.

[krasin]

Small correction: all UBSan type checks tests live in compiler-rt. There is a test for UBsan + devirtualization inside tools/clang. My point still stands.

[pcc]

We normally update the IR tests whenever we change IRgen. If we're missing test coverage, we should add it before landing this.

[krasin]

Sounds reasonable. Will do on Monday.

[krasin]

Add a regression test.

[pcc]

lib/CodeGen/CGExprCXX.cpp

93 ↗

(On Diff #77660)

What about this comment?

[krasin]

lib/CodeGen/CGExprCXX.cpp

93 ↗

(On Diff #77660)

Sorry, I have missed the comment. I have added this check here to preserve the existing behavior.

From what you describe, there could be a very similar issue related to the Microsoft C++ ABI to the one that I fix here. I am okay to file a bug or add a note about this, your choice.

[pcc]

lib/CodeGen/CGExprCXX.cpp

93 ↗

(On Diff #77660)

Have you looked at how hard it would be to fix this?

[krasin]

Do better job with destructors.

[krasin]

lib/CodeGen/CGExprCXX.cpp

93 ↗

(On Diff #77660)

Done.

Note: since I don't have an ability to test on Windows, this CL is now high-risk. If it breaks any Windows-specific tests, when submitted, I will revert the CL and undo the MSABI change. After all, the previous state was to keep things working as they were before.

[krasin]

Friendly ping.

[pcc]

lib/CodeGen/CGExprCXX.cpp

1928–1933 ↗

(On Diff #77941)

This could all be moved into EmitObjectDelete.

lib/CodeGen/MicrosoftCXXABI.cpp

1820–1833 ↗

(On Diff #77941)

If you undo this part do the tests still pass?

[krasin]

Address comments.

[krasin]

lib/CodeGen/MicrosoftCXXABI.cpp

1820–1833 ↗

(On Diff #77941)

Thank you for the catch. Yes, the other check covers this case too. Reverted this file.

[krasin]

Sync to Clang ToT.

[pcc]

lib/CodeGen/CGExprCXX.cpp

1769 ↗

(On Diff #78240)

DE will always be non-null at this point.

test/CodeGenCXX/ubsan-vtable-checks.cpp

23 ↗

(On Diff #78240)

Does this test pass in a -Asserts build?

[krasin]

Fix the test under -Asserts build.

[krasin]

test/CodeGenCXX/ubsan-vtable-checks.cpp

23 ↗

(On Diff #78240)

Fixed.

[krasin]

Address minor comment.

[krasin]

inline

[pcc]

LGTM

lib/CodeGen/CGExprCXX.cpp

1768 ↗

(On Diff #78279)

You could cite C++11 [expr.delete]p3 here:

"if the static type of the object to be deleted is different from its
dynamic type, the static type shall be a base class of the dynamic type of the object to be deleted and the
static type shall have a virtual destructor or the behavior is undefined."

cfe/trunk/lib/CodeGen/CGExprCXX.cpp

commonEmitCXXMemberOrOperatorCall(CodeGenFunction &CGF, const CXXMethodDecl *MD,
                                   QualType ImplicitParamTy, const CallExpr *CE,
                                   CallArgList &Args, CallArgList *RtlArgs) {
   assert(CE == nullptr || isa<CXXMemberCallExpr>(CE) ||
          isa<CXXOperatorCallExpr>(CE));
   assert(MD->isInstance() &&
          "Trying to emit a member or operator call expr on a static method!");
   ASTContext &C = CGF.getContext();
 
-  // C++11 [class.mfct.non-static]p2:
-  //   If a non-static member function of a class X is called for an object that
-  //   is not of type X, or of a type derived from X, the behavior is undefined.
-  SourceLocation CallLoc;
-  if (CE)
-    CallLoc = CE->getExprLoc();
-  CGF.EmitTypeCheck(isa<CXXConstructorDecl>(MD)
-                        ? CodeGenFunction::TCK_ConstructorCall
-                        : CodeGenFunction::TCK_MemberCall,
-                    CallLoc, This, C.getRecordType(MD->getParent()));
-
   // Push the this ptr.
   const CXXRecordDecl *RD =
       CGF.CGM.getCXXABI().getThisArgumentTypeForMethod(MD);
   Args.add(RValue::get(This),
            RD ? C.getPointerType(C.getTypeDeclType(RD)) : C.VoidPtrTy);
 
   // If there is an implicit parameter (e.g. VTT), emit it.
   if (ImplicitParam) {
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);
 
+  // C++11 [class.mfct.non-static]p2:
+  //   If a non-static member function of a class X is called for an object that
+  //   is not of type X, or of a type derived from X, the behavior is undefined.
+  SourceLocation CallLoc;
+  ASTContext &C = getContext();
+  if (CE)
+    CallLoc = CE->getExprLoc();
+
+  EmitTypeCheck(isa<CXXConstructorDecl>(CalleeDecl)
+                ? CodeGenFunction::TCK_ConstructorCall
+                : CodeGenFunction::TCK_MemberCall,
+                CallLoc, This.getPointer(), C.getRecordType(CalleeDecl->getParent()));
+
   // 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
EHStack.pushCleanup<CallObjectDelete>(NormalAndEHCleanup, CompletePtr,
                                         OperatorDelete, ElementType);
 }
 
 /// Emit the code for deleting a single object.
 static void EmitObjectDelete(CodeGenFunction &CGF,
                              const CXXDeleteExpr *DE,
                              Address Ptr,
                              QualType ElementType) {
+  // C++11 [expr.delete]p3:
+  //   If the static type of the object to be deleted is different from its
+  //   dynamic type, the static type shall be a base class of the dynamic type
+  //   of the object to be deleted and the static type shall have a virtual
+  //   destructor or the behavior is undefined.
+  CGF.EmitTypeCheck(CodeGenFunction::TCK_MemberCall,
+                    DE->getExprLoc(), Ptr.getPointer(),
+                    ElementType);
+
   // Find the destructor for the type, if applicable.  If the
   // destructor is virtual, we'll just emit the vcall and return.
   const CXXDestructorDecl *Dtor = nullptr;
   if (const RecordType *RT = ElementType->getAs<RecordType>()) {
     CXXRecordDecl *RD = cast<CXXRecordDecl>(RT->getDecl());
     if (RD->hasDefinition() && !RD->hasTrivialDestructor()) {
       Dtor = RD->getDestructor();
 

cfe/trunk/test/CodeGenCXX/ubsan-vtable-checks.cpp

+// RUN: %clang_cc1 -std=c++11 -triple %itanium_abi_triple -emit-llvm -fsanitize=null %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-NULL --check-prefix=ITANIUM
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-windows -emit-llvm -fsanitize=null %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-NULL --check-prefix=MSABI
+// RUN: %clang_cc1 -std=c++11 -triple %itanium_abi_triple -emit-llvm -fsanitize=vptr %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-VPTR --check-prefix=ITANIUM
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-windows -emit-llvm -fsanitize=vptr %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-VPTR --check-prefix=MSABI
+struct T {
+  virtual ~T() {}
+  virtual int v() { return 1; }
+};
+
+struct U : T {
+  ~U();
+  virtual int v() { return 2; }
+};
+
+U::~U() {}
+
+// ITANIUM: define i32 @_Z5get_vP1T
+// MSABI: define i32 @"\01?get_v
+int get_v(T* t) {
+  // First, we check that vtable is not loaded before a type check.
+  // CHECK-NULL-NOT: load {{.*}} (%struct.T*{{.*}})**, {{.*}} (%struct.T*{{.*}})***
+  // CHECK-NULL: [[UBSAN_CMP_RES:%[0-9]+]] = icmp ne %struct.T* %{{[_a-z0-9]+}}, null
+  // CHECK-NULL-NEXT: br i1 [[UBSAN_CMP_RES]], label %{{.*}}, label %{{.*}}
+  // CHECK-NULL: call void @__ubsan_handle_type_mismatch_abort
+  // Second, we check that vtable is actually loaded once the type check is done.
+  // CHECK-NULL: load {{.*}} (%struct.T*{{.*}})**, {{.*}} (%struct.T*{{.*}})***
+  return t->v();
+}
+
+// ITANIUM: define void @_Z9delete_itP1T
+// MSABI: define void @"\01?delete_it
+void delete_it(T *t) {
+  // First, we check that vtable is not loaded before a type check.
+  // CHECK-VPTR-NOT: load {{.*}} (%struct.T*{{.*}})**, {{.*}} (%struct.T*{{.*}})***
+  // CHECK-VPTR: br i1 {{.*}} label %{{.*}}
+  // CHECK-VPTR: call void @__ubsan_handle_dynamic_type_cache_miss_abort
+  // Second, we check that vtable is actually loaded once the type check is done.
+  // CHECK-VPTR: load {{.*}} (%struct.T*{{.*}})**, {{.*}} (%struct.T*{{.*}})***
+  delete t;
+}