[ubsan] Use pass_object_size info in bounds checks

[vsk]

Summary

Teach UBSan's bounds check to opportunistically use pass_object_size
information to check array accesses.

rdar://33272922

[george.burgess.iv]

Thanks for this!

It's interesting to me that these array-bound checks don't seem to use @llvm.objectsize in some form already. I can't find any notes about it grepping through git/source, so I'm happy with it.

lib/CodeGen/CGExpr.cpp

819

nit: would IgnoreParenImpCasts be better?

828

We should probably only do this if the argument to pass_object_size is 0 or 1. 2 and 3 give lower-bounds on size (and a default value of 0), which could result in false-positives.

(And please add a test that we don't do this for 2 or 3.)

[vsk]

Thanks for your feedback.

• Give up on 0-sized types.
• Give up on pass_object_size(2 | 3).

[vsk]

• Handle constant size modifiers while we're at it (e.g "int foo(int p[static 10])").

[george.burgess.iv]

LGTM. Thanks again!

[vsk]

Landed in r320128.

[efriedma]

It's interesting to me that these array-bound checks don't seem to use @llvm.objectsize in some form already.

That would be a cool experiment. That said, one of the upsides of the current ubsan is that whether it will produce a diagnostic is predictable (as long as you don't use uninitialized data); you lose that to some extent with llvm.objectsize because it depends on the optimizer.

lib/CodeGen/CGExpr.cpp

833

"int f(int a[10])" might look like an array, but it isn't: it's just a different syntax to declare a pointer. So it's legal to "lie" in the signature. (If you want to actually pass a pointer to an array, you have to write "int (*a)[10]".) And the definition of "static" says "an array with at least as many elements as specified by the size expression", which isn't a maximum, so that doesn't really help either.

Most people would consider it bad style to put a number into the array bound which doesn't reflect reality, but I think we shouldn't try to check it unless the user explicitly requests it.

[vsk]

I backed out the part of this patch which deals with array parameters declared like p[10] or p[static 10]: r320185.

lib/CodeGen/CGExpr.cpp

833

My copy of the C99 draft (n1256) is a little fuzzy on this point [*]. There's enough of a gray area here that it seems appropriate to back out this part of the patch.

• It states: "In addition to optional type qualifiers and the keyword static, the [ and ] may delimit an expression or *. If they delimit an expression (which specifies the size of an array) ...". I took the parenthetical literally, and didn't know about the 'at least as many' language.

[george.burgess.iv]

That said, one of the upsides of the current ubsan is that whether it will produce a diagnostic is predictable (as long as you don't use uninitialized data); you lose that to some extent with llvm.objectsize because it depends on the optimizer.

True. If that's not desirable to have in array-bounds, we could potentially move these checks under -fsanitize=object-size instead. We'd just have to be careful about not emitting object-size and array-bounds checks for the same array access.

[efriedma]

lib/CodeGen/CGExpr.cpp

833

"which specifies the size of an array" in this context just means that it's the part of the array declarator which specifies the size of the array, not that it's the size of any particular array. The decay rules for function parameters are the relevant bit of the standard.

And yes, this is all very confusing; this is why C++ has std::array.

lib/CodeGen/CGExpr.cpp

   return false;
 }
 
+llvm::Value *CodeGenFunction::LoadPassedObjectSize(const Expr *E,
+                                                   QualType EltTy) {
+  ASTContext &C = getContext();
+  uint64_t EltSize = C.getTypeSizeInChars(EltTy).getQuantity();
+  if (!EltSize)
+    return nullptr;
+
+  auto *ArrayDeclRef = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts());
+  if (!ArrayDeclRef)
+    return nullptr;
+
+  auto *ParamDecl = dyn_cast<ParmVarDecl>(ArrayDeclRef->getDecl());
+  if (!ParamDecl)
+    return nullptr;
+
+  // Arrays don't have pass_object_size attributes, but if they have a constant
+  // size modifier it's the array size (C99 6.5.7.2p1).
+  if (auto *DecayedArrayTy = dyn_cast<DecayedType>(ParamDecl->getType()))
+    if (auto *ArrayTy =
+            dyn_cast<ConstantArrayType>(DecayedArrayTy->getOriginalType()))
+      return llvm::ConstantInt::get(SizeTy,
+                                    ArrayTy->getSize().getLimitedValue());
+
+  auto *POSAttr = ParamDecl->getAttr<PassObjectSizeAttr>();
+  if (!POSAttr)
+    return nullptr;
+
+  // Don't load the size if it's a lower bound.
+  int POSType = POSAttr->getType();
+  if (POSType != 0 && POSType != 1)
+    return nullptr;
+
+  // Find the implicit size parameter.
+  auto PassedSizeIt = SizeArguments.find(ParamDecl);
+  if (PassedSizeIt == SizeArguments.end())
+    return nullptr;
+
+  const ImplicitParamDecl *PassedSizeDecl = PassedSizeIt->second;
+  assert(LocalDeclMap.count(PassedSizeDecl) && "Passed size not loadable");
+  Address AddrOfSize = LocalDeclMap.find(PassedSizeDecl)->second;
+  llvm::Value *SizeInBytes = EmitLoadOfScalar(AddrOfSize, /*Volatile=*/false,
+                                              C.getSizeType(), E->getExprLoc());
+  llvm::Value *SizeOfElement =
+      llvm::ConstantInt::get(SizeInBytes->getType(), EltSize);
+  return Builder.CreateUDiv(SizeInBytes, SizeOfElement);
+}
+
 /// 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(

         return CGF.Builder.getInt(CAT->getSize());
       else if (const auto *VAT = dyn_cast<VariableArrayType>(AT))
         return CGF.getVLASize(VAT).first;
+      // Ignore pass_object_size here. It's not applicable on decayed pointers.
     }
   }
 
