This diff extends StackAddrEscapeChecker to catch stack addr leaks via block captures if the block is executed
asynchronously.
Test plan: make check-all
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | ||
---|---|---|
145 | This will stop the analysis on this execution path. Is this desired? Usually, we stop the execution when there is no way to model the program state after the error, e.g.: after a division by zero. In this case the stack address escaped but it wasn't dereferenced (yet), so I think it might be safe to continue the analysis on this path. What do you think? | |
156 | The variable should start with an uppercase letter. |
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | ||
---|---|---|
128 | dispatch_once isn't really asynchronous; i think it's out of place here. Also we model it in the body farm, so i guess any errors inside its block would be caught anyway. | |
136–139 | If you like, feel free to add support for for (const auto &Item: Block->referenced_vars()) loops into BlockDataRegion :) | |
144 | getMemorySpace() is never null. Every region resides in a memory space. | |
153 | Not sure - the rest of the checker's code seems to be making use of the returned SourceRange by adding to the report, do we need that here? | |
test/Analysis/stack-async-leak.m | ||
2 | We don't add -analyzer-store=region nowadays; i don't think it's useful (though it doesn't hurt). | |
61 | You may enjoy adding an extra note piece (as in D24278) to the offending statement within the block. It might be helpful if it's not obvious which block is being executed or which pointer points to the leaking variable. Also it might be a good idea to think about a visitor that would track the block pointer to highlight where it was initialized with a concrete block code. Though probably it's not super useful because we're most likely staying within a single function with this checker. By the way, do we warn (or want to warn) on returning a block from a function, when this block captures a local variable? |
test/Analysis/stack-async-leak.m | ||
---|---|---|
61 | regarding tracking - yeah, i agree, that would be good, although i would prefer to make progress incrementally and add this part later.
good question - yeah, now we do |
I think this is a great idea, and I expect it to find some nasty bugs!
However, I'm worried about false positives in the following not-too-uncommon idiom:
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0); dispatch_async(some_queue, ^{ // Do some work dispatch_semaphore_signal(semaphore); }); // Do some other work concurrently with the asynchronous work // Wait for the asynchronous work to finish dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
What do you think about suppressing the diagnostic when the block captures a variable with type 'dispatch_semaphore_t'? This isn't a perfect suppression, but it will catch most of the cases that I have seen.
Also, does this checker diagnose when a block captures a C++ reference? If so, it would be great to add a test for that!
Some additional comments inline.
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | ||
---|---|---|
124 | Could we break with tradition and add a oxygen comment describing what this method does? | |
133 | Can you factor this logic out with the logic it duplicates in checkPreStmt()? | |
136 | Stylistically we use isa<> for cases like these where the result of dyn_cast<> is not used. | |
146 | Instead of "might leak ... " I would suggest " is captured by an asynchronously-executed block". In the context of the analyzer we use "leak" to mean "resource that is not freed" rather than in the sense of exposing information. You'll need to make a different variant of the diagnostic text for when the block is returned rather than asynchronously executed. | |
181 | Will this have a false positive if I create a block in a caller, pass it to a callee, and then the callee returns it? You may want to factor out and reuse the logic from below that compares the StackFrameContext for the current frame to the frame of the captured addresses in checkBlockCaptures(). | |
test/Analysis/stack-async-leak.m | ||
2 | Can you add an additional run line testing the expected behavior when ARC is not enabled? You can pass a -DARC_ENABLED=1 or similar to distinguish between the two (for example, to test to make sure that a diagnostic is emitted under MRR but not ARC). |
test/Analysis/stack-async-leak.m | ||
---|---|---|
2 | i've tried this, it somehow gets more complicated to read / change the tests - decided to create a separate file. |
test/Analysis/stack-capture-leak-arc.mm | ||
---|---|---|
68 ↗ | (On Diff #121230) | c++ reference |
Thanks!
Another round of comments inline. With those addressed it looks good to me -- but you should wait on Artem's go-ahead before committing.
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | ||
---|---|---|
118 | I think here you will need to check whether Q is a TypedefType and compare the IdentifierInfo for its TypedefNameDecl to 'dispatch_semaphore_t' instead. The reason is that NSObject<OS_dispatch_semaphore> is an implementation detail and in fact dispatch_semaphore_t is typedef'd to another type depending on whether the file is Objective-C(++) or not. There is an example of the idiom we use to do IdentifierInfo comparisons in ObjCDeallocChecker. We cache the identifier in the checker class. This lets us get constant-time comparisons on identifiers (since they are interned strings). Also: It would be good to add a comment explaining why we care whether a semaphore was captured. | |
280 | We generally prefer to avoid combining style cleanups like these with functional changes except in the code that is required to be updated for the functional change. This makes it easier for future maintainer to use the commit history to understand what a commit did and why. | |
test/Analysis/stack-async-leak.m | ||
2 | That's fine! | |
test/Analysis/stack-capture-leak-arc.mm | ||
144 ↗ | (On Diff #121230) | Can you add a // no-warning comment to this line? We use that to indicate that we explicitly don't expect a warning there. |
- Add no-warning comments
- Switch to using a cached IdentifierInfo (similar to how it's done in CheckObjCDealloc.cpp)
- Rerun the all the tests - they are fine.
@dcoughlin - many thanks for the code review,
yeah, i will wait for the feedback from NoQ@ as well.
Looks great, thanks! Minor inline stuff.
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | ||
---|---|---|
112 | Because stack address escapes are not fatal errors, it is not necessarily true that any other stack frame context here would be an ancestor of the current context. It may be an escaped stack variable from a function that has exited long time ago. You might want to check the getParent() chain. Hmm, or rather rename the function to "isNotInCurrentFrame" because that's how you seem to be using it. | |
131 | Typo: cap | |
135 | Hmm, why would such region even be in the list? | |
142 | I think bugtype should describe the bug, so that the user knew what sort of bug it is by looking at the list. Maybe add "...is captured" here? |
I found some nits, but overall I think this is getting close.
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | ||
---|---|---|
66 | In case you already change these lines you could replace the type name on the left side with auto to avoid repeating types. | |
119 | You can also use auto here to avoid mentioning the type twice. | |
150 | How long usually these error messages are? Maybe 512 is a bit large buffer for this? Note that in case the error message is longer this is still not a hard error, it just will hit the slow path (allocating the buffer on the heap). | |
172 | Maybe you wanted to mention "captured" here as well? | |
173 | Maybe the error reporting part could be abstracted out to avoid code duplication with the function above? |
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | ||
---|---|---|
150 |
thanks, i'm aware of that. |
In case you already change these lines you could replace the type name on the left side with auto to avoid repeating types.