Page MenuHomePhabricator

[analyzer] A checker for macOS-specific bool- and number-like objects.
ClosedPublic

Authored by NoQ on Jul 29 2016, 11:56 AM.

Details

Summary

If you store a boolean value as an Objective-C object NSNumber *X, and want to see if it's true or false, than it's not a good idea to write this check as 'X == YES'. Because X is a pointer, and it is unlikely that it's equal to 1. You perhaps wanted to say [X boolValue] instead. Similarly, instead of X == 3, you'd certainly want to write something like [X intValue] == 3.

Similarly, in XNU/driver code, if you have a C++ object OSBoolean *X, which points to one of the two immutable objects - kOSBooleanTrue or kOSBooleanFalse , it's not a good idea to convert X to a C++ bool directly (eg. bool B = X). You want to compare it to kOSBooleanTrue or call the ->isTrue() method instead.

Bugs of both kinds have been noticed in real-world code, so i'm attempting to make a simple checker for that. It is purely AST-based and moreover uses only ASTMatchers for everything (which makes it the first analyzer checker to use AST matchers). In fact i'm not sure if this checker should be in the analyzer or in clang-tidy; there should be no problem to move it around.

I'm still going to tweak these matchers a bit, as i'm in the middle of gathering statistics.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ updated this revision to Diff 66157.Jul 29 2016, 11:56 AM
NoQ retitled this revision from to [analyzer] A checker for macOS-specific bool-like objects..
NoQ updated this object.
NoQ added reviewers: zaks.anna, dcoughlin.
NoQ added a subscriber: cfe-commits.
dcoughlin added inline comments.Aug 26 2016, 6:48 PM
lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp
62 ↗(On Diff #66157)
  • The '[boolValue]' thing here is weird. Maybe use '-boolValue' instead?
  • How about "Comparison between 'NSNumber *' and 'BOOL'; use -boolValue instead."
  • You probably want to remove lifetime qualifiers from the type with .getUnqualifiedType(). before printing (i.e., don't mention '__strong') in the diagnostic.
80 ↗(On Diff #66157)

The AST matchers seem pretty convenient!

test/Analysis/bool-conversion.cpp
18 ↗(On Diff #66157)

It is generally good to include the diagnostic text in the test for the warning. This way we make sure we get the warning we expected.

test/Analysis/bool-conversion.m
2 ↗(On Diff #66157)

You should add a test invocation here where -fobjc-arc is set as well. This adds a bunch of implicit goop that it would be good to test with.

6 ↗(On Diff #66157)

These 'const's are not idiomatic. It is probably better to remove them.

10 ↗(On Diff #66157)

There is a Sema warning for p == YES already, right? ("comparison between pointer and integer ('NSNumber *' and 'int')")

11 ↗(On Diff #66157)

Should p == NO be on in non-pedantic mode, as well? It also seems to me that you could warn on comparison of *any* ObjCObjectPointerType type to NO. At a minimum it would probably be good to check for comparisons of id to NO.

zaks.anna edited edge metadata.Sep 12 2016, 10:54 AM

Let's test it on more real word bugs.

lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp
11 ↗(On Diff #66157)

Should we warn on comparisons of NSNumber to '0'? Comparisons to 'nil' would be considered a proper pointer comparison, while comparisons to literal '0' would be considered a faulty comparison of the numbers.

49 ↗(On Diff #66157)

"throws" -> "generates"?

Here are more comments. Could you address/answer these and upload the latest patch that compares NSNumber to other numbers?

Thanks!

lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp
88 ↗(On Diff #66157)

Can you test if matching for NSNumber works when they come from ivars, properties, and method returns, works?

117 ↗(On Diff #66157)

Can we use any binary operator here without any restrictions?

test/Analysis/bool-conversion.cpp
18 ↗(On Diff #66157)

+1

Are these pedantic because we assume these are comparing the pointer and not the value? Have you checked that this is a common idiom?

Same comment applies to NSNumber. If it is common practice to compare against nil..

21 ↗(On Diff #66157)

Why is this OK?

test/Analysis/bool-conversion.m
2 ↗(On Diff #66157)

+ 1!!!
Especially, since we are matching the AST.

10 ↗(On Diff #66157)

These tests seem to be even more relevant to OSBoolean.

NoQ updated this revision to Diff 72392.Sep 24 2016, 12:44 PM
NoQ retitled this revision from [analyzer] A checker for macOS-specific bool-like objects. to [analyzer] A checker for macOS-specific bool- and number-like objects..
NoQ updated this object.
NoQ edited edge metadata.
NoQ marked 10 inline comments as done.

The checker now checks conversions of NSNumber to integer types, not just boolean.

To do now - a bit more time needed to test various tweaks. The current version of the non-pedantic checker has found a couple of good new true positives, being relatively silent in general. I'm keeping positives of form x == 0 (where x is a number pointer) in non-pedantic checker, even though they have been reporting non-bugs, because bugs of this form have been found, and it seems very desirable to make the code more specific in this particular case.

To do later - consider conversions by passing NSNumber to a function that expects an integer, consider OSNumber and maybe CFNumber(?).

NoQ marked an inline comment as done.Sep 25 2016, 7:15 AM

Ouch, seems inline answers don't automatically post themselves when you update the diff :o

lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp
62 ↗(On Diff #66157)

Yeah, noticed the lifetime qualifiers. Changed messages significantly.

117 ↗(On Diff #66157)

Comparisons are handled here. Assignments are handled in a different matcher. Arithmetic operators are either a valid pointer arithmetic (such as + or -) or cause compile errors (such as * or /). Bitwise ops (&, >>, etc.) - perhaps somebody is computing shadow memory. Logical ops - also too commonly intentional (typically: if (p && [p boolValue])). Comma operator - ouch, we should probably warn about the very fact that somebody uses comma operators.

test/Analysis/bool-conversion.cpp
18 ↗(On Diff #66157)

Are these pedantic because we assume these are comparing the pointer and not the value? Have you checked that this is a common idiom?

I haven't checked if this is more popular or less popular than if (p == nil), but it's clearly too popular and too often correct for a warning enabled by default.

21 ↗(On Diff #66157)

Will test more closely; i think this gives a lot of intentionals, but a cast to a non-boolean integer is clearly an error that must be reported.

test/Analysis/bool-conversion.m
10 ↗(On Diff #66157)

There is a Sema warning

Yep, but it is disabled with -w. I think it's a good idea to add -w to the run line in all analyzer tests - it often makes tests easier to read, and in particular we're sure that all warnings in the test are coming from the enabled checkers, not from other sources.

11 ↗(On Diff #66157)

Whoops, accidental.

NoQ updated this revision to Diff 73889.Oct 7 2016, 1:21 AM
  • Rename test files for consistency.
  • Suppress false positives on intptr_t.
  • Add a triple to the tests to in order to stop worrying about integer types.
zaks.anna accepted this revision.Oct 12 2016, 10:00 PM
zaks.anna edited edge metadata.
zaks.anna added inline comments.
include/clang/StaticAnalyzer/Checkers/Checkers.td
479 ↗(On Diff #73889)

"number pointers" -> "objects representing numbers"

This revision is now accepted and ready to land.Oct 12 2016, 10:00 PM
NoQ updated this revision to Diff 74969.Oct 18 2016, 3:43 AM
NoQ edited edge metadata.
  • Support conversion though function calls.
  • Move "if (x == 0)" to pedantic for now (too loud).
This revision was automatically updated to reflect the committed changes.