On Sep 13, 2013, at 9:06 AM, Jordan Rose <[email protected]> wrote:
> > On Sep 13, 2013, at 9:03 , Anna Zaks <[email protected]> wrote: > >> Karthik, >> >> Why do we need the FIXME? It should set ill be possible to report the issue, >> no? You'd need to associate the warning with the location where the >> destructor was called. > > I asked for this but I think I got it wrong...an arbitrary destructor call > won't necessarily have an associated statement, We still should report the error even if the statement is not there. As far as I can see, the only downside is that we will not highlight the range. Use-after-free reports are most valuable to users and it does not really matter if we highlight the range or not. > but of course the one for a 'delete' should. I'm not sure we currently store > enough information to handle this, though. > I am asking to investigate this before putting in the FIXME. On which callback checkUseAfterFree is called with a NULL statement? Can we still report the error? If not yet included, please include the test case that tests this. > Jordan > > >> + // FIXME: In case of double delete of class instance. The call to >> destructor >> + // on second delete result in use of memory after free but doesn't >> correspond >> >> result -> results >> >> + // to any stmt. Currently skipping through handle the same. >> The last sentence reads wrong. >> >> + if (isReleased(Sym, C) && S) { >> ReportUseAfterFree(C, S->getSourceRange(), Sym); >> return true; >> } >> >> Cheers, >> Anna. >> >> On Sep 12, 2013, at 9:58 PM, Karthik Bhat <[email protected]> wrote: >> >>> Hi Jordan, >>> Thanks for the review and sorry for keeping this simple patch on hold for >>> such a long time. I'm a bit new to SA Core and was bit confused about the >>> UnknownVal thing. I think i understand now what you wanted to communicate. >>> So if my understanding is correct this is what we want to do- >>> >>> Memory region can be null here in 2 cases here- >>> 1) When it is actually bound to a null value. >>> 2) In case the SVal is Unknown and we try to get the region corresponding >>> to it. >>> >>> In the 1st case we can conclude that this is a null region and hence should >>> not run the destructor but in the second case we cannot be sure if it is >>> actually bound to a null region and hence should run destructor in this >>> case. >>> Am i right here? >>> >> >> Yes. That is correct. >> >>> Handled the same in ProcessDeleteDtor. What we are doing here is if we can >>> conclude that the SVal is null don't run the destructor. >>> For all other cases call VisitCXXDestructor so it is handled in the same >>> way as it is currently being done for object destruction. >>> >>> Also added a fixme in malloc checker and modified test cases to follow llvm >>> coding guidelines. >>> >>> Thanks! >>> >>> Hi jordan_rose, >>> >>> http://llvm-reviews.chandlerc.com/D1594 >>> >>> CHANGE SINCE LAST DIFF >>> http://llvm-reviews.chandlerc.com/D1594?vs=4222&id=4261#toc >>> >>> Files: >>> test/Analysis/new.cpp >>> include/clang/Analysis/CFG.h >>> lib/StaticAnalyzer/Checkers/MallocChecker.cpp >>> lib/StaticAnalyzer/Core/CallEvent.cpp >>> lib/StaticAnalyzer/Core/ExprEngine.cpp >>> <D1594.5.patch>_______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
