diff mbox

Fix sanitizer/67258 (by cherry picking upstream patch)

Message ID 20150909161825.GC432@x4
State New
Headers show

Commit Message

Markus Trippelsdorf Sept. 9, 2015, 4:18 p.m. UTC 
Tested on ppc64le.
OK for trunk and gcc-5?

	PR sanitizer/67258
	* ubsan/ubsan_type_hash.cc: Cherry pick upstream r244101.

Upstream patch:
commit 1d2477faafda9ad2cc19927b3c31efd22747f013
Author: Alexey Samsonov <vonosmas@gmail.com>
Date:   Wed Aug 5 19:35:46 2015 +0000

    [UBSan] Fix UBSan-vptr false positive.

    Offset from vptr to the start of most-derived object can actually
    be positive in some virtual base class vtables.

    Patch by Stephan Bergmann!

    git-svn-id: https://llvm.org/svn/llvm-project/compiler-rt/trunk@244101 91177308-0d34-0410-b5e6-96231b3b80d8

Comments

Jakub Jelinek Sept. 9, 2015, 4:25 p.m. UTC | #1 
On Wed, Sep 09, 2015 at 06:18:25PM +0200, Markus Trippelsdorf wrote:
> Tested on ppc64le.
> OK for trunk and gcc-5?
> 
> 	PR sanitizer/67258
> 	* ubsan/ubsan_type_hash.cc: Cherry pick upstream r244101.

Please add
-fno-sanitize-recover=vptr
to dg-options.

Ok with that change.

> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ubsan/vptr-10.C
> @@ -0,0 +1,15 @@
> +// { dg-do run }
> +// { dg-options "-fsanitize=vptr" }
> +
> +struct A
> +{
> +    virtual ~A() {}
> +};
> +struct B : virtual A {};
> +struct C : virtual A {};
> +struct D : B, virtual C {};
> +
> +int main()
> +{
> +    D d;
> +}
> diff --git a/libsanitizer/ubsan/ubsan_type_hash.cc b/libsanitizer/ubsan/ubsan_type_hash.cc
> index d01009426db0..5eab1f561f27 100644
> --- a/libsanitizer/ubsan/ubsan_type_hash.cc
> +++ b/libsanitizer/ubsan/ubsan_type_hash.cc
> @@ -186,8 +186,8 @@ namespace {
>  
>  struct VtablePrefix {
>    /// The offset from the vptr to the start of the most-derived object.
> -  /// This should never be greater than zero, and will usually be exactly
> -  /// zero.
> +  /// This will only be greater than zero in some virtual base class vtables
> +  /// used during object con-/destruction, and will usually be exactly zero.
>    sptr Offset;
>    /// The type_info object describing the most-derived class type.
>    std::type_info *TypeInfo;
> @@ -197,7 +197,7 @@ VtablePrefix *getVtablePrefix(void *Object) {
>    if (!*VptrPtr)
>      return 0;
>    VtablePrefix *Prefix = *VptrPtr - 1;
> -  if (Prefix->Offset > 0 || !Prefix->TypeInfo)
> +  if (!Prefix->TypeInfo)
>      // This can't possibly be a valid vtable.
>      return 0;
>    return Prefix;
> -- 
> 2.5.1
> 
> -- 
> Markus

	Jakub
diff mbox

Patch

diff --git a/lib/ubsan/ubsan_type_hash_itanium.cc b/lib/ubsan/ubsan_type_hash_itanium.cc
index 5cd46df16a33..b84e88d4c71d 100644
--- a/lib/ubsan/ubsan_type_hash_itanium.cc
+++ b/lib/ubsan/ubsan_type_hash_itanium.cc
@@ -185,8 +185,8 @@  namespace {

 struct VtablePrefix {
   /// The offset from the vptr to the start of the most-derived object.
-  /// This should never be greater than zero, and will usually be exactly
-  /// zero.
+  /// This will only be greater than zero in some virtual base class vtables
+  /// used during object con-/destruction, and will usually be exactly zero.
   sptr Offset;
   /// The type_info object describing the most-derived class type.
   std::type_info *TypeInfo;
@@ -196,7 +196,7 @@  VtablePrefix *getVtablePrefix(void *Vtable) {
   if (!Vptr)
     return 0;
   VtablePrefix *Prefix = Vptr - 1;
-  if (Prefix->Offset > 0 || !Prefix->TypeInfo)
+  if (!Prefix->TypeInfo)
     // This can't possibly be a valid vtable.
     return 0;
   return Prefix;
diff --git a/test/ubsan/TestCases/TypeCheck/vptr-virtual-base-construction.cpp b/test/ubsan/TestCases/TypeCheck/vptr-virtual-base-construction.cpp
new file mode 100644
index 000000000000..dc27d9f39ce3
--- /dev/null
+++ b/test/ubsan/TestCases/TypeCheck/vptr-virtual-base-construction.cpp
@@ -0,0 +1,13 @@ 
+// RUN: %clangxx -frtti -fsanitize=vptr -fno-sanitize-recover=vptr %s -o %t
+// RUN: %run %t
+
+// REQUIRES: cxxabi
+
+int volatile n;
+
+struct A { virtual ~A() {} };
+struct B: virtual A {};
+struct C: virtual A { ~C() { n = 0; } };
+struct D: virtual B, virtual C {};
+
+int main() { delete new D; }
---
 gcc/testsuite/g++.dg/ubsan/vptr-10.C  | 15 +++++++++++++++
 libsanitizer/ubsan/ubsan_type_hash.cc |  6 +++---
 2 files changed, 18 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-10.C

diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-10.C b/gcc/testsuite/g++.dg/ubsan/vptr-10.C
new file mode 100644
index 000000000000..719e942bdbe3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ubsan/vptr-10.C
@@ -0,0 +1,15 @@ 
+// { dg-do run }
+// { dg-options "-fsanitize=vptr" }
+
+struct A
+{
+    virtual ~A() {}
+};
+struct B : virtual A {};
+struct C : virtual A {};
+struct D : B, virtual C {};
+
+int main()
+{
+    D d;
+}
diff --git a/libsanitizer/ubsan/ubsan_type_hash.cc b/libsanitizer/ubsan/ubsan_type_hash.cc
index d01009426db0..5eab1f561f27 100644
--- a/libsanitizer/ubsan/ubsan_type_hash.cc
+++ b/libsanitizer/ubsan/ubsan_type_hash.cc
@@ -186,8 +186,8 @@  namespace {
 
 struct VtablePrefix {
   /// The offset from the vptr to the start of the most-derived object.
-  /// This should never be greater than zero, and will usually be exactly
-  /// zero.
+  /// This will only be greater than zero in some virtual base class vtables
+  /// used during object con-/destruction, and will usually be exactly zero.
   sptr Offset;
   /// The type_info object describing the most-derived class type.
   std::type_info *TypeInfo;
@@ -197,7 +197,7 @@  VtablePrefix *getVtablePrefix(void *Object) {
   if (!*VptrPtr)
     return 0;
   VtablePrefix *Prefix = *VptrPtr - 1;
-  if (Prefix->Offset > 0 || !Prefix->TypeInfo)
+  if (!Prefix->TypeInfo)
     // This can't possibly be a valid vtable.
     return 0;
   return Prefix;