[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

1 ↗

(On Diff #11458)

Can you fold this into an existing test file?

13 ↗

(On Diff #11458)

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

15 ↗

(On Diff #11458)

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/CodeGenCXX/catch-undef-behavior.cpp

 // RUN: %clang_cc1 -std=c++11 -fsanitize=signed-integer-overflow,integer-divide-by-zero,float-divide-by-zero,shift,unreachable,return,vla-bound,alignment,null,vptr,object-size,float-cast-overflow,bool,enum,array-bounds,function -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -fsanitize=vptr -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=DOWNCAST-NULL
 
 struct S {
   double d;
   int a, b;
   virtual int f();
 };
 
 struct T : S {};
int bad_enum_value() {
   // CHECK: %[[E3:.*]] = icmp ule i32 {{.*}}, 2147483647
   // CHECK: br i1 %[[E3]]
   // CHECK: call void @__ubsan_handle_load_invalid_value(
   int c = e3;
   return a + b + c;
 }
 
 // CHECK: @_Z20bad_downcast_pointer
+// DOWNCAST-NULL: @_Z20bad_downcast_pointer
 void bad_downcast_pointer(S *p) {
   // CHECK: %[[NONNULL:.*]] = icmp ne {{.*}}, null
   // CHECK: br i1 %[[NONNULL]],
 
+  // A null poiner access is guarded without -fsanitize=null.
+  // DOWNCAST-NULL: %[[NONNULL:.*]] = icmp ne {{.*}}, null
+  // DOWNCAST-NULL: br i1 %[[NONNULL]],
+
   // CHECK: %[[SIZE:.*]] = call i64 @llvm.objectsize.i64.p0i8(
   // CHECK: %[[E1:.*]] = icmp uge i64 %[[SIZE]], 24
   // CHECK: %[[MISALIGN:.*]] = and i64 %{{.*}}, 7
   // CHECK: %[[E2:.*]] = icmp eq i64 %[[MISALIGN]], 0
   // CHECK: %[[E12:.*]] = and i1 %[[E1]], %[[E2]]
   // CHECK: br i1 %[[E12]],
 
   // CHECK: call void @__ubsan_handle_type_mismatch
   // CHECK: br label
 
   // CHECK: br i1 %{{.*}},
 
   // CHECK: call void @__ubsan_handle_dynamic_type_cache_miss
   // CHECK: br label
+
+  // DOWNCAST-NULL: call void @__ubsan_handle_dynamic_type_cache_miss
+  // DOWNCAST-NULL: br label
   (void) static_cast<T*>(p);
 }
 
 // CHECK: @_Z22bad_downcast_reference
 void bad_downcast_reference(S &p) {
   // CHECK: %[[E1:.*]] = icmp ne {{.*}}, null
   // CHECK-NOT: br i1
   // CHECK: %[[SIZE:.*]] = call i64 @llvm.objectsize.i64.p0i8(