-fsanitize=vptr warnings on bad static types in dynamic_cast and typeid

[sberg]

Summary

...when such an operation is done on an object during con-/destruction.

This adds a test case to compiler-rt/test/ubsan/TestCases/TypeCheck/vptr.cpp that, unlike the existing test cases there, wants to detect multiple UBSan warnings in one go. Therefore, that file had to be changed from globally using -fno-sanitize-recover to individually using halt_on_error only where appropriate.

[sberg]

Some items I'm unclear about:

• Should those additional calls to EmitTypeCheck be restricted via those SkippedChecks or not?

• Maybe there's a better way to add tests for this than to try shoehorn it into the existing vptr.cpp?

[sberg]

Oh, I'd meant to upload the version of the patch where the newly added calls to EmitTypeCheck use the default Alignment and SkippedChecks arguments, instead of explicitly skipping Alignment and ObjectSize checks. (That's the version I had successfully used in a LibreOffice UBSan 'make check' test build. But I still don't know which version is better.)

[sberg]

friendly ping; any input on my two questions maybe?

[vsk]

I don't think any checks can be skipped in the newly-introduced calls to EmitTypeCheck. Clang uses EmitDynamicCast on arbitrary addresses, not just addresses which are known to be checked for alignment/etc. Regarding the test update, I think it makes sense to extend the runtime test in vptr.cpp, but that we'd also benefit from a small/narrow IR test (e.g in test/CodeGenCXX/ubsan-vtable-checks.cpp). With the added test I think this patch would be in great shape.

[sberg]

added a small IR test

[vsk]

Thanks, lgtm. Could you wait a day or two before committing? IIRC Richard or Nico have a -fsanitize=vptr Chromium bot, and they may have further comments.

