This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Fix false positives in Keychain API checker
ClosedPublic

Authored by zaks.anna on Jan 4 2017, 2:57 PM.

Details

Summary

The checker has several false positives that this patch addresses:

    1. Do not check if the return status has been compared to error (or no error) at the time when leaks are reported since the status symbol might no longer be alive. Instead, pattern match on the assume and stop tracking allocated symbols on error paths.
    2. The checker used to report error when an unknown symbol was freed. This could lead to false positives, let's not repot those. This leads to loss of coverage in double frees.
    3. Do not enforce that we should only call free if we are sure that error was not returned and the pointer is not null. That warning is too noisy and we received several false positive reports about it. (I removed: "Only call free if a valid (non-NULL) buffer was returned")
  1. Use !isDead instead of isLive in leak reporting. Otherwise, we report leaks for objects we loose track of. This change triggered change #1.

This also adds checker specific dump to the state.

Diff Detail

Event Timeline

zaks.anna updated this revision to Diff 83146.Jan 4 2017, 2:57 PM
zaks.anna retitled this revision from to [analyzer] Fix false positives in Keychain API checker.
zaks.anna updated this object.
zaks.anna added a reviewer: dcoughlin.
zaks.anna added subscribers: cfe-commits, dergachev.a.
alexander-shaposhnikov added inline comments.
lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
527

nit: auto I = ...

lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
527

and the same below (although not particularly important)

dcoughlin accepted this revision.Jan 4 2017, 4:06 PM
dcoughlin edited edge metadata.

Looks good to me, aside from minor quibbles about capitalization and variable naming.

lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
502

It seems a bit weird to call this a 'Set' -- it is a map, right?

514

Typo: 'SymintExpr' --> 'SymIntExpr'

518

'errorIsReturned' should start with a capital letter.

This revision is now accepted and ready to land.Jan 4 2017, 4:06 PM
zaks.anna updated this revision to Diff 83160.Jan 4 2017, 4:22 PM
zaks.anna edited edge metadata.

Addressed all comments

zaks.anna marked 5 inline comments as done.Jan 4 2017, 4:23 PM
NoQ added a subscriber: NoQ.Jan 5 2017, 3:42 AM

Do not check if the return status has been compared to error (or no error) at the time when leaks are reported since the status symbol might no longer be alive. Instead, pattern match on the assume and stop tracking allocated symbols on error paths.

Aha, i see! So we have pairs (A, B) of symbols (symbol A - the data that needs to be freed, and symbol B - the corresponding return value that needs to be checked for error). And liveness of A during free() doesn't imply liveness of B during free().

There are multiple options:

  1. In checkDeadSymbols, detect that B is dying, extract the necessary assume() results, and update the allocation state similarly to how you did in evalAssume, but only upon death of B.
  2. In checkLiveSymbols, mark B as live for as long as A is alive.

I'm in favor of option 2 ideologically (if we ever automate GDM symbol-to-symbol maps to avoid manual cleanup, they'd naturally work that way out of the box and will be easy to understand) and of option 1 performance-wise (we'd maintain less live symbols, our most frequently-accessed maps will become smaller).

In any case, we shouldn't keep dead symbols in checker state maps, because the kind of error you spotted may show up pretty often.

I did not think of solution #1! It's definitely better than the pattern matching I've added here. However, this checker fires so infrequently, that I do not think it's worth investing more time into perfecting it.

I suspect the solution #2 is what this checker was trying to use to begin with. It marks the return symbol as dependent on the allocated symbol by calling:

C.getSymbolManager().addSymbolDependency(V, RetStatusSymbol);

However, addSymbolDependency only worked for isLive() and not for !isDead(). Would be good to investigate this further as other checkers such as malloc also use addSymbolDependency.

NoQ accepted this revision.Jan 9 2017, 2:16 AM
NoQ added a reviewer: NoQ.

I did not think of solution #1! It's definitely better than the pattern matching I've added here. However, this checker fires so infrequently, that I do not think it's worth investing more time into perfecting it.

Well, the thing i like about your solution is that it resists arbitrary state splits. Eg., if the reason for the state split is some body farm function or a checker state split, which doesn't constitute a programmer's intent to check for error, then there's no reason to clean state. However, we won't be able to find this out by looking at range constraints retrospectively - we'd only know it during evalAssume/checkBranchCondition. However, currently we're doing a great job in other places around the analyzer to avoid unnecessary state splits, so range constraints are pretty reliable.

In fact, a check in an inlined function should also not be considered a valid reason for state cleanup. Or probably even for a state split. But that's another story. Anyway, i approve your approach here :)

This revision was automatically updated to reflect the committed changes.