[ubsan] Fix IsAccessibleMemoryRange call for VtablePrefix to prevent SIGSEGV.

[filcab]

Test case?

[Dor1s]

In D34215#780539, @filcab wrote:

Test case?

I would say this is a follow-up for https://reviews.llvm.org/D33712, but let me see if I can build another testcase...

[vsk]

Lgtm with some more testing. You could modify PR33221.cpp. Something like this?

char *c = new char[16 + sizeof(Derived)];
memset((void *)c, 0, 16 + sizeof(Derived));
Derived *list = (Derived *)(c + 16);
int foo = list->i;

[Dor1s]

It revealed to be a bit more complicated, so there are two tests now.

1. I fixed PR33221.cpp in a way that it fails with old revisions (i.e. before https://reviews.llvm.org/D33712), but works for any other revision since then. Otherwise, the test didn't make sense, as UBSan has been reporting an error even with older revisions due to the check of Vptr against NULL value.

2. I added another test for this particular CL. The idea of the test is to reproduce a case when VptrPrefix is pointing to a non accessible memory, while Vptr points to an accessible one. I haven't found a better way rather then allocate two sequential memory pages, so this extra test is Linux only.

[Dor1s]

Remove a typo.

[Dor1s]

Fix formatting

[vsk]

Lgtm with minor changes.

test/ubsan/TestCases/TypeCheck/Linux/PR33221.cpp

25

0 -> nullptr

26

Could you bail out with 'return 0' when the mmap fails?

30

Linux doesn't guarantee that 'accessible' will be at non_accessible + page_size, so bail out if it doesn't? Also bail out if the mmap fails.

[Dor1s]

Address comments

test/ubsan/TestCases/TypeCheck/Linux/PR33221.cpp

30

With MAP_FIXED it should fail if the specified address cannot be used, so bail out in case of a failure seems to be enough. Thanks for the review!

[vsk]

test/ubsan/TestCases/TypeCheck/Linux/PR33221.cpp

3

One last thing. FileCheck will fail if either of the mmap's do. Could you pipe "%t &> %t.log", then in a separate RUN line, run FileCheck if the log is non-empty? Something like "cat %t.log | not count 0 && FileCheck ...".

[Dor1s]

Now the test also works if any of the mmap calls failed.

test/ubsan/TestCases/TypeCheck/Linux/PR33221.cpp

3

Oh, right! Huge thanks for the hint on how to handle that!

[vsk]

LGTM, thanks for fixing this. Aside: FileCheck's has an "-input-file" option which can be useful in situations like this.

[Dor1s]

Good to know, it's useful indeed! May I ask you please to land this change as I'm not a committer yet? I'd greatly appreciate if you have a couple of minutes to do that :)

[eugenis]

test/ubsan/TestCases/TypeCheck/Linux/PR33221.cpp

30

MAP_FIXED

[...]
If the memory region specified by addr and len overlaps pages of any existing mapping(s), then the overlapped part of the existing mapping(s) will be discarded.
[...]

I've been unlucky enough for this to overlap AsanThread for the main thread, and has been staring at null pointers for a while.

[eugenis]

test/ubsan/TestCases/TypeCheck/Linux/PR33221.cpp

30

Fixed in r315247.

[Dor1s]

test/ubsan/TestCases/TypeCheck/Linux/PR33221.cpp

30

Wow! Thanks for the info and for the fix!

lib/ubsan/ubsan_type_hash_itanium.cc

struct VtablePrefix {
   /// 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;
 };
 VtablePrefix *getVtablePrefix(void *Vtable) {
   VtablePrefix *Vptr = reinterpret_cast<VtablePrefix*>(Vtable);
-  if (!IsAccessibleMemoryRange((uptr)Vptr, sizeof(VtablePrefix)))
-    return nullptr;
   VtablePrefix *Prefix = Vptr - 1;
+  if (!IsAccessibleMemoryRange((uptr)Prefix, sizeof(VtablePrefix)))
+    return nullptr;
   if (!Prefix->TypeInfo)
     // This can't possibly be a valid vtable.
     return nullptr;
   return Prefix;
 }
 
 }
 

test/ubsan/TestCases/TypeCheck/Linux/PR33221.cpp

+// RUN: %clangxx -std=c++11 -frtti -fsanitize=vptr -g %s -O3 -o %t
+// RUN: %run %t &> %t.log
+// RUN: cat %t.log | not count 0 && FileCheck --input-file %t.log %s || cat %t.log | count 0
+
+// REQUIRES: cxxabi
+
+#include <sys/mman.h>
+#include <unistd.h>
+
+class Base {
+public:
+  int i;
+  virtual void print() {}
+};
+
+class Derived : public Base {
+public:
+  void print() {}
+};
+
+
+int main() {
+  int page_size = getpagesize();
+
+  void *non_accessible = mmap(nullptr, page_size, PROT_NONE,
+                              MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  
+  if (non_accessible == MAP_FAILED)
+    return 0;
+
+  void *accessible = mmap((char*)non_accessible + page_size, page_size,
+                          PROT_READ | PROT_WRITE,
+                          MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (accessible == MAP_FAILED)
+    return 0;
+
+  char *c = new char[sizeof(Derived)];
+
+  // The goal is to trigger a condition when Vptr points to accessible memory,
+  // but VptrPrefix does not. That has been triggering SIGSEGV in UBSan code.
+  void **vtable_ptr = reinterpret_cast<void **>(c);
+  *vtable_ptr = (void*)accessible;
+
+  Derived *list = (Derived *)c;
+
+// CHECK: PR33221.cpp:[[@LINE+2]]:19: runtime error: member access within address {{.*}} which does not point to an object of type 'Base'
+// CHECK-NEXT: invalid vptr
+  int foo = list->i;
+  return 0;
+}

test/ubsan/TestCases/TypeCheck/Linux/lit.local.cfg

+def getRoot(config):
+  if not config.parent:
+    return config
+  return getRoot(config.parent)
+
+root = getRoot(config)
+
+if root.host_os not in ['Linux']:
+  config.unsupported = True

test/ubsan/TestCases/TypeCheck/PR33221.cpp

 
 class Derived : public Base {
 public:
   void print() {}
 };
 
 int main() {
   char *c = new char[sizeof(Derived)];
-  memset((void *)c, 0, sizeof(Derived));
+  memset((void *)c, 0xFF, sizeof(Derived));
   Derived *list = (Derived *)c;
 
 // CHECK: PR33221.cpp:[[@LINE+2]]:19: runtime error: member access within address {{.*}} which does not point to an object of type 'Base'
 // CHECK-NEXT: invalid vptr
   int foo = list->i;
   return 0;
 }