Page MenuHomePhabricator

[analyzer] Check for dead/impossible status checks
Needs RevisionPublic

Authored by trixirt on May 30 2018, 1:16 PM.

Details

Summary

The c++ 'new' function has several variants.
Only the no-throw versions of these return a nullptr on failure.
Error handling based on tha nullptr check for the throw version
is dead code.

ex/

char *n = new char[x];
if (n == nullptr) // this is dead/impossible code

do_something();

Diff Detail

Repository
rC Clang

Event Timeline

trixirt created this revision.May 30 2018, 1:16 PM

@trixirt I wrote a bunch of comments inline, but then after looking at the test cases,
.... this would not work.

This is a dataflow, all-paths check (the variable can NEVER be a nullptr), yet symbolic execution runs on A given paths.

Currently in the analyzer there's no way to write dataflow checks using the checker API.

HOWEVER, in your case all comparisons to NULL are constants, so this should be just implemented as an AST matcher (a good introduction is https://eli.thegreenplace.net/2014/07/29/ast-matchers-and-clang-refactoring-tools).
The good thing it probably could be expressed in just a few lines of code.

george.karpenkov requested changes to this revision.May 30 2018, 1:57 PM
This revision now requires changes to proceed.May 30 2018, 1:57 PM
MTC added a subscriber: MTC.May 30 2018, 7:13 PM
trixirt updated this revision to Diff 149868.Jun 4 2018, 4:08 PM

Convert to AST Visitor

Remove Ctx parameter from shouldNullCheckAllocation

Add a shouldNullCheckAllocation ASTMatcher

trixirt updated this revision to Diff 150003.Jun 5 2018, 9:55 AM

cleanup of last patch

Getting there! Sorry for the delay. Would it be possible to rewrite the remaining imperative code using matchers?
👍 on defining your own matcher. However, that would probably need to go in a separate PR (with tests), @alexfh and @aaron.ballman can help with the review.

lib/StaticAnalyzer/Checkers/DeadStatusChecker.cpp
33

Could this be rewritten as a matcher instead? Surely there must be one comparing the literal to null?

alexfh added inline comments.Jun 13 2018, 3:27 AM
lib/StaticAnalyzer/Checkers/DeadStatusChecker.cpp
33

For this particular part a matchers-based solution might be a bit wordy:

binaryOperator(anyOf(
  allOf(hasLHS(ignoringParenCasts(nullPointerConstant())), hasRHS(expr().bind("other"))),
  allOf(hasRHS(ignoringParenCasts(nullPointerConstant())), hasLHS(expr().bind("other")))))

But maybe it's time to add a helper matcher similar to hasEitherOperand, for example:

inline internal::Matcher<BinaryOperator> hasOperands(
    const internal::Matcher<Expr> &One, const internal::Matcher<Expr> &Another) {
  return anyOf(allOf(hasLHS(One), hasRHS(Another)), allOf(hasRHS(One), hasLHS(Another)));
}

This can be done locally, but I wouldn't object adding it to ASTMatchers.h, since it could already be used a few times in the existing code (at least in clang-tidy/bugprone/IncorrectRoundingsCheck.cpp and clang-tidy/misc/RedundantExpressionCheck.cpp).

NoQ added a comment.Jun 13 2018, 2:59 PM

Thanks for adding me!

Hmm, i think a matcher-based solution would be pretty limited. This is definitely your typical all-path data-flow problem because you want to make sure that you're looking at the last assignment to the variable.

For example:

int *m = new int;
m = nullptr;
if (m == nullptr) { ... } // no-warning

but

int *m = nullptr;
m = new int;
if (m == nullptr) { ... } // expected-warning{{}}

You might be able to fix false positives by adding a condition that the variable is not re-assigned within the function (with the help of assignment operator or due to taking a non-constant reference to it, etc). But you'll end up with a checker that finds a lot less bugs than a full-featured data flow analysis could have found. There's a canonical implementation of "the variable is not modified" check via ASTMatchers in LoopUnrolling.cpp.

If you'll ever want to find a full-featured data flow check, i'm not sure but you might be able to re-use LiveVariables analysis (the non-Relaxed one) to find the last assignment, and in this case you won't have to write data flow analysis yourself. DeadStores checker has an example of that.

NoQ added a comment.Jun 13 2018, 3:10 PM

For now the tests that i proposed may in fact accidentally work because as far as i understand you're updating the "variable contains a value returned by operator new" flag when you encounter assignment operators. The real problems will arise when the order of the assignments in the AST doesn't correspond to the order of assignments during program execution.

So one more test i'd like to have is:

int *m = new int;
while (true) {
  if (m == nullptr) { ... } // no-warning
  m = nullptr;
}
george.karpenkov requested changes to this revision.Jun 26 2018, 5:24 PM

@NoQ had some valid comments. I think this could be still useful without dataflow if we negate matches containing escaping and assignments.
Though of course this check really begs for a dataflow engine.

This revision now requires changes to proceed.Jun 26 2018, 5:24 PM

Sorry for the delay, i will have something (hopefully) this weekend.