clang/lib/CodeGen/CGExpr.cpp

 
 bool CodeGenFunction::isNullPointerAllowed(TypeCheckKind TCK) {
   return TCK == TCK_DowncastPointer || TCK == TCK_Upcast ||
-         TCK == TCK_UpcastToVirtualBase;
+         TCK == TCK_UpcastToVirtualBase || TCK == TCK_DynamicOperation;
 }
 
 bool CodeGenFunction::isVptrCheckRequired(TypeCheckKind TCK, QualType Ty) {

   return (RD && RD->hasDefinition() && RD->isDynamicClass()) &&
          (TCK == TCK_MemberAccess || TCK == TCK_MemberCall ||
           TCK == TCK_DowncastPointer || TCK == TCK_DowncastReference ||
-          TCK == TCK_UpcastToVirtualBase);
+          TCK == TCK_UpcastToVirtualBase || TCK == TCK_DynamicOperation);
 }
 
 bool CodeGenFunction::sanitizePerformTypeCheck() const {

clang/lib/CodeGen/CGExprCXX.cpp

   // Get the vtable pointer.
   Address ThisPtr = CGF.EmitLValue(E).getAddress();
 
+  QualType SrcRecordTy = E->getType();
+
+  // C++ [class.cdtor]p4:
+  //   If the operand of typeid refers to the object under construction or
+  //   destruction and the static type of the operand is neither the constructor
+  //   or destructor’s class nor one of its bases, the behavior is undefined.
+  SanitizerSet SkippedChecks;
+  SkippedChecks.set(SanitizerKind::Alignment, true);
+  SkippedChecks.set(SanitizerKind::ObjectSize, true);
+  CGF.EmitTypeCheck(CodeGenFunction::TCK_DynamicOperation, E->getExprLoc(),
+                    ThisPtr.getPointer(), SrcRecordTy,
+                    /*Alignment=*/CharUnits::Zero(), SkippedChecks);
+
   // C++ [expr.typeid]p2:
   //   If the glvalue expression is obtained by applying the unary * operator to
   //   a pointer and the pointer is a null pointer value, the typeid expression

   // However, this paragraph's intent is not clear.  We choose a very generous
   // interpretation which implores us to consider comma operators, conditional
   // operators, parentheses and other such constructs.
-  QualType SrcRecordTy = E->getType();
   if (CGF.CGM.getCXXABI().shouldTypeidBeNullChecked(
           isGLValueFromPointerDeref(E), SrcRecordTy)) {
     llvm::BasicBlock *BadTypeidBlock =

   CGM.EmitExplicitCastExprType(DCE, this);
   QualType DestTy = DCE->getTypeAsWritten();
 
-  if (DCE->isAlwaysNull())
-    if (llvm::Value *T = EmitDynamicCastToNull(*this, DestTy))
-      return T;
-
   QualType SrcTy = DCE->getSubExpr()->getType();
 
   // C++ [expr.dynamic.cast]p7:

     DestRecordTy = DestTy->castAs<ReferenceType>()->getPointeeType();
   }
 
+  // C++ [class.cdtor]p5:
+  //   If the operand of the dynamic_cast refers to the object under
+  //   construction or destruction and the static type of the operand is not a
+  //   pointer to or object of the constructor or destructor’s own class or one
+  //   of its bases, the dynamic_cast results in undefined behavior.
+  SanitizerSet SkippedChecks;
+  SkippedChecks.set(SanitizerKind::Alignment, true);
+  SkippedChecks.set(SanitizerKind::ObjectSize, true);
+  EmitTypeCheck(TCK_DynamicOperation, DCE->getExprLoc(), ThisAddr.getPointer(), SrcRecordTy,
+                /*Alignment=*/CharUnits::Zero(), SkippedChecks);
+
+  if (DCE->isAlwaysNull())
+    if (llvm::Value *T = EmitDynamicCastToNull(*this, DestTy))
+      return T;
+
   assert(SrcRecordTy->isRecordType() && "source type must be a record type!");
 
   // C++ [expr.dynamic.cast]p4: 

clang/lib/CodeGen/CodeGenFunction.h

     /// object within its lifetime.
     TCK_UpcastToVirtualBase,
     /// Checking the value assigned to a _Nonnull pointer. Must not be null.
-    TCK_NonnullAssign
+    TCK_NonnullAssign,
+    /// Checking the operand of a dynamic_cast or a typeid expression.  Must be
+    /// null or an object within its lifetime.
+    TCK_DynamicOperation
   };
 
   /// Determine whether the pointer type check \p TCK permits null pointers.

