Hi all,
I am starting work on a cppcoreguidelines [1] checker related to object
lifetime in C++ code. In the long term, the idea is to make the
`NewDeleteChecker` more robust by modeling STL functions. Afaik, the
`NewDeleteChecker` already catches local use-after-free and mem-leak
bugs. However, as [1] suggests, the long-term view is to conservatively
flag scenarios where dangling pointers can be created.
After experimenting with Clang SA checkers, I concluded that in the
short term, it would be nice to model lifetime of stack vars. Something
as simple as the following example:
cpp
1 #include <cstddef>
2 void f() {
3 int* p = NULL;
4 {
5 int i = 0;
6 p = &i;
7 *p = 42; // ok
8 } // i's lifetime ends here
9 *p = 1; // ERROR, p was invalidated when i went out of scope
10 }
The `StackAddressEscape` checker flags a warning when the address of a
stack-local variable is returned or assigned to a global. But there
lifetime is `modeled` by simply checking if addresses can escape a stack
frame. More fine-grained checking (scopes) is missing.
Questions:
1. Why doesn't Clang SA proper not mark `i` as dead on Line 9?
2. What is the missing code that can link `i`'s lifetime to validity of `p`?
I would be happy to contribute patches to catch this simple case.
[1]:
https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/Lifetimes%20I%20and%20II%20-%20v0.9.1.pdf
Regards
Bhargava
Hi Bhargava,
Questions:
1. Why doesn't Clang SA proper not mark `i` as dead on Line 9?
This is something we would definitely like to add support for, especially now that clang is more aggressively exploiting live ranges of local variables in optimizations.
2. What is the missing code that can link `i`'s lifetime to validity of `p`?
I haven’t looked into this too deeply, but I think a reasonable approach would be to:
- Add a new kind of LocationContext for the scope of local variables. There is already an (unused!) ScopeContext subclass of LocationContext that seems like it might have been the beginning of work intended for this purpose. You might be able to just use it.
- Change the RegionStore to allow multiple StackLocalsSpaceRegion spaces per frame. I think it might be enough to just change StackLocalsSpaceRegion to take the new local scope context rather than a StackFrameContext.
- Update MemRegionManager::getVarRegion(VarDecl, const LocationContext) to create local variable regions in the correct StackLocalsSpaceRegion for the variable. (There is a FIXME there related to this “FIXME: Once we implement scope handling…”).
- Add a new check::EndLocalScope checker callback (similar to check::EndFunction) to enable checkers to be notified when a local scope is exited.
- Update the CFG and CFGBuilder to keep track of when a local scope begins and ends. (It already does this to be able to to call C++ destructors on locals when they go out of scope, so I don’t think this should be a huge change, but it will need discussion.)
- Update ExprEngine to (a) create a new local scope context when analyzing a scope beginning in the CFG and (b) call the check::EndLocalScope callback when it ends.
- Update the StackAddrEscapeChecker to register for the check::EndLocalScope callback and diagnose when an address of a variable with a no-longer in-scope ScopeContext escapes.
This may seem daunting, but most of these are relatively small changes and we would be happy to provide help and feedback along the way. If you’re up for tackling this, I would suggest taking on the CFG/CFGBuilder changes first because (1) these will have an impact on more than just the analyzer (and so will require a broader discussion) and (2) they can be can be easily tested in isolation.
Devin
Hi Devin,
Thanks for a nice action plan 
I will study the CFG builder class first and get back to you if I have
any questions.
Thanks,
Bhargava
Hi again,
Update: The CFG patch is public. Feedback much appreciated [1].
In the meantime, we can tackle low-hanging use-after-frees as an
extension of the `NewDeleteChecker`. The basic idea is to implement
lifetime checks mentioned in the Cpp core guidelines. Afaik, flagging
bugs in the following examples does not need additional support in the
analyzer i.e., implementation is purely in the realm of a checker.
Please correct me if I am wrong. Any other feedback welcome.
One extension is to catch potential raw pointer invalidations caused by
invocation of non-const container methods. Examples:
#include <memory>
using namespace std;
1. void f() {
2. auto s = make_shared<int>(0);
3. int *p = s.get();
4. s = make_shared<int>(1); // s is reallocated
5. *p = 21; // write to an invalid memory location
6. }
7.
8. void g() {
9. auto v = make_shared<vector<int>>(10);
10. vector<int>* pv = v.get();
11. v->push_back(10); // push_back is a non-const method
12. pv->push_back(100); // ERROR, pv has been invalidated
13. }
[1]: http://reviews.llvm.org/D16403