[ubsan] Disable bounds-check for flexible array ivars

[vsk]

Summary

Ubsan does not emit a bounds check when accessing flexible array members in C-style structs, e.g:

struct Foo { char arr[0]; };
char bar(struct Foo *F) { return F->arr[1]; } //< Unchecked access.

Teach ubsan to skip the bounds check for flexible array ObjC ivars as well.

This reduces the false-positive rate when instrumenting Objective-C frameworks.

A note on testing: it looks like runnable ubsan tests are typically added to compiler-rt. I chose to create a test in clang instead, because I don't want to make the ubsan tests depend on libobjc.

[vsk]

Ping.

[vsk]

Ping.

[vsk]

Ping.

[vsk]

Ping.

[vsk]

Ping.

[vsk]

Ping?

[aprantl]

Not knowing the specifics of the ObjC class layout: Does this work correctly with private ivars (i.e.: Does this need an extra check that there are no extra ivars in a later @implementation)?

[vsk]

The ivar list is set by all_declared_ivar_begin(), which accounts for ivars introduced by an implementation. Thanks for raising the point.

Consider this test:

@interface HasFlexibleArray {
@public char chars[0];
}
@implementation HasFlexibleArray {
@public char chars2[0];
}

I found that the current patch will sanitize accesses to 'chars', but will not sanitize accesses to 'chars2'. I think that's the desired behavior -- wdyt?

[vsk]

Add tests for implementations which introduce ivars.

[aprantl]

Can you double-check that the memory layout is actually what we think it is by inspecting the generated IR? Otherwise this LGTM.

[vsk]

Thanks for the review!

I looked at the IR and confirmed that the ivars are laid out in the order they're defined, that the indirect ivar offsets make sense, and that the runtime ivar offsets match up with what we expect. E.g;

@"OBJC_IVAR_$_FlexibleArray1.chars" = global i64 0 ...
@"\01l_OBJC_$_INSTANCE_VARIABLES_FlexibleArray1" = private global { i32, i32, [1 x %struct._ivar_t] } ...

---

@"OBJC_IVAR_$_FlexibleArray2.chars" = global i64 0
@"OBJC_IVAR_$_FlexibleArray2.chars2" = global i64 0 ;; < This offset increases if we sandwich an int between chars and chars2.
@"\01l_OBJC_$_INSTANCE_VARIABLES_FlexibleArray2" = private global { i32, i32, [2 x %struct._ivar_t] } ...

cfe/trunk/lib/CodeGen/CGExpr.cpp

static bool isFlexibleArrayMemberExpr(const Expr *E) {
   if (const auto *ME = dyn_cast<MemberExpr>(E)) {
     // FIXME: If the base type of the member expr is not FD->getParent(),
     // this should not be treated as a flexible array member access.
     if (const auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl())) {
       RecordDecl::field_iterator FI(
           DeclContext::decl_iterator(const_cast<FieldDecl *>(FD)));
       return ++FI == FD->getParent()->field_end();
     }
+  } else if (const auto *IRE = dyn_cast<ObjCIvarRefExpr>(E)) {
+    return IRE->getDecl()->getNextIvar() == nullptr;
   }
 
   return false;
 }
 
 /// If Base is known to point to the start of an array, return the length of
 /// that array. Return 0 if the length cannot be determined.
 static llvm::Value *getArrayIndexingBound(

cfe/trunk/test/CodeGenObjC/ubsan-array-bounds.m

+// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -Wno-objc-root-class -fsanitize=array-bounds %s -o - | FileCheck %s
+
+@interface FlexibleArray1 {
+@public
+  char chars[0];
+}
+@end
+@implementation FlexibleArray1
+@end
+
+// CHECK-LABEL: test_FlexibleArray1
+char test_FlexibleArray1(FlexibleArray1 *FA1) {
+  // CHECK-NOT: !nosanitize
+  return FA1->chars[1];
+  // CHECK: }
+}
+
+@interface FlexibleArray2 {
+@public
+  char chars[0];
+}
+@end
+@implementation FlexibleArray2 {
+@public
+  char chars2[0];
+}
+@end
+
+// CHECK-LABEL: test_FlexibleArray2_1
+char test_FlexibleArray2_1(FlexibleArray2 *FA2) {
+  // CHECK: !nosanitize
+  return FA2->chars[1];
+  // CHECK: }
+}
+
+// CHECK-LABEL: test_FlexibleArray2_2
+char test_FlexibleArray2_2(FlexibleArray2 *FA2) {
+  // CHECK-NOT: !nosanitize
+  return FA2->chars2[1];
+  // CHECK: }
+}
+
+@interface FlexibleArray3 {
+@public
+  char chars[0];
+}
+@end
+@implementation FlexibleArray3 {
+@public
+  int i;
+}
+@end
+
+// CHECK-LABEL: test_FlexibleArray3
+char test_FlexibleArray3(FlexibleArray3 *FA3) {
+  // CHECK: !nosanitize
+  return FA3->chars[1];
+  // CHECK: }
+}