[PATCH] D25448: [ubsan] Disable -fsanitize=vptr checks for devirtualized calls

[Vedant Kumar via cfe-commits]

vsk created this revision.
vsk added reviewers: pcc, rjmccall.
vsk added subscribers: krasin, cfe-commits.

ubsan reports a false positive 'invalid member call' diagnostic on the
following example (PR30478):

  struct Base1 {
    virtual int f1() { return 1; }
  };
  
  struct Base2 {
    virtual int f1() { return 2; }
  };
  
  struct Derived2 final : Base1, Base2 {
    int f1() override { return 3; }
  };
  
  int t1() {
    Derived2 d;
    return static_cast<Base2 *>(&d)->f1();
  }

Adding the "final" attribute to a most-derived class allows clang to
devirtualize member calls into an instance of that object. In this case,
it means that the wrong vptr (or, alternatively, the wrong type info) is
passed to ubsan's checkDynamicType routine.

By the time clang emits the instrumentation to check for UB member calls, there
doesn't seem to to be enough information to properly diagnose the case where
the dynamic type of the cast operand isn't "Derived". In light of that, the
simplest way to fix this FP should be to disable -fsanitize=vptr
instrumentation for devirtualized member calls.


https://reviews.llvm.org/D25448

Files:
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprCXX.cpp
  lib/CodeGen/CodeGenFunction.h
  test/CodeGenCXX/ubsan-devirtualized-calls.cpp


Index: test/CodeGenCXX/ubsan-devirtualized-calls.cpp
===================================================================
--- /dev/null
+++ test/CodeGenCXX/ubsan-devirtualized-calls.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -std=c++11 -triple %itanium_abi_triple -emit-llvm -fsanitize=vptr %s -o - | FileCheck %s
+
+struct Base1 {
+  virtual int f1() { return 1; }
+};
+
+struct Base2 {
+  virtual int f1() { return 2; }
+};
+
+struct Derived1 final : Base1 {
+  int f1() override { return 3; }
+};
+
+struct Derived2 final : Base1, Base2 {
+  int f1() override { return 3; }
+};
+
+// CHECK-LABEL: define i32 @_Z2t1v
+int t1() {
+  Derived1 d1;
+  return static_cast<Base1 *>(&d1)->f1();
+// CHECK-NOT: !nosanitize
+}
+
+// CHECK-LABEL: define i32 @_Z2t2v
+int t2() {
+  Derived2 d2;
+  return static_cast<Base1 *>(&d2)->f1();
+// CHECK-NOT: !nosanitize
+}
+
+// CHECK-LABEL: define i32 @_Z2t3v
+int t3() {
+  Derived2 d2;
+  return static_cast<Base2 *>(&d2)->f1();
+// CHECK-NOT: !nosanitize
+}
Index: lib/CodeGen/CodeGenFunction.h
===================================================================
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -33,6 +33,7 @@
 #include "clang/Frontend/CodeGenOptions.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Debug.h"
@@ -374,6 +375,9 @@
     return DominatingValue<T>::save(*this, value);
   }
 
+  /// Set of object pointers which are blacklisted from the UB sanitizer.
+  llvm::SmallPtrSet<llvm::Value *, 1> SanitizerBasePointerBlacklist;
+
 public:
   /// ObjCEHValueStack - Stack of Objective-C exception values, used for
   /// rethrows.
Index: lib/CodeGen/CGExprCXX.cpp
===================================================================
--- lib/CodeGen/CGExprCXX.cpp
+++ lib/CodeGen/CGExprCXX.cpp
@@ -194,6 +194,8 @@
   else
     This = EmitLValue(Base).getAddress();
 
+  if (SanOpts.has(SanitizerKind::Vptr) && DevirtualizedMethod)
+    SanitizerBasePointerBlacklist.insert(This.getPointer());
 
   if (MD->isTrivial() || (MD->isDefaulted() && MD->getParent()->isUnion())) {
     if (isa<CXXDestructorDecl>(MD)) return RValue::get(nullptr);
Index: lib/CodeGen/CGExpr.cpp
===================================================================
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -619,8 +619,11 @@
   //    -- the [pointer or glvalue] is used to access a non-static data member
   //       or call a non-static member function
   CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
+  bool SupportedMemberCall = (TCK == TCK_MemberCall)
+                                 ? !SanitizerBasePointerBlacklist.count(Ptr)
+                                 : false;
   if (SanOpts.has(SanitizerKind::Vptr) &&
-      (TCK == TCK_MemberAccess || TCK == TCK_MemberCall ||
+      (TCK == TCK_MemberAccess || SupportedMemberCall ||
        TCK == TCK_DowncastPointer || TCK == TCK_DowncastReference ||
        TCK == TCK_UpcastToVirtualBase) &&
       RD && RD->hasDefinition() && RD->isDynamicClass()) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D25448.74164.patch
Type: text/x-patch
Size: 3124 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161010/7ba9d526/attachment-0001.bin>