[RFC] Sanitize upcasts and conversion to virtual base

[samsonov]

Summary

This change adds UBSan check to upcasts. Namely, when we
perform derived-to-base conversion, we:

1. check that the pointer-to-derived has suitable alignment and underlying storage, if this pointer is non-null.
2. if vptr-sanitizer is enabled, and we perform conversion to virtual base, we check that pointer-to-derived has a matching vptr.

For now, this change lacks UBSan output tests in compiler-rt, I'm sending
it as an RFC to check that I'm doing the correct thing, and to agree
on error messages and enums wordings.

[samsonov]

Remove bogus comment.

[rsmith]

tools/clang/lib/CodeGen/CGClass.cpp

186

I am not sure that the SkipNullCheck behavior here is right. If NullCheckValue is false, then I think we should be performing a null check inside EmitTypeCheck rather than assuming the pointer is non-null.

[samsonov]

tools/clang/lib/CodeGen/CGClass.cpp

186

That is, just pass "false" here? But in some cases we're calling CodeGenFunction::GetAddressOfBaseClass on values which are guaranteed to be non-null (e.g. on materialized temporaries), as they happen in compiler-generated code. I'm not sure we still want to emit null-check in EmitTypeCheck in this case.

[rsmith]

OK, let's skip emitting the null check for now. If someone manages to get a null in here, then we'll probably crash on the UB, which is not ideal but acceptable.

projects/compiler-rt/lib/ubsan/ubsan_handlers.cc

static bool ignoreReport(SourceLocation SLoc, ReportOptions Opts) {
   // it's possible that location was required by concurrently running thread.
   // In this case, we should continue the execution to ensure that any of
   // threads will grab the report mutex and print the report before
   // crashing the program.
   return SLoc.isDisabled() && !Opts.DieAfterReport;
 }
 
 namespace __ubsan {
-  const char *TypeCheckKinds[] = {
     "load of", "store to", "reference binding to", "member access within",
-    "member call on", "constructor call on", "downcast of", "downcast of"
-  };
+    "member call on", "constructor call on", "downcast of", "downcast of",
+    "upcast of", "cast to virtual base of"};
 }
 
 static void handleTypeMismatchImpl(TypeMismatchData *Data, ValueHandle Pointer,
                                    Location FallbackLoc, ReportOptions Opts) {
   Location Loc = Data->Loc.acquire();
   // Use the SourceLocation from Data to track deduplication, even if 'invalid'
   if (ignoreReport(Loc.getSourceLocation(), Opts))
     return;

tools/clang/lib/CodeGen/CGClass.cpp

ApplyNonVirtualAndVirtualOffset(CodeGenFunction &CGF, llvm::Value *ptr,
   }
   
   // Apply the base offset.
   ptr = CGF.Builder.CreateBitCast(ptr, CGF.Int8PtrTy);
   ptr = CGF.Builder.CreateInBoundsGEP(ptr, baseOffset, "add.ptr");
   return ptr;
 }
 