+  QualType EltTy{Base->getType()->getPointeeOrArrayElementType(), 0};
+  if (llvm::Value *POS = CGF.LoadPassedObjectSize(Base, EltTy)) {
+    IndexedType = Base->getType();
+    return POS;
+  }
+
   return nullptr;
 }
 

lib/CodeGen/CodeGenFunction.h

                                    LValueBaseInfo *BaseInfo = nullptr,
                                    TBAAAccessInfo *TBAAInfo = nullptr);
 
+  /// If \p E references a parameter with pass_object_size info or a constant
+  /// array size modifier, emit the object size divided by the size of \p EltTy.
+  /// Otherwise return null.
+  llvm::Value *LoadPassedObjectSize(const Expr *E, QualType EltTy);
+
   void EmitSanitizerStatReport(llvm::SanitizerStatKind SSK);
 
 private:

test/CodeGen/ubsan-pass-object-size.c

+// RUN: %clang_cc1 %s -emit-llvm -w -triple x86_64-apple-darwin10 -fsanitize=array-bounds -o - | FileCheck %s
+
+// CHECK-LABEL: define i32 @foo(
+int foo(int *const p __attribute__((pass_object_size(0))), int n) {
+  int x = (p)[n];
+
+  // CHECK: [[SIZE_ALLOCA:%.*]] = alloca i64, align 8
+  // CHECK: store i64 %{{.*}}, i64* [[SIZE_ALLOCA]], align 8
+  // CHECK: [[LOAD_SIZE:%.*]] = load i64, i64* [[SIZE_ALLOCA]], align 8, !nosanitize
+  // CHECK-NEXT: [[SCALED_SIZE:%.*]] = udiv i64 [[LOAD_SIZE]], 4, !nosanitize
+  // CHECK-NEXT: [[SEXT_N:%.*]] = sext i32 %{{.*}} to i64, !nosanitize
+  // CHECK-NEXT: [[ICMP:%.*]] = icmp ult i64 [[SEXT_N]], [[SCALED_SIZE]], !nosanitize
+  // CHECK-NEXT: br i1 [[ICMP]], {{.*}} !nosanitize
+  // CHECK: __ubsan_handle_out_of_bounds
+
+  {
+    int **p = &p; // Shadow the parameter. The pass_object_size info is lost.
+    // CHECK-NOT: __ubsan_handle_out_of_bounds
+    x = *p[n];
+  }
+
+  // CHECK: ret i32
+  return x;
+}
+
+typedef struct {} ZeroSizedType;
+
+// CHECK-LABEL: define void @bar(
+ZeroSizedType bar(ZeroSizedType *const p __attribute__((pass_object_size(0))), int n) {
+  // CHECK-NOT: __ubsan_handle_out_of_bounds
+  // CHECK: ret void
+  return p[n];
+}
+
+// CHECK-LABEL: define i32 @baz(
+int baz(int *const p __attribute__((pass_object_size(1))), int n) {
+  // CHECK: __ubsan_handle_out_of_bounds
+  // CHECK: ret i32
+  return p[n];
+}
+
+// CHECK-LABEL: define i32 @mat(
+int mat(int *const p __attribute__((pass_object_size(2))), int n) {
+  // CHECK-NOT: __ubsan_handle_out_of_bounds
+  // CHECK: ret i32
+  return p[n];
+}
+
+// CHECK-LABEL: define i32 @pat(
+int pat(int *const p __attribute__((pass_object_size(3))), int n) {
+  // CHECK-NOT: __ubsan_handle_out_of_bounds
+  // CHECK: ret i32
+  return p[n];
+}
+
+// CHECK-LABEL: define i32 @cat(
+int cat(int p[static 10], int n) {
+  // CHECK: icmp ult i64 {{.*}}, 10, !nosanitize
+  // CHECK: __ubsan_handle_out_of_bounds
+  // CHECK: ret i32
+  return p[n];
+}
+
+// CHECK-LABEL: define i32 @bat(
+int bat(int n, int p[n]) {
+  // CHECK-NOT: __ubsan_handle_out_of_bounds
+  // CHECK: ret i32
+  return p[n];
+}