[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

       (TCK == TCK_MemberAccess || TCK == TCK_MemberCall ||
        TCK == TCK_DowncastPointer || TCK == TCK_DowncastReference) &&
       RD && RD->hasDefinition() && RD->isDynamicClass()) {
+
+    llvm::BasicBlock *VptrNullBlock = nullptr;
+    llvm::BasicBlock *VptrNotNullBlock = nullptr;
+
+    // Skip Vptr check instrumentations when the pointer value is null.
+    // FIXME: Add optional flags.
+    if (TCK_DowncastPointer && !SanOpts->Null) {
+      Cond = Builder.CreateICmpNE(
+        Address, llvm::Constant::getNullValue(Address->getType()));
+
+      VptrNullBlock = createBasicBlock("vptr_null");
+      VptrNotNullBlock = createBasicBlock("not.vptr_null");
+
+      Builder.CreateCondBr(Cond, VptrNotNullBlock, VptrNullBlock);
+      EmitBlock(VptrNotNullBlock);
+    }
+
     // Compute a hash of the mangled name of the type.
     //
     // FIXME: This is not guaranteed to be deterministic! Move to a

     EmitCheck(Builder.CreateICmpEQ(CacheVal, Hash),
               "dynamic_type_cache_miss", StaticData, DynamicData,
               CRK_AlwaysRecoverable);
+
+    if (VptrNullBlock) {
+      Builder.CreateBr(VptrNullBlock);
+      EmitBlock(VptrNullBlock);
+    }
   }
 
   if (Done) {