-llvm::Value *
-CodeGenFunction::GetAddressOfBaseClass(llvm::Value *Value, 
-                                       const CXXRecordDecl *Derived,
-                                       CastExpr::path_const_iterator PathBegin,
-                                       CastExpr::path_const_iterator PathEnd,
-                                       bool NullCheckValue) {
+llvm::Value *CodeGenFunction::GetAddressOfBaseClass(
+    llvm::Value *Value, const CXXRecordDecl *Derived,
+    CastExpr::path_const_iterator PathEnd, bool NullCheckValue,
+    SourceLocation Loc) {
   assert(PathBegin != PathEnd && "Base path should not be empty!");
 
   CastExpr::path_const_iterator Start = PathBegin;
   const CXXRecordDecl *VBase = nullptr;
 
   // Sema has done some convenient canonicalization here: if the
   // access path involved any virtual steps, the conversion path will
   // *start* with a step down to the correct virtual base subobject,
if (VBase && Derived->hasAttr<FinalAttr>()) {
     NonVirtualOffset += vBaseOffset;
     VBase = nullptr; // we no longer have a virtual step
   }
 
   // Get the base pointer type.
   llvm::Type *BasePtrTy = 
     ConvertType((PathEnd[-1])->getType())->getPointerTo();
 
+  QualType DerivedTy = getContext().getRecordType(Derived);
+  CharUnits DerivedAlign = getContext().getTypeAlignInChars(DerivedTy);
+
   // If the static offset is zero and we don't have a virtual step,
   // just do a bitcast; null checks are unnecessary.
   if (NonVirtualOffset.isZero() && !VBase) {
+    if (sanitizePerformTypeCheck()) {
+      EmitTypeCheck(TCK_Upcast, Loc, Value, DerivedTy, DerivedAlign,
+                    !NullCheckValue);
+    }
+    // Don't skip null check in this case!
     return Builder.CreateBitCast(Value, BasePtrTy);
   }
 
   llvm::BasicBlock *origBB = nullptr;
   llvm::BasicBlock *endBB = nullptr;
 
   // Skip over the offset (and the vtable load) if we're supposed to
   // null-check the pointer.
   if (NullCheckValue) {
     origBB = Builder.GetInsertBlock();
     llvm::BasicBlock *notNullBB = createBasicBlock("cast.notnull");
     endBB = createBasicBlock("cast.end");
     
     llvm::Value *isNull = Builder.CreateIsNull(Value);
     Builder.CreateCondBr(isNull, endBB, notNullBB);
     EmitBlock(notNullBB);
   }
 
+  if (sanitizePerformTypeCheck()) {
+    EmitTypeCheck(VBase ? TCK_UpcastToVirtualBase : TCK_Upcast, Loc, Value,
+                  DerivedTy, DerivedAlign, true);
+  }
+
   // Compute the virtual offset.
   llvm::Value *VirtualOffset = nullptr;
   if (VBase) {
     VirtualOffset =
       CGM.getCXXABI().GetVirtualBaseClassOffset(*this, Value, Derived, VBase);
   }
 
   // Apply both offsets.

tools/clang/lib/CodeGen/CGExpr.cpp

LValue CodeGenFunction::EmitMaterializeTemporaryExpr(
   for (unsigned I = Adjustments.size(); I != 0; --I) {
     SubobjectAdjustment &Adjustment = Adjustments[I-1];
     switch (Adjustment.Kind) {
     case SubobjectAdjustment::DerivedToBaseAdjustment:
       Object =
           GetAddressOfBaseClass(Object, Adjustment.DerivedToBase.DerivedClass,
                                 Adjustment.DerivedToBase.BasePath->path_begin(),
                                 Adjustment.DerivedToBase.BasePath->path_end(),
-                                /*NullCheckValue=*/ false);
+                                /*NullCheckValue=*/ false, E->getExprLoc());
       break;
 
     case SubobjectAdjustment::FieldAdjustment: {
       LValue LV = MakeAddrLValue(Object, E->getType());
       LV = EmitLValueForField(LV, Adjustment.Field);
       assert(LV.isSimple() &&
              "materialized temporary field is not a simple lvalue");
       Object = LV.getAddress();

 }
 
 bool CodeGenFunction::sanitizePerformTypeCheck() const {
   return SanOpts->Null | SanOpts->Alignment | SanOpts->ObjectSize |
          SanOpts->Vptr;
 }
 
 void CodeGenFunction::EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc,
-                                    llvm::Value *Address,
-                                    QualType Ty, CharUnits Alignment) {
+                                    llvm::Value *Address, QualType Ty,
+                                    CharUnits Alignment, bool SkipNullCheck) {
   if (!sanitizePerformTypeCheck())
     return;
 
   // Don't check pointers outside the default address space. The null check
   // isn't correct, the object-size check isn't supported by LLVM, and we can't
   // communicate the addresses to the runtime handler for the vptr check.
   if (Address->getType()->getPointerAddressSpace())
     return;
 
   SanitizerScope SanScope(this);
 
   llvm::Value *Cond = nullptr;
   llvm::BasicBlock *Done = nullptr;
 
-  if (SanOpts->Null || TCK == TCK_DowncastPointer) {
+  bool AllowNullPointers = TCK == TCK_DowncastPointer || TCK == TCK_Upcast ||
+                           TCK == TCK_UpcastToVirtualBase;
+  if ((SanOpts->Null || AllowNullPointers) && !SkipNullCheck) {
     // The glvalue must not be an empty glvalue.
     Cond = Builder.CreateICmpNE(
         Address, llvm::Constant::getNullValue(Address->getType()));
 
-    if (TCK == TCK_DowncastPointer) {
-      // When performing a pointer downcast, it's OK if the value is null.
+    if (AllowNullPointers) {
+      // When performing pointer casts, it's OK if the value is null.
       // Skip the remaining checks in that case.
       Done = createBasicBlock("null");
       llvm::BasicBlock *Rest = createBasicBlock("not.null");
       Builder.CreateCondBr(Cond, Rest, Done);
       EmitBlock(Rest);
       Cond = nullptr;
     }
   }
void CodeGenFunction::EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc,
   // C++11 [basic.life]p5,6:
   //   [For storage which does not refer to an object within its lifetime]
   //   The program has undefined behavior if:
   //    -- the [pointer or glvalue] is used to access a non-static data member
   //       or call a non-static member function
   CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
   if (SanOpts->Vptr &&
       (TCK == TCK_MemberAccess || TCK == TCK_MemberCall ||
-       TCK == TCK_DowncastPointer || TCK == TCK_DowncastReference) &&
+       TCK == TCK_DowncastPointer || TCK == TCK_DowncastReference ||
+       TCK == TCK_UpcastToVirtualBase) &&
       RD && RD->hasDefinition() && RD->isDynamicClass()) {
     // Compute a hash of the mangled name of the type.
     //
     // FIXME: This is not guaranteed to be deterministic! Move to a
     //        fingerprinting mechanism once LLVM provides one. For the time
     //        being the implementation happens to be deterministic.
     SmallString<64> MangledName;
     llvm::raw_svector_ostream Out(MangledName);
case CK_DerivedToBase: {
     const RecordType *DerivedClassTy =
       E->getSubExpr()->getType()->getAs<RecordType>();
     auto *DerivedClassDecl = cast<CXXRecordDecl>(DerivedClassTy->getDecl());
 
     LValue LV = EmitLValue(E->getSubExpr());
     llvm::Value *This = LV.getAddress();
 
     // Perform the derived-to-base conversion
-    llvm::Value *Base =
-      GetAddressOfBaseClass(This, DerivedClassDecl,
-                            E->path_begin(), E->path_end(),
-                            /*NullCheckValue=*/false);
+    llvm::Value *Base = GetAddressOfBaseClass(
+        This, DerivedClassDecl, E->path_begin(), E->path_end(),
+        /*NullCheckValue=*/false, E->getExprLoc());
 
     return MakeAddrLValue(Base, E->getType());
   }
   case CK_ToUnion:
     return EmitAggExprToLValue(E);
   case CK_BaseToDerived: {
     const RecordType *DerivedClassTy = E->getType()->getAs<RecordType>();
     auto *DerivedClassDecl = cast<CXXRecordDecl>(DerivedClassTy->getDecl());

tools/clang/lib/CodeGen/CGExprScalar.cpp

case CK_BaseToDerived: {
     return Derived;
   }
   case CK_UncheckedDerivedToBase:
   case CK_DerivedToBase: {
     const CXXRecordDecl *DerivedClassDecl =
       E->getType()->getPointeeCXXRecordDecl();
     assert(DerivedClassDecl && "DerivedToBase arg isn't a C++ object pointer!");
 
-    return CGF.GetAddressOfBaseClass(Visit(E), DerivedClassDecl,
-                                     CE->path_begin(), CE->path_end(),
-                                     ShouldNullCheckClassCastValue(CE));
+    return CGF.GetAddressOfBaseClass(
+        Visit(E), DerivedClassDecl, CE->path_begin(), CE->path_end(),
+        ShouldNullCheckClassCastValue(CE), CE->getExprLoc());
   }
   case CK_Dynamic: {
     Value *V = Visit(const_cast<Expr*>(E));
     const CXXDynamicCastExpr *DCE = cast<CXXDynamicCastExpr>(CE);
     return CGF.EmitDynamicCast(V, DCE);
   }
 
   case CK_ArrayToPointerDecay: {

tools/clang/lib/CodeGen/CodeGenFunction.h

GetAddressOfDirectBaseInCompleteClass(llvm::Value *Value,
                                         bool BaseIsVirtual);
 
   /// GetAddressOfBaseClass - This function will add the necessary delta to the
   /// load of 'this' and returns address of the base class.
   llvm::Value *GetAddressOfBaseClass(llvm::Value *Value,
                                      const CXXRecordDecl *Derived,
                                      CastExpr::path_const_iterator PathBegin,
                                      CastExpr::path_const_iterator PathEnd,
-                                     bool NullCheckValue);
+                                     bool NullCheckValue, SourceLocation Loc);
 
   llvm::Value *GetAddressOfDerivedClass(llvm::Value *Value,
                                         const CXXRecordDecl *Derived,
                                         CastExpr::path_const_iterator PathBegin,
                                         CastExpr::path_const_iterator PathEnd,
                                         bool NullCheckValue);
 
   /// GetVTTParameter - Return the VTT parameter that should be passed to a
enum TypeCheckKind {
     TCK_MemberCall,
     /// Checking the 'this' pointer for a constructor call.
     TCK_ConstructorCall,
     /// Checking the operand of a static_cast to a derived pointer type. Must be
     /// null or an object within its lifetime.
     TCK_DowncastPointer,
     /// Checking the operand of a static_cast to a derived reference type. Must
     /// be an object within its lifetime.
-    TCK_DowncastReference
+    TCK_DowncastReference,
+    /// Checking the operand of a cast to a base object. Must be suitably sized
+    /// and aligned.
+    TCK_Upcast,
+    /// Checking the operand of a cast to a virtual base object. Must be an
+    /// object within its lifetime.
+    TCK_UpcastToVirtualBase,
   };
 
   /// \brief Whether any type-checking sanitizers are enabled. If \c false,
   /// calls to EmitTypeCheck can be skipped.
   bool sanitizePerformTypeCheck() const;
 
   /// \brief Emit a check that \p V is the address of storage of the
   /// appropriate size and alignment for an object of type \p Type.
   void EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc, llvm::Value *V,
-                     QualType Type, CharUnits Alignment = CharUnits::Zero());
+                     QualType Type, CharUnits Alignment = CharUnits::Zero(),
+                     bool SkipNullCheck = false);
 
   /// \brief Emit a check that \p Base points into an array object, which
   /// we can access at index \p Index. \p Accessed should be \c false if we
   /// this expression is used as an lvalue, for instance in "&Arr[Idx]".
   void EmitBoundsCheck(const Expr *E, const Expr *Base, llvm::Value *Index,
                        QualType IndexType, bool Accessed);
 
   llvm::Value *EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,

tools/clang/test/CodeGenCXX/catch-undef-behavior.cpp

void indirect_function_call(void (*p)(int)) {
   // RTTI pointer check
   // CHECK: [[RTTIPTR:%[0-9]*]] = getelementptr <{ i32, i8* }>* [[PTR]], i32 0, i32 1
   // CHECK-NEXT: [[RTTI:%[0-9]*]] = load i8** [[RTTIPTR]]
   // CHECK-NEXT: [[RTTICMP:%[0-9]*]] = icmp eq i8* [[RTTI]], bitcast ({ i8*, i8* }* @_ZTIFviE to i8*)
   // CHECK-NEXT: br i1 [[RTTICMP]]
   p(42);
 }
 
+namespace UpcastPointerTest {
+struct S {};
+struct T : S { double d; };
+struct V : virtual S {};
+
+// CHECK-LABEL: upcast_pointer
+S* upcast_pointer(T* t) {
+  // Check for null pointer
+  // CHECK: %[[NONNULL:.*]] = icmp ne {{.*}}, null
+  // CHECK: br i1 %[[NONNULL]]
+
+  // Check alignment
+  // CHECK: %[[MISALIGN:.*]] = and i64 %{{.*}}, 7
+  // CHECK: icmp eq i64 %[[MISALIGN]], 0
+
+  // CHECK: call void @__ubsan_handle_type_mismatch
+  return t;
+}
+
+V getV();
+
+// CHECK-LABEL: upcast_to_vbase
+void upcast_to_vbase() {
+  // No need to check for null here, as we have a temporary here.
+
+  // CHECK-NOT: br i1
+
+  // CHECK: call i64 @llvm.objectsize
+  // CHECK: call void @__ubsan_handle_type_mismatch
+  // CHECK: call void @__ubsan_handle_dynamic_type_cache_miss
+  const S& s = getV();
+}
+}
+
 namespace CopyValueRepresentation {
   // CHECK-LABEL: define {{.*}} @_ZN23CopyValueRepresentation2S3aSERKS0_
   // CHECK-NOT: call {{.*}} @__ubsan_handle_load_invalid_value
   // CHECK-LABEL: define {{.*}} @_ZN23CopyValueRepresentation2S4aSEOS0_
   // CHECK-NOT: call {{.*}} @__ubsan_handle_load_invalid_value
   // CHECK-LABEL: define {{.*}} @_ZN23CopyValueRepresentation2S5C2ERKS0_
   // CHECK-NOT: call {{.*}} __ubsan_handle_load_invalid_value
   // CHECK-LABEL: define {{.*}} @_ZN23CopyValueRepresentation2S2C2ERKS0_
void test5() {
     S5 s51;
     callee5(s51);
     S5 s52;
     s52 = s51;
   }
 }
 
 // CHECK: attributes [[NR_NUW]] = { noreturn nounwind }
+
+