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 ↗

(On Diff #59319)

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

62 ↗

(On Diff #59319)

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 ↗

(On Diff #59433)

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 ↗

(On Diff #59433)

Okay, sounds reasonable enough.

62 ↗

(On Diff #59433)

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

[krasin]

nits

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 must be non-negative and
+/// no larger than this value. It's a weak check for Vtable corruption.
+const int VptrMaxOffset = 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 < -VptrMaxOffset || Vtable->Offset > VptrMaxOffset) {
+    // 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)
+  if (!Vtable || Vtable->Offset < -VptrMaxOffset ||
+      Vtable->Offset > VptrMaxOffset)
     return DynamicTypeInfo(0, 0, 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