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

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

What about this comment?

[krasin]

lib/CodeGen/CGExprCXX.cpp

93

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

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

[krasin]

Do better job with destructors.

[krasin]

lib/CodeGen/CGExprCXX.cpp

93

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

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

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

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

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::EmitCXXDestructorCall(
     const CXXDestructorDecl *DD, const CGCallee &Callee, llvm::Value *This,
     llvm::Value *ImplicitParam, QualType ImplicitParamTy, const CallExpr *CE,
     StructorType Type) {
   CallArgList Args;
   commonEmitCXXMemberOrOperatorCall(*this, DD, This, ImplicitParam,
                                     ImplicitParamTy, CE, Args, nullptr);
   return EmitCall(CGM.getTypes().arrangeCXXStructorDeclaration(DD, Type),
                   Callee, ReturnValueSlot(), Args);
 }
 
 RValue CodeGenFunction::EmitCXXPseudoDestructorExpr(
                                             const CXXPseudoDestructorExpr *E) {
   QualType DestroyedType = E->getDestroyedType();
   if (DestroyedType.hasStrongOrWeakObjCLifetime()) {
     // Automatic Reference Counting:
     //   If the pseudo-expression names a retainable object with weak or
     //   strong lifetime, the object shall be released.
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) {
   // 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();
 
       if (Dtor->isVirtual()) {
         CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr, ElementType,
Ptr = Address(Builder.CreateInBoundsGEP(Ptr.getPointer(), GEP, "del.first"),
                   Ptr.getAlignment());
   }
 
   assert(ConvertTypeForMem(DeleteTy) == Ptr.getElementType());
 
   if (E->isArrayForm()) {
     EmitArrayDelete(*this, E, Ptr, DeleteTy);
   } else {
     EmitObjectDelete(*this, E, Ptr, DeleteTy);
   }
 
   EmitBlock(DeleteEnd);
 }
 
 static bool isGLValueFromPointerDeref(const Expr *E) {
   E = E->IgnoreParens();
 
   if (const auto *CE = dyn_cast<CastExpr>(E)) {
     if (!CE->getSubExpr()->isGLValue())
       return false;
     return isGLValueFromPointerDeref(CE->getSubExpr());
   }