[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators
Anton Yartsev
anton.yartsev at gmail.com
Fri Feb 22 07:24:52 PST 2013
On 22.02.2013 9:30, Anna Zaks wrote:
>
> On Feb 21, 2013, at 6:26 PM, Anna Zaks <ganna at apple.com
> <mailto:ganna at apple.com>> wrote:
>
>>
>> On Feb 21, 2013, at 6:00 PM, Anton Yartsev <anton.yartsev at gmail.com
>> <mailto:anton.yartsev at gmail.com>> wrote:
>>
>>>>
>>>> On Feb 19, 2013, at 10:18 PM, Anton Yartsev
>>>> <anton.yartsev at gmail.com <mailto:anton.yartsev at gmail.com>> wrote:
>>>>
>>>>> Hi, Jordan. Thanx for the review!
>>>>>
>>>>> Attached is the new version of the patch with all the comments
>>>>> addressed. Also added support for directly called operator
>>>>> new()/new[]() and operator delete()
>>>>>
>>>>> There is currently one problem with handling of operator delete().
>>>>> The following test
>>>>>
>>>>> void testDeleteOp1() {
>>>>> int *p = (int *)malloc(sizeof(int));
>>>>> operator delete(p); // FIXME: should complain "Argument to
>>>>> operator delete() was allocated by malloc(), not operator new"
>>>>> }
>>>>>
>>>>> produce no warnings as attached RefState seem to be missing at the
>>>>> point when checkPostStmt(const CallExpr *CE, CheckerContext &C)
>>>>> callback is called for operator delete(p).
>>>>> I haven't investigated the problem deeply yet, intend to address
>>>>> it parallel with the review.
>>>>>
>>>>>> + if (NE->getNumPlacementArgs())
>>>>>> + return;
>>>>>> + // skip placement new operators as they may not allocate memory
>>>>>>
>>>>>> Two comments here:
>>>>>> - Please make sure all comments are complete, capitalized, and
>>>>>> punctuated sentences. (This has the important one,
>>>>>> "complete"....just missing capitalization and punctuation.)
>>>>> I'll try. Unfortunately I am not as good in English as I want to
>>>>> be, so sorry for my grammar, syntax, and punctuation.
>>>>>
>>>>> --
>>>>> Anton
>>>>>
>>>>> <MallocChecker_v2.patch>
>>>>
>>>
>>> Hi Anna. Thanks for your comments! Attached is the new patch.
>>>
>>>> Just adding another kind as extra enumeration values does not seem
>>>> right. Another option is to make Kind be a pointer to a static
>>>> array, which contains objects recording all necessary info about
>>>> each kind (MacOSKeychainAPIChecker uses this approach). This is
>>>> probably an overkill for now, but is another option.
>>> Not sure that I have got an idea.
>>> The memory and deallocator kind are both set for a RefState. Do you
>>> mean creating a static array with 'memory kinds' * 'deallocator
>>> kind' elements for all possible combinations? Also there is no
>>> necessary info other then the kind itself.
>>> Left for now.
>>
>> I think of ToBeReleasedWith* as being different types of Allocate; I
>> don't think they should be separate values in the same enum. It's
>> also unfortunate to have to copy the constant values in both places -
>> DeallocatorKind and RefState::Kind. Maybe you could restructure this
>> similarly to how this is done in SVals.h?
>>>
>>>> + const FunctionDecl *getCalleeDecl() const { return CalleeDecl; }
>>>> Do we only store the name of the allocator declaration here?
>>
>> If the Decl is always an allocator Decl, we should change the name of
>> the method to be more descriptive.
>>>> Do we need to store this in the state? (Growing states implies
>>>> memory overhead.) Can't this be easily implied from the kind?
>>> Kind can't give us information about the name of an allocator that
>>> can be malloc(), realloc(), a user function with ownership_takes
>>> attribute, etc.
>>> One solution to avoid keeping a CalleeDecl in RefState is to
>>> rollback to CallExpr::getDirectCallee() from
>>> CheckerContext::getCalleeDec() and to print "malloc()" in case of
>>> indirect calls.
>>
>> Ok, I see.
>
> After thinking about it some more, I do not think we should add an
> extra pointer to the state to differentiate between few allocator
> functions. In the case when we do not have ownership attributes, we
> only have few possible allocators, whose names we know ahead of time.
> In case we support ownership attributes, we are likely to have few
> allocator functions whose names we could just store in the checker
> state on the first encounter (like we store the IdentifierInfo).
>
> In addition, we could change the ownership attributes in such a way
> that each allocator would have a corresponding deallocator; for
> example, if we wanted to check matching allocators and deallocators.
> Annotated deallocators won't necessarily be one of the functions you
> know at compile time, so the DeallocatorKind enum would not cover it.
> I think, it's best if we kept a table on a side that would store this
> info (allocation function name, deallocator) and refer to the entries
> in the table from the state. (Take a look at MacOSKeychainAPIChecker -
> it's very similar to malloc, but it handles different
> allocator/deallocator pairs. I think a similar solution could work in
> this case as well. Other solutions that address these issues are
> welcome as well!)
Attached is the patch that uses approach with a dynamic table that holds
both allocator name and expected deallocator kind. This approach allows
to keep any allocator names, either known or new ones. The table could
be easily extended to hold additional data such as info about special
deallocators, etc.
>
>>> Jordan, what do you think about this?
>>>
>>>> +void MallocChecker::checkPostStmt(const CXXNewExpr *NE,
>>>> + CheckerContext &C) const {NE
>>>> +
>>>> + FunctionDecl *OperatorNew = NE->getOperatorNew();
>>>> + if (!OperatorNew)
>>>> + return;
>>>> +
>>>> + // Skip custom new operators
>>>> + if (!OperatorNew->isImplicit() &&
>>>> +
>>>> !C.getSourceManager().isInSystemHeader(OperatorNew->getLocStart()) &&
>>>> + !NE->isGlobalNew())
>>>> + return;
>>>> +
>>>> + // Skip standard global placement operator
>>>> new/new[](std::size_t, void * p);
>>>> + // process all other standard new/new[] operators including
>>>> placement
>>>> + // operators new/new[](std::size_t, const std::nothrow_t&)
>>>> + if (OperatorNew->isReservedGlobalPlacementOperator())
>>>> + return;
>>>>
>>>> Is there a reason why we first process each operator new in
>>>> "checkPostStmt(const callExpr" and finish processing in
>>>> "checkPostStmt(const CXXNewExpr" ? I think the code would be
>>>> simpler if everything could be done in a single callback.
>>> Code added to "checkPostStmt(const callExpr" is for processing
>>> direct calls to operator new/delete functions, "checkPostStmt(const
>>> CXXNewExpr" is for handling new expressions. Either first or second
>>> callback is called in each particular case but not both of them. Am
>>> I right?
>>>
>>
>> I see; makes sense. Please, add a comment in "checkPostStmt(const
>> callExpr*".
>>
>>>>
>>>> +void MallocChecker::PrintExpectedAllocName(raw_ostream &os,
>>>> CheckerContext &C,
>>>> + const Expr *E) const {
>>>> + DeallocatorKind dKind = GetDeallocKind(C, E);
>>>> +
>>>> + switch(dKind) {
>>>> + case D_free: os << "malloc()"; return;
>>>> + case D_delete: os << "operator new"; return;
>>>> + case D_deleteArray: os << "operator new[]"; return;
>>>> + case D_unknown: os << "unknown means"; return;
>>>>
>>>> This function is used to form user visible warnings. Do we ever
>>>> expect it to print "unknown means"? Can this be based on the Kind
>>>> stored inside of RefState, where there is no D_unknown?
>>> Right, changed the case to assert. There is actually implicit
>>> D_unknown in RefState - case when 2nd and 3rd bits are set to zero.
>>>
>>> --
>>> Anton
>>> <MallocChecker_v3.patch>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
--
Anton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130222/2df8a597/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: MallocChecker_v4.patch
Type: text/x-diff
Size: 32970 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130222/2df8a597/attachment.patch>
More information about the cfe-commits
mailing list