Page MenuHomePhabricator

[analyzer] Syntactic matcher for leaks associated with run loop and autoreleasepool
ClosedPublic

Authored by george.karpenkov on Jul 18 2018, 5:55 PM.

Details

Summary

A checker for detecting leaks resulting from allocating temporary
autoreleasing objects before starting the main run loop.

Checks for two antipatterns:
1. ObjCMessageExpr followed by [[NARunLoop mainRunLoop] run] in the same
autorelease pool.
2. ObjCMessageExpr followed by [[NARunLoop mainRunLoop] run] in no
autorelease pool.

Happens-before relationship is modeled purely syntactically.

rdar://39299145

Diff Detail

Repository
rC Clang

Event Timeline

NoQ added inline comments.Jul 18 2018, 6:35 PM
clang/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
60 ↗(On Diff #156198)

*makes a joke about QuadBoolTy and runs away*

88 ↗(On Diff #156198)

I wonder if we could make a reusable matcher out of this: hasDescendantsInOrder(A, ..., B).

89 ↗(On Diff #156198)

This memoization map is short-lived. I don't quite understand how would we ever encounter the same statement twice during our traversal, could you give an example? Like, you're traversing the AST in a manner in which RecursiveASTVisitors usually do that, and i'm pretty sure that they encounter every statement just once, and you throw away the map immediately after that. What am i missing?

91 ↗(On Diff #156198)

So is the optional here only to add an assertion? It's essentially a non-optional in release mode(?) Maybe just assert in the callee that we never return None?

188–189 ↗(On Diff #156198)

That'd match non-functions and C++ methods and stuff like that.

In the non-matcher world i'd have checked FunctionDecl::isMain() here, but i don't think we have a matcher for that, but i guess we may add it.

clang/test/Analysis/html_diagnostics/relevant_lines/macros_same_file.c
16 ↗(On Diff #156198)

I guess this is accidental.

NoQ added inline comments.Jul 18 2018, 6:48 PM
clang/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
88 ↗(On Diff #156198)

Aha, i guess you're already discussing it on the mailing list, i didn't intend to block the patch on that.

george.karpenkov marked 3 inline comments as done.Jul 19 2018, 3:36 PM
george.karpenkov added inline comments.
clang/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
89 ↗(On Diff #156198)

i think we've discussed that some statements can still be DAGs (e.g. elvis operator)

NoQ accepted this revision.Jul 23 2018, 6:17 PM

Looks good!

clang/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
89 ↗(On Diff #156198)

Yeah, apparently it has as many as 4 (!!) sub-expressions, 3 of which partially overlap. Still not sure it's worth the memoization, but let's add a comment explaining why we did that.

This revision is now accepted and ready to land.Jul 23 2018, 6:17 PM

It is great to see a check for this!

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
572 ↗(On Diff #156904)

Mentioning "loops" three times here doesn't seem right.

"Check for leaked memory in autorelease pools that will never be popped"

clang/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
12 ↗(On Diff #156904)

"autoreleasing" --> "autoreleased"

19 ↗(On Diff #156904)

I think it would be good to explain why this pattern may lead to leaks/abandoned memory.

Something like: "Any temporary objects autoreleased in code called in the expression will not be deallocated until the program exits and so are effectively leaks."

133 ↗(On Diff #156904)

Should "mainRunLoop" really be camelCase? Does "mainRunLoop" make sense when the call is to xpc_main?

Why not just "the main run loop"?

134 ↗(On Diff #156904)

The comma here should be a semi-colon to match our other diagnostics: "may never get released; consider moving them to a".

dcoughlin added inline comments.Jul 23 2018, 9:53 PM
clang/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp
15 ↗(On Diff #156904)

Also note the typo: "NARunLoop" --> "NSRunLoop"

george.karpenkov marked 5 inline comments as done.
This revision was automatically updated to reflect the committed changes.