[UBsan] Skip -fsanitize=vptr instrumentations when the pointer value is null

[byoungyoung]

Summary

Skip Vptr checks when the pointer value is null.

In general static_cast on null pointers causes undefined behaviors, it may not depending on the contexts. This patch enforces to skip null pointers just like it was done in -fsanitize=null.

[rsmith]

Please provide a test.

lib/CodeGen/CGExpr.cpp

553–554

Nit: lowercase 'v'.

It'd be nice to expand on this a bit: a null pointer here is undefined behavior, but if -fsanitize=null is not enabled, we don't want to change the behavior of code in that case, so that the user doesn't have to fix all their null pointer bugs before they can find their type mismatch bugs (which are likely to be more serious).

[byoungyoung]

Expanded a comment, and added a testcase. I'm not sure whether the checks in the testcase would be enough, so please let me know if it doesn't.

[rsmith]

lib/CodeGen/CGExpr.cpp

464

Instead of the below fix, please instead fix this by changing this line to

if (SanOpts->Null || TCK == TCK_DowncastPointer)

test/CodeGen/ubsan-vptr-null.cpp

2

Can you fold this into an existing test file?

14

Please don't check the label names here: this test will fail in non-debug builds where we don't name blocks.

16

Likewise here.

[byoungyoung]

Update the patch as commented except the test cast folding. Richard, could you please point which file should I fold into for the testcase? As far as I checked, all existing ubsan tests are written in C (except type-blacklist one), but this case has to be done in C++.

[byoungyoung]

Ah... thanks again Richard to point where the test file is :(, and merged the test.

[rsmith]

LGTM

[byoungyoung]

rsmith@ - could you please land this patch as I don't have a commit permission?

lib/CodeGen/CGExpr.cpp

553–554

Thanks Richard for the comments! Let me change the patch as you suggested.

I was confused on your comment in -fsanitize=null, which says "When performing a pointer downcast, it's OK if the value is null. Skip the remaining checks in that case". Is this mean that the down-casted null pointer is a result of "defined behavior", or did you mean something else? From my shallow understanding and your review comments, it seems "undefined" though.

[samsonov]

Landed in r213393. Thanks!

lib/CodeGen/CGExpr.cpp

void CodeGenFunction::EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc,
   // 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;
 
   llvm::Value *Cond = nullptr;
   llvm::BasicBlock *Done = nullptr;
 
-  if (SanOpts->Null) {
+  if (SanOpts->Null || TCK == TCK_DowncastPointer) {
     // 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.
       // Skip the remaining checks in that case.
       Done = createBasicBlock("null");
if (SanOpts->Vptr &&
     CGM.getCXXABI().getMangleContext().mangleCXXRTTI(Ty.getUnqualifiedType(),
                                                      Out);
 
     // Blacklist based on the mangled type.
     if (!CGM.getSanitizerBlacklist().isBlacklistedType(Out.str())) {
       llvm::hash_code TypeHash = hash_value(Out.str());
 
       // Load the vptr, and compute hash_16_bytes(TypeHash, vptr).
       llvm::Value *Low = llvm::ConstantInt::get(Int64Ty, TypeHash);
       llvm::Type *VPtrTy = llvm::PointerType::get(IntPtrTy, 0);
       llvm::Value *VPtrAddr = Builder.CreateBitCast(Address, VPtrTy);
       llvm::Value *VPtrVal = Builder.CreateLoad(VPtrAddr);
       llvm::Value *High = Builder.CreateZExt(VPtrVal, Int64Ty);
 
       llvm::Value *Hash = emitHash16Bytes(Builder, Low, High);
       Hash = Builder.CreateTrunc(Hash, IntPtrTy);
 
       // Look the hash up in our cache.

test/CodeGen/ubsan-vptr-null.cpp

+// Verify ubsan vptr skip null pointer value cases.
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsanitize=vptr -emit-llvm %s -o - | FileCheck %s
+
+class Bar {
+public:
+  virtual ~Bar() {}
+};
+class Foo : public Bar {};
+
+// CHECK-LABEL: @_Z7checkmev
+void checkme() {
+  // CHECK: [[CMP_RES:%.*]] = icmp ne %class.Foo* %{{[0-9]+}}, null
+  // CHECK: br {{.*}} [[CMP_RES]], label %[[NOT_NULLBB:.*]], label %[[NULLBB:.*]]
+  // CHECK: br label %[[NULLBB]]
+  Bar *bar = 0;
+  Foo* foo = static_cast<Foo*>(bar); // static_cast on the null pointer value.
+  return;
+}