[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 ↗

(On Diff #80856)

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 ↗

(On Diff #80856)

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 ↗

(On Diff #80856)

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 ↗

(On Diff #80856)

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 ↗

(On Diff #80856)

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

1224 ↗

(On Diff #80856)

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!

cfe/trunk/lib/CodeGen/CGExpr.cpp

 #include "CGOpenMPRuntime.h"
 #include "CGRecordLayout.h"
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/NSAPI.h"
 #include "clang/Frontend/CodeGenOptions.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/Support/ConvertUTF.h"
static bool hasBooleanRepresentation(QualType Ty) {
   if (const AtomicType *AT = Ty->getAs<AtomicType>())
     return hasBooleanRepresentation(AT->getValueType());
 
   return false;
 }
 
 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();
if (NumNegativeBits) {
       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,
llvm::Value *CodeGenFunction::EmitLoadOfScalar(Address Addr, bool Volatile,
   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) ||
+                NSAPI(CGM.getContext()).isObjCBOOLType(Ty);
+  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));

cfe/trunk/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
+
+typedef signed char BOOL;
+
+// 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;
+}