[ubsan] Detect UB loads from bitfields

[vsk]

Summary

We frequently run into user bugs caused by UB loads of out-of-range values from enum / BOOL bitfields. Teach UBSan to diagnose the issue.

[arphaman]

test/CodeGenCXX/ubsan-bitfields.cpp

21 ↗

(On Diff #89914)

Can we avoid the check if the bitfield is 2 bits wide?

test/CodeGenObjC/ubsan-bool.m

25 ↗

(On Diff #89914)

One unrelated thing that I noticed in the IR, the zexts to i64 are emitted before the branch, even though they are used only in the invalid_value blocks. I know that the optimizer can move those anyway, but would there any be any benefit in moving them into the blocks at the frontend IRGen level?

26 ↗

(On Diff #89914)

Is it possible to avoid the check here since the bitfield is just one bit wide?

[vsk]

Thanks for the feedback!

test/CodeGenCXX/ubsan-bitfields.cpp

21 ↗

(On Diff #89914)

I don't think so, because if the user memset()'s the memory backing the struct, we could still load a value outside of {1, 2, 3} from the bitfield.

test/CodeGenObjC/ubsan-bool.m

25 ↗

(On Diff #89914)

I'm not sure. Maybe it would make the live range of the zext smaller at -O0? I'll look into this and see if there's a real perf impact.

26 ↗

(On Diff #89914)

No, a user could do:

struct S1 s;
s.b1 = 1;
if (s.b1 == 1) // evaluates to false, s.b1 is negative

With this patch we get:

runtime error: load of value -1, which is not a valid value for type 'BOOL' (aka 'signed char')

[jroelofs]

I think this might miss loads from bitfield ivars. Also, what about the conversion that happens for properties whose backing ivar is a bitfield? (or does that happen in the runtime? can't remember)

test/CodeGenObjC/ubsan-bool.m

25 ↗

(On Diff #89914)

@arphaman if it can be done easily/eleganly during IRGen, then this is a compile-duration win.

26 ↗

(On Diff #89914)

What if BOOL is an unsigned char, or a char that's not signed?

[vsk]

In D30423#694003, @jroelofs wrote:

I think this might miss loads from bitfield ivars.

I'll add a test that shows that this case is covered.

Also, what about the conversion that happens for properties whose backing ivar is a bitfield? (or does that happen in the runtime? can't remember)

I'll also check that synthesized getters have the range check.

test/CodeGenObjC/ubsan-bool.m

26 ↗

(On Diff #89914)

Good point, we don't need to emit the range check if the bitfield is an unsigned, 1-bit wide BOOL. Would you be OK with me tackling that in a follow-up patch?

[vsk]

• Improve objc test coverage per Jon's suggestions.

[vsk]

test/CodeGenCXX/ubsan-bitfields.cpp

21 ↗

(On Diff #89914)

Sorry, I misread your question as 'if the enum can be represented in 2 bits'. You're right, the check can be skipped if the _bitfield_ is 2 bits wide. I think this can be handled along with the 1-bit unsigned BOOL case in a follow up.

[jroelofs]

LGTM

test/CodeGenObjC/ubsan-bool.m

50 ↗

(On Diff #90869)

hmmm. just occurred to me that this value is always going to be 0xff or 0x0, so it might be useful if there were also a frontend warning that complains about signed bitfield bools (maybe something useful for another patch).

26 ↗

(On Diff #89914)

No problem, sounds good to me.

[filcab]

test/CodeGenObjC/ubsan-bool.m

50 ↗

(On Diff #90869)

I'm more ok with this, assuming it makes sense in ObjC.

26 ↗

(On Diff #89914)

I don't like generating an error here.
-1 is a perfectly valid BOOL (signed char) value and it's a perfectly valid value to have in a (signed) 1-wide bitfield.

[vsk]

test/CodeGenObjC/ubsan-bool.m

50 ↗

(On Diff #90869)

Yes, we have an internal feature request for such a warning.

26 ↗

(On Diff #89914)

True/false should be the only values loaded from a boolean. Since we made ubsan stricter about the range of BOOL (r289290), the check has caught a lot of bugs in our software. Specifically, it's helped root out buggy BOOL initialization and portability problems (the signedness of BOOL is not consistent on Apple platforms). There have been no false positives so far.

Is this an issue you think needs to be addressed in this patch?

[filcab]

LGTM since my issue is only an issue on ObjC platforms and it seems those are the semantics it should have there.

test/CodeGenObjC/ubsan-bool.m

26 ↗

(On Diff #89914)

It looks weird that we would disallow using a 1-bit (signed) bitfield for storing BOOL. But since this will only be a problem on obj-c, I don't think it will be a problem in general. If you're saying those are the semantics, then I'm ok with it.
P.S: If it documented that the only possible values for BOOL are YES and NO (assuming they are 1 and 0)?
If so, I wonder if it's planned to document that you can't store a YES on a BOOL bitfield with a width of 1, since it looks like a weird case.

[vsk]

In D30423#695608, @filcab wrote:

LGTM since my issue is only an issue on ObjC platforms and it seems those are the semantics it should have there.

Thanks for the review.

P.S: If it documented that the only possible values for BOOL are YES and NO (assuming they are 1 and 0)?

I think this more or less counts: https://developer.apple.com/reference/objectivec/objective_c_runtime/boolean_values

If so, I wonder if it's planned to document that you can't store a YES on a BOOL bitfield with a width of 1, since it looks like a weird case.

I will ask around. There is a warning about signedness issues here: https://developer.apple.com/reference/objectivec/bool?language=objc

cfe/trunk/lib/CodeGen/CGAtomic.cpp

if (LVal.isSimple()) {
     return CGF.convertTempToRValue(addr, getValueType(), loc);
   }
   if (!asValue)
     // Get RValue from temp memory as atomic for non-simple lvalues
     return RValue::get(CGF.Builder.CreateLoad(addr));
   if (LVal.isBitField())
     return CGF.EmitLoadOfBitfieldLValue(
         LValue::MakeBitfield(addr, LVal.getBitFieldInfo(), LVal.getType(),
-                             LVal.getAlignmentSource()));
+                             LVal.getAlignmentSource()), loc);
   if (LVal.isVectorElt())
     return CGF.EmitLoadOfLValue(
         LValue::MakeVectorElt(addr, LVal.getVectorIdx(), LVal.getType(),
                               LVal.getAlignmentSource()), loc);
   assert(LVal.isExtVectorElt());
   return CGF.EmitLoadOfExtVectorElementLValue(LValue::MakeExtVectorElt(
       addr, LVal.getExtVectorElts(), LVal.getType(),
       LVal.getAlignmentSource()));

cfe/trunk/lib/CodeGen/CGExpr.cpp

RValue CodeGenFunction::EmitLoadOfLValue(LValue LV, SourceLocation Loc) {
   if (LV.isExtVectorElt())
     return EmitLoadOfExtVectorElementLValue(LV);
 
   // Global Register variables always invoke intrinsics
   if (LV.isGlobalReg())
     return EmitLoadOfGlobalRegLValue(LV);
 
   assert(LV.isBitField() && "Unknown LValue type!");
-  return EmitLoadOfBitfieldLValue(LV);
+  return EmitLoadOfBitfieldLValue(LV, Loc);
 }
 
-RValue CodeGenFunction::EmitLoadOfBitfieldLValue(LValue LV) {
+RValue CodeGenFunction::EmitLoadOfBitfieldLValue(LValue LV,
+                                                 SourceLocation Loc) {
   const CGBitFieldInfo &Info = LV.getBitFieldInfo();
 
   // Get the output type.
   llvm::Type *ResLTy = ConvertType(LV.getType());
 
   Address Ptr = LV.getBitFieldAddress();
   llvm::Value *Val = Builder.CreateLoad(Ptr, LV.isVolatileQualified(), "bf.load");
 
   if (Info.IsSigned) {
     assert(static_cast<unsigned>(Info.Offset + Info.Size) <= Info.StorageSize);
     unsigned HighBits = Info.StorageSize - Info.Offset - Info.Size;
     if (HighBits)
       Val = Builder.CreateShl(Val, HighBits, "bf.shl");
     if (Info.Offset + HighBits)
       Val = Builder.CreateAShr(Val, Info.Offset + HighBits, "bf.ashr");
   } else {
     if (Info.Offset)
       Val = Builder.CreateLShr(Val, Info.Offset, "bf.lshr");
     if (static_cast<unsigned>(Info.Offset) + Info.Size < Info.StorageSize)
       Val = Builder.CreateAnd(Val, llvm::APInt::getLowBitsSet(Info.StorageSize,
                                                               Info.Size),
                               "bf.clear");
   }
   Val = Builder.CreateIntCast(Val, ResLTy, Info.IsSigned, "bf.cast");
-
+  EmitScalarRangeCheck(Val, LV.getType(), Loc);
   return RValue::get(Val);
 }
 
 // If this is a reference to a subset of the elements of a vector, create an
 // appropriate shufflevector.
 RValue CodeGenFunction::EmitLoadOfExtVectorElementLValue(LValue LV) {
   llvm::Value *Vec = Builder.CreateLoad(LV.getExtVectorAddress(),
                                         LV.isVolatileQualified());

cfe/trunk/lib/CodeGen/CodeGenFunction.h

public:
   /// If so, atomic qualifiers are ignored and the store is always non-atomic.
   void EmitStoreOfScalar(llvm::Value *value, LValue lvalue, bool isInit=false);
 
   /// EmitLoadOfLValue - Given an expression that represents a value lvalue,
   /// this method emits the address of the lvalue, then loads the result as an
   /// rvalue, returning the rvalue.
   RValue EmitLoadOfLValue(LValue V, SourceLocation Loc);
   RValue EmitLoadOfExtVectorElementLValue(LValue V);
-  RValue EmitLoadOfBitfieldLValue(LValue LV);
+  RValue EmitLoadOfBitfieldLValue(LValue LV, SourceLocation Loc);
   RValue EmitLoadOfGlobalRegLValue(LValue LV);
 
   /// EmitStoreThroughLValue - Store the specified rvalue into the specified
   /// lvalue, where both are guaranteed to the have the same type, and that type
   /// is 'Ty'.
   void EmitStoreThroughLValue(RValue Src, LValue Dst, bool isInit = false);
   void EmitStoreThroughExtVectorComponentLValue(RValue Src, LValue Dst);
   void EmitStoreThroughGlobalRegLValue(RValue Src, LValue Dst);

cfe/trunk/test/CodeGenCXX/ubsan-bitfields.cpp

+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=enum | FileCheck %s
+
+enum E {
+  a = 1,
+  b = 2,
+  c = 3
+};
+
+struct S {
+  E e1 : 10;
+};
+
+// CHECK-LABEL: define i32 @_Z4loadP1S
+E load(S *s) {
+  // CHECK: [[LOAD:%.*]] = load i16, i16* {{.*}}
+  // CHECK: [[CLEAR:%.*]] = and i16 [[LOAD]], 1023
+  // CHECK: [[CAST:%.*]] = zext i16 [[CLEAR]] to i32
+  // CHECK: icmp ule i32 [[CAST]], 3, !nosanitize
+  // CHECK: call void @__ubsan_handle_load_invalid_value
+  return s->e1;
+}

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 objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - -w | 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 - -w | 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;
+  // SHARED: ret i8
 }
+
+struct S1 {
+  BOOL b1 : 1;
+};
+
+// SHARED-LABEL: f2
+BOOL f2(struct S1 *s) {
+  // OBJC: [[LOAD:%.*]] = load i8, i8* {{.*}}
+  // OBJC: [[SHL:%.*]] = shl i8 [[LOAD]], 7
+  // OBJC: [[ASHR:%.*]] = ashr i8 [[SHL]], 7
+  // OBJC: icmp ule i8 [[ASHR]], 1, !nosanitize
+  // OBJC: call void @__ubsan_handle_load_invalid_value
+
+  // C-NOT: call void @__ubsan_handle_load_invalid_value
+  return s->b1;
+  // SHARED: ret i8
+}
+
+#ifdef __OBJC__
+@interface I1 {
+@public
+  BOOL b1 : 1;
+}
+@property (nonatomic) BOOL b1;
+@end
+@implementation I1
+@synthesize b1;
+@end
+
+// Check the synthesized getter.
+// OBJC-LABEL: define internal signext i8 @"\01-[I1 b1]"
+// OBJC: [[IVAR:%.*]] = load i64, i64* @"OBJC_IVAR_$_I1.b1"
+// OBJC: [[ADDR:%.*]] = getelementptr inbounds i8, i8* {{.*}}, i64 [[IVAR]]
+// OBJC: [[LOAD:%.*]] = load i8, i8* {{.*}}
+// OBJC: [[SHL:%.*]] = shl i8 [[LOAD]], 7
+// OBJC: [[ASHR:%.*]] = ashr i8 [[SHL]], 7
+// OBJC: icmp ule i8 [[ASHR]], 1, !nosanitize
+// OBJC: call void @__ubsan_handle_load_invalid_value
+
+// Also check direct accesses to the ivar.
+// OBJC-LABEL: f3
+BOOL f3(I1 *i) {
+  // OBJC: [[LOAD:%.*]] = load i8, i8* {{.*}}
+  // OBJC: [[SHL:%.*]] = shl i8 [[LOAD]], 7
+  // OBJC: [[ASHR:%.*]] = ashr i8 [[SHL]], 7
+  // OBJC: icmp ule i8 [[ASHR]], 1, !nosanitize
+  // OBJC: call void @__ubsan_handle_load_invalid_value
+
+  return i->b1;
+  // OBJC: ret i8
+}
+#endif /* __OBJC__ */