compiler-rt/lib/ubsan/ubsan_handlers.cc

 const char *TypeCheckKinds[] = {
     "load of", "store to", "reference binding to", "member access within",
     "member call on", "constructor call on", "downcast of", "downcast of",
-    "upcast of", "cast to virtual base of", "_Nonnull binding to"};
+    "upcast of", "cast to virtual base of", "_Nonnull binding to",
+    "dynamic operation on"};
 }
 
 static void handleTypeMismatchImpl(TypeMismatchData *Data, ValueHandle Pointer,

compiler-rt/test/ubsan/TestCases/TypeCheck/vptr.cpp

-// RUN: %clangxx -frtti -fsanitize=null,vptr -fno-sanitize-recover=null,vptr -g %s -O3 -o %t -mllvm -enable-tail-merge=false
-// RUN: %run %t rT && %run %t mT && %run %t fT && %run %t cT
-// RUN: %run %t rU && %run %t mU && %run %t fU && %run %t cU
-// RUN: %run %t rS && %run %t rV && %run %t oV
-// RUN: %env_ubsan_opts=print_stacktrace=1 not %run %t mS 2>&1 | FileCheck %s --check-prefix=CHECK-MEMBER --check-prefix=CHECK-%os-MEMBER --strict-whitespace
-// RUN: %env_ubsan_opts=print_stacktrace=1 not %run %t fS 2>&1 | FileCheck %s --check-prefix=CHECK-MEMFUN --strict-whitespace
-// RUN: %env_ubsan_opts=print_stacktrace=1 not %run %t cS 2>&1 | FileCheck %s --check-prefix=CHECK-DOWNCAST --check-prefix=CHECK-%os-DOWNCAST --strict-whitespace
-// RUN: %env_ubsan_opts=print_stacktrace=1 not %run %t mV 2>&1 | FileCheck %s --check-prefix=CHECK-MEMBER --check-prefix=CHECK-%os-MEMBER --strict-whitespace
-// RUN: %env_ubsan_opts=print_stacktrace=1 not %run %t fV 2>&1 | FileCheck %s --check-prefix=CHECK-MEMFUN --strict-whitespace
-// RUN: %env_ubsan_opts=print_stacktrace=1 not %run %t cV 2>&1 | FileCheck %s --check-prefix=CHECK-DOWNCAST --check-prefix=CHECK-%os-DOWNCAST --strict-whitespace
-// RUN: %env_ubsan_opts=print_stacktrace=1 not %run %t oU 2>&1 | FileCheck %s --check-prefix=CHECK-OFFSET --check-prefix=CHECK-%os-OFFSET --strict-whitespace
-// RUN: %env_ubsan_opts=print_stacktrace=1 not %run %t m0 2>&1 | FileCheck %s --check-prefix=CHECK-INVALID-MEMBER --check-prefix=CHECK-%os-NULL-MEMBER --strict-whitespace
-// RUN: %env_ubsan_opts=print_stacktrace=1 not %run %t m0 2>&1 | FileCheck %s --check-prefix=CHECK-INVALID-MEMBER --check-prefix=CHECK-%os-NULL-MEMBER --strict-whitespace
-// RUN: not %run %t nN 2>&1 | FileCheck %s --check-prefix=CHECK-NULL-MEMFUN --strict-whitespace
+// RUN: %clangxx -frtti -fsanitize=null,vptr -g %s -O3 -o %t -mllvm -enable-tail-merge=false
+// RUN: %env_ubsan_opts=halt_on_error=1 %run %t rT
+// RUN: %env_ubsan_opts=halt_on_error=1 %run %t mT
+// RUN: %env_ubsan_opts=halt_on_error=1 %run %t fT
+// RUN: %env_ubsan_opts=halt_on_error=1 %run %t cT
+// RUN: %env_ubsan_opts=halt_on_error=1 %run %t rU
+// RUN: %env_ubsan_opts=halt_on_error=1 %run %t mU
+// RUN: %env_ubsan_opts=halt_on_error=1 %run %t fU
+// RUN: %env_ubsan_opts=halt_on_error=1 %run %t cU
+// RUN: %env_ubsan_opts=halt_on_error=1 %run %t rS
+// RUN: %env_ubsan_opts=halt_on_error=1 %run %t rV
+// RUN: %env_ubsan_opts=halt_on_error=1 %run %t oV
+// RUN: %env_ubsan_opts=halt_on_error=1 %run %t zN
+// RUN: %env_ubsan_opts=halt_on_error=1:print_stacktrace=1 not %run %t mS 2>&1 | FileCheck %s --check-prefix=CHECK-MEMBER --check-prefix=CHECK-%os-MEMBER --strict-whitespace
+// RUN: %env_ubsan_opts=halt_on_error=1:print_stacktrace=1 not %run %t fS 2>&1 | FileCheck %s --check-prefix=CHECK-MEMFUN --strict-whitespace
+// RUN: %env_ubsan_opts=halt_on_error=1:print_stacktrace=1 not %run %t cS 2>&1 | FileCheck %s --check-prefix=CHECK-DOWNCAST --check-prefix=CHECK-%os-DOWNCAST --strict-whitespace
+// RUN: %env_ubsan_opts=halt_on_error=1:print_stacktrace=1 not %run %t mV 2>&1 | FileCheck %s --check-prefix=CHECK-MEMBER --check-prefix=CHECK-%os-MEMBER --strict-whitespace
+// RUN: %env_ubsan_opts=halt_on_error=1:print_stacktrace=1 not %run %t fV 2>&1 | FileCheck %s --check-prefix=CHECK-MEMFUN --strict-whitespace
+// RUN: %env_ubsan_opts=halt_on_error=1:print_stacktrace=1 not %run %t cV 2>&1 | FileCheck %s --check-prefix=CHECK-DOWNCAST --check-prefix=CHECK-%os-DOWNCAST --strict-whitespace
+// RUN: %env_ubsan_opts=halt_on_error=1:print_stacktrace=1 not %run %t oU 2>&1 | FileCheck %s --check-prefix=CHECK-OFFSET --check-prefix=CHECK-%os-OFFSET --strict-whitespace
+// RUN: %env_ubsan_opts=halt_on_error=1:print_stacktrace=1 not %run %t m0 2>&1 | FileCheck %s --check-prefix=CHECK-INVALID-MEMBER --check-prefix=CHECK-%os-NULL-MEMBER --strict-whitespace
+// RUN: %env_ubsan_opts=halt_on_error=1:print_stacktrace=1 not %run %t m0 2>&1 | FileCheck %s --check-prefix=CHECK-INVALID-MEMBER --check-prefix=CHECK-%os-NULL-MEMBER --strict-whitespace
+// RUN: %env_ubsan_opts=halt_on_error=1 not %run %t nN 2>&1 | FileCheck %s --check-prefix=CHECK-NULL-MEMFUN --strict-whitespace
+// RUN: %env_ubsan_opts=print_stacktrace=1 %run %t dT 2>&1 | FileCheck %s --check-prefix=CHECK-DYNAMIC --check-prefix=CHECK-%os-DYNAMIC --strict-whitespace
 
 // RUN: (echo "vptr_check:S"; echo "vptr_check:T"; echo "vptr_check:U") > %t.supp
-// RUN: %env_ubsan_opts=suppressions='"%t.supp"' %run %t mS
-// RUN: %env_ubsan_opts=suppressions='"%t.supp"' %run %t fS
-// RUN: %env_ubsan_opts=suppressions='"%t.supp"' %run %t cS
-// RUN: %env_ubsan_opts=suppressions='"%t.supp"' %run %t mV
-// RUN: %env_ubsan_opts=suppressions='"%t.supp"' %run %t fV
-// RUN: %env_ubsan_opts=suppressions='"%t.supp"' %run %t cV
-// RUN: %env_ubsan_opts=suppressions='"%t.supp"' %run %t oU
+// RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.supp"' %run %t mS
+// RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.supp"' %run %t fS
+// RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.supp"' %run %t cS
+// RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.supp"' %run %t mV
+// RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.supp"' %run %t fV
+// RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.supp"' %run %t cV
+// RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.supp"' %run %t oU
+// RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.supp"' %run %t dT
 
 // RUN: echo "vptr_check:S" > %t.loc-supp
-// RUN: %env_ubsan_opts=suppressions='"%t.loc-supp"' not %run %t x- 2>&1 | FileCheck %s --check-prefix=CHECK-LOC-SUPPRESS
+// RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.loc-supp"' not %run %t x- 2>&1 | FileCheck %s --check-prefix=CHECK-LOC-SUPPRESS
 
 // REQUIRES: stable-runtime, cxxabi
 // UNSUPPORTED: win32
 // Suppressions file not pushed to the device.
 // UNSUPPORTED: android
 #include <new>
+#include <typeinfo>
 #include <assert.h>
 #include <stdio.h>
 
 struct S {
   S() : a(0) {}
-  ~S() {}
+  ~S();
   int a;
   int f() { return 0; }
   virtual int v() { return 0; }

 
 struct V : S {};
 
-// Make p global so that lsan does not complain.
+namespace {
+  struct W {};
+}
+
 T *p = 0;
 
+bool dtorCheck = false;
+
 volatile void *sink1, *sink2;
 
 int access_p(T *p, char type);
 
+S::~S() {
+  if (dtorCheck)
+    access_p(p, '~');
+}
+
 int main(int argc, char **argv) {
   assert(argc > 1);
   fprintf(stderr, "Test case: %s\n", argv[1]);

   case 'n':
     // CHECK-NULL-MEMFUN: vptr.cpp:[[@LINE+1]]:15: runtime error: member call on null pointer of type 'T'
     return p->g();
+
+  case 'd':
+    dtorCheck = true;
+    delete p;
+    dtorCheck = false;
+    return 0;
+  case '~':
+    // CHECK-DYNAMIC: vptr.cpp:[[@LINE+6]]:11: runtime error: dynamic operation on address [[PTR:0x[0-9a-f]*]] which does not point to an object of type 'T'
+    // CHECK-DYNAMIC-NEXT: [[PTR]]: note: object is of type 'S'
+    // CHECK-DYNAMIC-NEXT: {{^ .. .. .. ..  .. .. .. .. .. .. .. ..  }}
+    // CHECK-DYNAMIC-NEXT: {{^              \^~~~~~~~~~~(~~~~~~~~~~~~)? *$}}
+    // CHECK-DYNAMIC-NEXT: {{^              vptr for}} 'S'
+    // CHECK-Linux-DYNAMIC: #0 {{.*}}access_p{{.*}}vptr.cpp:[[@LINE+1]]
+    (void)dynamic_cast<V*>(p);
+    // CHECK-DYNAMIC: vptr.cpp:[[@LINE+6]]:11: runtime error: dynamic operation on address [[PTR:0x[0-9a-f]*]] which does not point to an object of type 'T'
+    // CHECK-DYNAMIC-NEXT: [[PTR]]: note: object is of type 'S'
+    // CHECK-DYNAMIC-NEXT: {{^ .. .. .. ..  .. .. .. .. .. .. .. ..  }}
+    // CHECK-DYNAMIC-NEXT: {{^              \^~~~~~~~~~~(~~~~~~~~~~~~)? *$}}
+    // CHECK-DYNAMIC-NEXT: {{^              vptr for}} 'S'
+    // CHECK-Linux-DYNAMIC: #0 {{.*}}access_p{{.*}}vptr.cpp:[[@LINE+1]]
+    (void)dynamic_cast<W*>(p);
+    try {
+      // CHECK-DYNAMIC: vptr.cpp:[[@LINE+6]]:13: runtime error: dynamic operation on address [[PTR:0x[0-9a-f]*]] which does not point to an object of type 'T'
+      // CHECK-DYNAMIC-NEXT: [[PTR]]: note: object is of type 'S'
+      // CHECK-DYNAMIC-NEXT: {{^ .. .. .. ..  .. .. .. .. .. .. .. ..  }}
+      // CHECK-DYNAMIC-NEXT: {{^              \^~~~~~~~~~~(~~~~~~~~~~~~)? *$}}
+      // CHECK-DYNAMIC-NEXT: {{^              vptr for}} 'S'
+      // CHECK-Linux-DYNAMIC: #0 {{.*}}access_p{{.*}}vptr.cpp:[[@LINE+1]]
+      (void)dynamic_cast<V&>(*p);
+    } catch (std::bad_cast &) {}
+    // CHECK-DYNAMIC: vptr.cpp:[[@LINE+6]]:18: runtime error: dynamic operation on address [[PTR:0x[0-9a-f]*]] which does not point to an object of type 'T'
+    // CHECK-DYNAMIC-NEXT: [[PTR]]: note: object is of type 'S'
+    // CHECK-DYNAMIC-NEXT: {{^ .. .. .. ..  .. .. .. .. .. .. .. ..  }}
+    // CHECK-DYNAMIC-NEXT: {{^              \^~~~~~~~~~~(~~~~~~~~~~~~)? *$}}
+    // CHECK-DYNAMIC-NEXT: {{^              vptr for}} 'S'
+    // CHECK-Linux-DYNAMIC: #0 {{.*}}access_p{{.*}}vptr.cpp:[[@LINE+1]]
+    (void)typeid(*p);
+    return 0;
+
+  case 'z':
+    (void)dynamic_cast<V*>(p);
+    try {
+      (void)typeid(*p);
+    } catch (std::bad_typeid &) {}
+    return 0;
   }
   return 0;
 }