This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] C++17: Fix leak false positives when an object with destructor is returned from the top frame.
ClosedPublic

Authored by NoQ on Dec 17 2018, 6:49 PM.

Details

Summary

The return_from_top_frame test demonstrates what happens. The object bound to the sub-expression of the return statement is a lazy compound value in this case. Liveness of that expression ends slightly before the end of the analysis, leaving time for MallocChecker to diagnose a leak.

This is fixed by admitting that we do not know how to properly model the memory region in which the return value is constructed. Neither CXXTempObjectRegion nor SymbolicRegion seems suitable, because the region needs to be in an unknown memory space and must also be live after the last reference disappears. The fix is only applied to C++17 when the object has a destructor (which produces a CXX17ElidedCopyReturnedValueConstructionContext) because other cases seem to work due to liveness analysis behaving correctly.

We could (and probably should) teach liveness analysis that with C++17 AST with CXXBindTemporaryExpr it is still a good idea to keep the top-frame returned expression alive forever. But we still don't have the correct region to represent construction target. Either we should modify liveness analysis and use SymbolicRegion, or we need to come up with a new kind of region (either for the return-to object itself, or for an implicit parameter through which its address is passed into the callee, so that the return-to object would be a SymbolicRegion of its SymbolRegionValue, similarly to how CXXThisRegion works).

Before i forget: re-enable tests for temporaries on C++17. Notice that some still fail. The current patch doesn't regress any of the old tests in this file that are now FIXME'd out.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Dec 17 2018, 6:49 PM

Is there any downsides for using symbolic region for the construction target? For me that would make perfect sense, since this is often modelled by passing the address of the target into the callee. The programmer could do RVO like thing by hand, so modeling automatic and manual RVO the same way would be the least surprising in my opinion.

NoQ added a comment.Dec 18 2018, 10:54 AM

Is there any downsides for using symbolic region for the construction target? For me that would make perfect sense, since this is often modelled by passing the address of the target into the callee. The programmer could do RVO like thing by hand, so modeling automatic and manual RVO the same way would be the least surprising in my opinion.

Hmm, let me actually see how hard it is. The main reason why i don't like it is that we still need to come up with a symbol to stuff into it, so we're kinda just delaying the inevitable. If we had, for instance, a region for the "implicit variable" that stores the return address, we could announce that this region is live for as long as the stack frame is on the stack, and its SymbolRegionValue (together with the SymbolicRegion around it) would be automatically kept alive. But if we make an anonymous SymbolConjured instead, we would also need to introduce a separate liveness hack to keep it alive, which should ideally be removed later.

NoQ added a comment.EditedDec 18 2018, 10:56 AM

Actually, LiveVariables analysis is not the right place to deal with this problem; these changes would go into SymbolReaper (or directly into ExprEngine), because we're dealing with something that depends on the stack frame, while LiveVariables analysis only depends on the CFG.

NoQ updated this revision to Diff 178802.Dec 18 2018, 3:21 PM

Wait a minute.

In D55804#1334957, @NoQ wrote:

we would also need to introduce a separate liveness hack to keep it alive

No, we wouldn't! With a symbolic region it is fine if it dies too early, because putting anything into a symbolic region is an immediate escape anyways. It is still scary to keep it dying too early, but i don't see any actual problem arise from it, and in any case it's better than before, and the leaks do indeed go away.

Additionally, this is still a fairly safe change because there's nothing new in constructing into symbolic regions.

dcoughlin accepted this revision.Dec 18 2018, 8:13 PM

This seems reasonable to me, although I have a question inline about why you are using makeZeroElementRegion().

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
224 ↗(On Diff #178802)

I don't understand why you are using makeZeroElementRegion() here. Doesn't the assertion of !IsArray mean it must just return 'V'?

This revision is now accepted and ready to land.Dec 18 2018, 8:13 PM
NoQ updated this revision to Diff 178936.Dec 19 2018, 12:02 PM
NoQ marked 2 inline comments as done.

Fxd!

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
224 ↗(On Diff #178802)

Hmm. Right. I mis-remembered that it's also modeling a cast.

This revision was automatically updated to reflect the committed changes.