UBSan: crash less often on corrupted Vtables.

[krasin]

Summary

This CL adds a weak check for a Vtable prefix: for a well-formed
Vtable, we require the prefix to be within [-1<<20; 1<<20].

Practically, this solves most of the known cases when UBSan segfaults
without providing any useful diagnostics.

[krasin]

Hi Peter,

I realize that this CL misses a test and I would like to write one. I am currently stubled by the fact that the code changed lives within ubsan_type_hash_itanium.cc, so the test could only run on a subset of supported platforms. It's unclear to me what should I add to the test restrictions to make it happen.

[krasin]

Added a test.

[krasin]

remove debug printf.

[krasin]

Hi Peter,

I have added a test. Please, take a look.

[pcc]

test/ubsan/TestCases/TypeCheck/vptr_itanium.cpp

1 ↗

(On Diff #59291)

Maybe name this something like vptr-corrupted-vtable-itanium.cpp?

33 ↗

(On Diff #59291)

This isn't necessarily true for objects with sufficiently large bases. Can we change this to say that the object probably has an invalid vptr?

[krasin]

rename test; print better diagnostics for the suspicious offset.

[krasin]

Peter,

please, take another look.
Note: I have also fixed a hidden bug (reproduced under TSAN) that sometimes the test would fail due to TypeInfo being zero rather than big abs(Offset). Now I try to properly initialize that field.

test/ubsan/TestCases/TypeCheck/vptr_itanium.cpp

33 ↗

(On Diff #59291)

Done. Is it what you meant?

[krasin]

Peter,

friendly ping!

[pcc]

LGTM with nits

lib/ubsan/ubsan_handlers_cxx.cc

60

"abs(offset) too big" doesn't really mean anything to users, maybe drop it?

62

same here

lib/ubsan/ubsan_type_hash.h

56

The comment is inaccurate, your code allows negative offsets.

Also, just "offsets" is a little unclear, maybe say "offset to top"?

[krasin]

fix comments / error messages.

[krasin]

Please, take another look.

lib/ubsan/ubsan_handlers_cxx.cc

60

abs(offset to top) too big -- is it better now?
even if the users would not be able to grasp the details, it will let them to distinguish between different error messages here, which is important since some them (like this one) has a lower confidence of being right.

Let me know if you feel strongly about removing these details from the message. I will comply.

lib/ubsan/ubsan_type_hash.h

56

Done.

[pcc]

LGTM

lib/ubsan/ubsan_handlers_cxx.cc

60

Okay, sounds reasonable enough.

62

But you don't need to repeat the description here.

[krasin]

nits

lib/ubsan/ubsan_handlers_cxx.cc

static bool HandleDynamicTypeCacheMiss(
 
   ScopedReport R(Opts, Loc, ET);
 
   Diag(Loc, DL_Error,
        "%0 address %1 which does not point to an object of type %2")
     << TypeCheckKinds[Data->TypeCheckKind] << (void*)Pointer << Data->Type;
 
   // If possible, say what type it actually points to.
-  if (!DTI.isValid())
+  if (!DTI.isValid()) {
+    if (DTI.getOffset() < -VptrMaxOffsetToTop || DTI.getOffset() > VptrMaxOffsetToTop) {
+      Diag(Pointer, DL_Note, "object has a possibly invalid vptr: abs(offset to top) too big")
+          << TypeName(DTI.getMostDerivedTypeName())
+          << Range(Pointer, Pointer + sizeof(uptr), "possibly invalid vptr");
+    } else {
       Diag(Pointer, DL_Note, "object has invalid vptr")
           << TypeName(DTI.getMostDerivedTypeName())
           << Range(Pointer, Pointer + sizeof(uptr), "invalid vptr");
-  else if (!DTI.getOffset())
+    }
+  } else if (!DTI.getOffset())
     Diag(Pointer, DL_Note, "object is of type %0")
         << TypeName(DTI.getMostDerivedTypeName())
         << Range(Pointer, Pointer + sizeof(uptr), "vptr for %0");
   else
     // FIXME: Find the type at the specified offset, and include that
     //        in the note.
     Diag(Pointer - DTI.getOffset(), DL_Note,
          "object is base class subobject at offset %0 within object of type %1")

lib/ubsan/ubsan_type_hash.h

 
 /// \brief Check whether the dynamic type of \p Object has a \p Type subobject
 /// at offset 0.
 /// \return \c true if the type matches, \c false if not.
 bool checkDynamicType(void *Object, void *Type, HashValue Hash);
 
 const unsigned VptrTypeCacheSize = 128;
 
+/// A sanity check for Vtable. Offsets to top must be reasonably small
+/// numbers (by absolute value). It's a weak check for Vtable corruption.
+const int VptrMaxOffsetToTop = 1<<20;
+
 /// \brief A cache of the results of checkDynamicType. \c checkDynamicType would
 /// return \c true (modulo hash collisions) if
 /// \code
 ///   __ubsan_vptr_type_cache[Hash % VptrTypeCacheSize] == Hash
 /// \endcode
 extern "C" SANITIZER_INTERFACE_ATTRIBUTE
 HashValue __ubsan_vptr_type_cache[VptrTypeCacheSize];
 
 } // namespace __ubsan
 
 #endif // UBSAN_TYPE_HASH_H

lib/ubsan/ubsan_type_hash_itanium.cc

if (*Bucket == Hash) {
     __ubsan_vptr_type_cache[Hash % VptrTypeCacheSize] = Hash;
     return true;
   }
 
   void *VtablePtr = *reinterpret_cast<void **>(Object);
   VtablePrefix *Vtable = getVtablePrefix(VtablePtr);
   if (!Vtable)
     return false;
+  if (Vtable->Offset < -VptrMaxOffsetToTop || Vtable->Offset > VptrMaxOffsetToTop) {
+    // Too large or too small offset are signs of Vtable corruption.
+    return false;
+  }
 
   // Check that this is actually a type_info object for a class type.
   abi::__class_type_info *Derived =
     dynamic_cast<abi::__class_type_info*>(Vtable->TypeInfo);
   if (!Derived)
     return false;
 
   abi::__class_type_info *Base = (abi::__class_type_info*)Type;
   if (!isDerivedFromAtOffset(Derived, Base, -Vtable->Offset))
     return false;
 
   // Success. Cache this result.
   __ubsan_vptr_type_cache[Hash % VptrTypeCacheSize] = Hash;
   *Bucket = Hash;
   return true;
 }
 
 __ubsan::DynamicTypeInfo
 __ubsan::getDynamicTypeInfoFromVtable(void *VtablePtr) {
   VtablePrefix *Vtable = getVtablePrefix(VtablePtr);
   if (!Vtable)
     return DynamicTypeInfo(0, 0, 0);
+  if (Vtable->Offset < -VptrMaxOffsetToTop || Vtable->Offset > VptrMaxOffsetToTop)
+    return DynamicTypeInfo(0, Vtable->Offset, 0);
   const abi::__class_type_info *ObjectType = findBaseAtOffset(
     static_cast<const abi::__class_type_info*>(Vtable->TypeInfo),
     -Vtable->Offset);
   return DynamicTypeInfo(Vtable->TypeInfo->__type_name, -Vtable->Offset,
                          ObjectType ? ObjectType->__type_name : "<unknown>");
 }
 
 #endif  // CAN_SANITIZE_UB && !SANITIZER_WINDOWS

test/ubsan/TestCases/TypeCheck/vptr-corrupted-vtable-itanium.cpp

+// RUN: %clangxx -frtti -fsanitize=vptr -fno-sanitize-recover=vptr -g %s -O3 -o %t
+// RUN: not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-CORRUPTED-VTABLE --strict-whitespace
+
+// UNSUPPORTED: win32
+// REQUIRES: stable-runtime, cxxabi
+#include <cstddef>
+
+#include <typeinfo>
+
+struct S {
+  S() {}
+  ~S() {}
+  virtual int v() { return 0; }
+};
+
+// See the proper definition in ubsan_type_hash_itanium.cc
+struct VtablePrefix {
+  signed long Offset;
+  std::type_info *TypeInfo;
+};
+
+int main(int argc, char **argv) {
+  // Test that we don't crash on corrupted vtable when
+  // offset is too large or too small.
+  S Obj;
+  void *Ptr = &Obj;
+  VtablePrefix* RealPrefix = reinterpret_cast<VtablePrefix*>(
+      *reinterpret_cast<void**>(Ptr)) - 1;
+
+  VtablePrefix Prefix[2];
+  Prefix[0].Offset = 1<<21; // Greater than VptrMaxOffset
+  Prefix[0].TypeInfo = RealPrefix->TypeInfo;
+
+  // Hack Vtable ptr for Obj.
+  *reinterpret_cast<void**>(Ptr) = static_cast<void*>(&Prefix[1]);
+
+  // CHECK-CORRUPTED-VTABLE: vptr-corrupted-vtable-itanium.cpp:[[@LINE+3]]:16: runtime error: member call on address [[PTR:0x[0-9a-f]*]] which does not point to an object of type 'S'
+  // CHECK-CORRUPTED-VTABLE-NEXT: [[PTR]]: note: object has a possibly invalid vptr: abs(offset to top) too big
+  S* Ptr2 = reinterpret_cast<S*>(Ptr);
+  return Ptr2->v();
+}