Page MenuHomePhabricator

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

Authored by vsk on Dec 8 2016, 7:42 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

vsk updated this revision to Diff 80856.Dec 8 2016, 7:42 PM
vsk retitled this revision from to [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}.
vsk updated this object.
vsk added a reviewer: ahatanak.
vsk added subscribers: kubamracek, cfe-commits.
arphaman added inline comments.
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 added inline comments.
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 added inline comments.Dec 9 2016, 12:32 PM
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 marked 3 inline comments as done.Dec 9 2016, 2:59 PM

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 updated this revision to Diff 80960.Dec 9 2016, 3:00 PM
vsk marked 3 inline comments as done.
  • Use NSAPI's 'is-BOOL' predicate.
  • Simplify test.
zaks.anna accepted this revision.Dec 9 2016, 3:42 PM
zaks.anna added a reviewer: zaks.anna.

LGTM!

This revision is now accepted and ready to land.Dec 9 2016, 3:42 PM
This revision was automatically updated to reflect the committed changes.

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 added a comment.Oct 23 2017, 11:40 AM

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

Awesome, good to know. Thanks!