Page MenuHomePhabricator

[analyzer] Detect pointers escaped after return statement execution in MallocChecker
ClosedPublic

Authored by rnkovacs on Jul 15 2018, 9:48 PM.

Details

Summary

Sometimes an object is destroyed right after the statement returning it is executed. This patch aims to make MallocChecker warn for these cases as well.

FIXME1: Using two traits might not be the best solution (fixed).
FIXME2: The warning is emitted at the end of the function, which might be confusing.

Diff Detail

Repository
rL LLVM

Event Timeline

rnkovacs created this revision.Jul 15 2018, 9:48 PM
NoQ added a comment.Jul 15 2018, 10:05 PM

Aha, so the destructors are called after the return statement! Makes sense.

The return statement is available in ExprEngine::processEndOfFunction(), so i guess we could improve the checkEndFunction() callback to provide it, and then we wouldn't need any of those extra state traits: the returned expression would be a sub-expression of the return statement and the return value would be, well, the value of the returned expression (hopefully). The procedure of adding more arguments to a callback is a bit annoying but straightforward.

I guess it'd be great to add a MallocChecker-only test. It should be possible by mocking a simple object that frees memory in its destructor (in a manner that the Analyzer can understand by inlining all methods of the object) and then returning that memory.

The warning is emitted at the end of the function, which might be confusing.

I'm afraid it might be hard to fix; path-sensitive reports are usually thrown against the node and that node is already too far and the return statement node is too early.

@george.karpenkov would your getEndPath()-fu help here?

rnkovacs updated this revision to Diff 157375.Jul 25 2018, 3:04 PM
rnkovacs retitled this revision from [analyzer][WIP] Detect pointers escaped after return statement execution in MallocChecker to [analyzer] Detect pointers escaped after return statement execution in MallocChecker.
rnkovacs edited the summary of this revision. (Show Details)

Updated to use the extended checkEndFunction() callback (committed in rL337215 - I forgot to add it as a dependency).

The first ones of the 2-2 tests added for InnerPointerChecker and NewDeleteChecker already worked before this patch, the second ones are the newly working ones. I just felt they are somewhat both relevant and maybe worth comparing.

NoQ accepted this revision.Jul 25 2018, 3:54 PM

Yup, tests for temporaries are great to have as well.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2475–2477 ↗(On Diff #157375)

The old code makes the warning appear slightly earlier, so we still need it, even if duplicated. Let's comment about that and maybe de-duplicate?

This revision is now accepted and ready to land.Jul 25 2018, 3:54 PM
rnkovacs updated this revision to Diff 157966.Jul 30 2018, 8:09 AM
rnkovacs marked an inline comment as done.

De-duplicate & add comment.

NoQ added inline comments.Jul 30 2018, 11:45 AM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2488 ↗(On Diff #157966)

Let's do a common sub-function, similarly to how MallocChecker::checkPointerEscape and MallocChecker::checkConstPointerEscape both call MallocChecker::checkPointerEscapeAux. Should prevent us from unleashing addTransition hell if the code becomes more complicated.

NoQ accepted this revision.Jul 30 2018, 11:45 AM
rnkovacs updated this revision to Diff 158681.Aug 1 2018, 8:04 PM
rnkovacs marked an inline comment as done.

Add helper function to be used in both callbacks.

xazax.hun accepted this revision.Aug 2 2018, 10:54 AM
NoQ accepted this revision.Aug 2 2018, 10:58 AM

All this stuff looks great! Please commit.

This revision was automatically updated to reflect the committed changes.