[PATCH] [StaticAnalyzer]Handle Destructor call generated by C++ delete expr

Anna Zaks ganna at apple.com
Fri Sep 13 09:17:27 PDT 2013


On Sep 13, 2013, at 9:06 AM, Jordan Rose <jordan_rose at apple.com> wrote:

> 
> On Sep 13, 2013, at 9:03 , Anna Zaks <ganna at apple.com> 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 <kv.bhat at samsung.com> 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
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> 
> 




More information about the cfe-commits mailing list