[ubsan] Treat ObjC's BOOL as if its range is always {0, 1}

[vsk]

Summary

On some Apple platforms, the ObjC BOOL type is defined as a signed char.
When performing instrumentation for -fsanitize=bool, we'd like to treat
the range of BOOL like it's always {0, 1}. While we can't change clang's
IRGen for char-backed BOOL's due to ABI compatibility concerns, we can
teach ubsan to catch potential abuses of this type.

rdar://problem/29502773

[arphaman]

lib/CodeGen/CGExpr.cpp

1224

LangOptions.ObjC1 should be always set if LangOptions.ObjC2 is set, so the second check is redundant I think.

[zaks.anna]

lib/CodeGen/CGExpr.cpp

1222

Could you use the existing method for this? From NSAPI.h:

// \brief Returns true if \param T is a typedef of "BOOL" in objective-c.
bool isObjCBOOLType(QualType T) const;

[ahatanak]

lib/CodeGen/CGExpr.cpp

1221

If your intention is to exclude BOOLs defined in files that aren't system headers, is it possible to add a test for that?

void foo() {
  typedef long long BOOL;
  ...
}

[vsk]

Thanks for your feedback. I will updated the patch shortly.

lib/CodeGen/CGExpr.cpp

1221

Thinking about this some more, I think it's enough to check for an ObjC language mode, and that the system header check is unnecessary.

1222

Thanks, this is a lot better than rolling my own.

1224

Using NSAPI takes care of this.

[vsk]

• Use NSAPI's 'is-BOOL' predicate.
• Simplify test.

[zaks.anna]

LGTM!

[fjricci]

On platforms where BOOL == signed char, is it actually undefined behavior (or is it just bad programming practice) to store a value other than 0 or 1 in your BOOL? I can't find any language specs suggesting that it is, and given that it's just a typedef for a signed char, I don't see why it would be.

If it's not actually undefined behavior, could we make it controllable via a separate fsanitize switch (like we have for unsigned integer overflow, which is also potentially bad practice but not actually undefined behavior).

[vsk]

In D27607#901882, @fjricci wrote:

On platforms where BOOL == signed char, is it actually undefined behavior (or is it just bad programming practice) to store a value other than 0 or 1 in your BOOL? I can't find any language specs suggesting that it is, and given that it's just a typedef for a signed char, I don't see why it would be.

Treating BOOL as a regular 'signed char' creates bad interactions with bitfields. For example, this code calls panic:

struct S {
  BOOL b1 : 1;
};

S s;
s.b1 = YES;
if (s.b1 != YES)
  panic();

If it's not actually undefined behavior, could we make it controllable via a separate fsanitize switch (like we have for unsigned integer overflow, which is also potentially bad practice but not actually undefined behavior).

The only defined values for BOOL are 'YES' and 'NO' {1, 0}. We've documented that it's UB to load values outside of this range from a BOOL here:
https://developer.apple.com/documentation/code_diagnostics/undefined_behavior_sanitizer/invalid_boolean

[fjricci]

Awesome, good to know. Thanks!

lib/CodeGen/CGExpr.cpp

     return ET->getDecl()->getIntegerType()->isBooleanType();
 
   if (const AtomicType *AT = Ty->getAs<AtomicType>())
     return hasBooleanRepresentation(AT->getValueType());
 
   return false;
 }
 
