[PATCH] D28330: [analyzer] Fix false positives in Keychain API checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> 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.


https://reviews.llvm.org/D28330