+/// Check if \p Ty is defined as BOOL in a system header. In ObjC language
+/// modes, it's safe to treat such a type as 'the builtin bool'.
+static bool isObjCBool(QualType Ty, const SourceManager &SM,
+                       const LangOptions &LO) {
+  if (!LO.ObjC1 && !LO.ObjC2)
+    return false;
+
+  const auto *TT = Ty.getTypePtr()->getAs<TypedefType>();
+  if (!TT)
+    return false;
+
+  const TypedefNameDecl *TND = TT->getDecl();
+  return TND->getName() == "BOOL" &&
+         SM.isInSystemHeader(
+             TND->getTypeSourceInfo()->getTypeLoc().getBeginLoc());
+}
+
 static bool getRangeForType(CodeGenFunction &CGF, QualType Ty,
                             llvm::APInt &Min, llvm::APInt &End,
-                            bool StrictEnums) {
+                            bool StrictEnums, bool IsBool) {
   const EnumType *ET = Ty->getAs<EnumType>();
   bool IsRegularCPlusPlusEnum = CGF.getLangOpts().CPlusPlus && StrictEnums &&
                                 ET && !ET->getDecl()->isFixed();
-  bool IsBool = hasBooleanRepresentation(Ty);
   if (!IsBool && !IsRegularCPlusPlusEnum)
     return false;
 
   if (IsBool) {
     Min = llvm::APInt(CGF.getContext().getTypeSize(Ty), 0);
     End = llvm::APInt(CGF.getContext().getTypeSize(Ty), 2);
   } else {
     const EnumDecl *ED = ET->getDecl();

       Min = llvm::APInt(Bitwidth, 0);
     }
   }
   return true;
 }
 
 llvm::MDNode *CodeGenFunction::getRangeForLoadFromType(QualType Ty) {
   llvm::APInt Min, End;
-  if (!getRangeForType(*this, Ty, Min, End,
-                       CGM.getCodeGenOpts().StrictEnums))
+  if (!getRangeForType(*this, Ty, Min, End, CGM.getCodeGenOpts().StrictEnums,
+                       hasBooleanRepresentation(Ty)))
     return nullptr;
 
   llvm::MDBuilder MDHelper(getLLVMContext());
   return MDHelper.createRange(Min, End);
 }
 
 llvm::Value *CodeGenFunction::EmitLoadOfScalar(Address Addr, bool Volatile,
                                                QualType Ty,

   if (TBAAInfo) {
     llvm::MDNode *TBAAPath = CGM.getTBAAStructTagInfo(TBAABaseType, TBAAInfo,
                                                       TBAAOffset);
     if (TBAAPath)
       CGM.DecorateInstructionWithTBAA(Load, TBAAPath,
                                       false /*ConvertTypeToTag*/);
   }
 
-  bool NeedsBoolCheck =
-      SanOpts.has(SanitizerKind::Bool) && hasBooleanRepresentation(Ty);
+  bool IsBool =
+      hasBooleanRepresentation(Ty) ||
+      isObjCBool(Ty, CGM.getContext().getSourceManager(), CGM.getLangOpts());
+  bool NeedsBoolCheck = SanOpts.has(SanitizerKind::Bool) && IsBool;
   bool NeedsEnumCheck =
       SanOpts.has(SanitizerKind::Enum) && Ty->getAs<EnumType>();
   if (NeedsBoolCheck || NeedsEnumCheck) {
     SanitizerScope SanScope(this);
     llvm::APInt Min, End;
-    if (getRangeForType(*this, Ty, Min, End, true)) {
+    if (getRangeForType(*this, Ty, Min, End, /*StrictEnums=*/true, IsBool)) {
       --End;
       llvm::Value *Check;
       if (!Min)
         Check = Builder.CreateICmpULE(
           Load, llvm::ConstantInt::get(getLLVMContext(), End));
       else {
         llvm::Value *Upper = Builder.CreateICmpSLE(
           Load, llvm::ConstantInt::get(getLLVMContext(), End));

test/CodeGenObjC/ubsan-bool.m

+// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC
+// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC
+// RUN: %clang_cc1 -x c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,C
+
+#ifdef IS_SYSHEADER
+
+// Create a system definition of ObjC's BOOL.
+#pragma clang system_header
+typedef signed char BOOL;
+
+#else
+
+// Import an official-looking definition of BOOL.
+#define IS_SYSHEADER
+#include __FILE__
+
+// SHARED-LABEL: f1
+BOOL f1() {
+  // OBJC: call void @__ubsan_handle_load_invalid_value
+  // C-NOT: call void @__ubsan_handle_load_invalid_value
+  BOOL a = 2;
+  return a + 1;
+}
+
+#endif // IS_SYSHEADER