[cfe-commits] [PATCH 1/1] Checking zero byte allocation for realloc() and calloc().

[Ted Kremenek]

Looks great!  Applied in r147500!  Thank you for working on this!

On Dec 25, 2011, at 12:55 PM, Cyril Roelandt wrote:

> On 12/06/2011 07:33 AM, Ted Kremenek wrote:
>> On Nov 19, 2011, at 9:07 PM, Cyril Roelandt wrote:
>> 
>>> The code should look a little bit better now. Three different functions are used. The 0-checking and report emission are now two different functions, for I thought they served a different purpose, and that the code would be easier to understand that way.
>> 
>> Hi Cyril,
>> 
>> I like this patch.  I have a few specific comments inline:
>> 
> 
> Hi,
> 
> I finally found some time to work on this issue. Attached is the new patch. I inlined some comments about your review.
> 
>>> +
>>> +// N should be non-null here.
>> 
>> Please expand the comments, indicating what this function does.
>> 
>> Also, instead of having a comment that says "N should be non-null", how about
>> using __attribute__((nonnull))?
>> 
> 
> I've finally decided not to pass an ExplodedNode * to this function, so I did not use this attribute.
> 
>>> +  if (argVal.isUnknownOrUndef())
>>> +    goto check_snd_arg;
>>> +
>>> +  if (IsZeroByteAllocation(state, argVal,&trueState,&falseState)) {
>>> +    ExplodedNode *N = C.generateSink(falseState);
>>> +    if (!N)
>>> +      goto check_snd_arg;
>>> +
>>> +    UnixAPIChecker::ReportZeroByteAllocation(C, N, arg, "calloc");
>>> +    return;
>>> +  }
>> 
>> I'm not a huge fan of gotos.  They stylistically are problematic in C++.  It's okay here, but I still think it is a little gross.  You could alternatively have a "do ... while(0)" and use "break" statements instead.  It's a little gross as well (to some people), but it is more structured.
>> 
> 
> This part of the code now uses a for loop, which is even cleaner. The body of that loop is not that clean, but I think it's the best I could do without copying/pasting a lot of code.
> 
> 
>>> +    return;
>>> +  }
>> 
>> If we have 'ReportZeroByteAllocation() generate the sink node and do the null check, you could just do:
>> 
>>   if (IsZeroByteAllocation(state, argVal,&trueState,&falseState) {
>>    UnixAPIChecker::ReportZeroByteAllocation(C, falseState, arg, "calloc");
>>    return;
>>   }
>> 
>> Same thing for the other callsites.
>> 
> 
> Excellent point.
> 
> 
> I haven't modified the test cases.
> 
> 
> Cyril.
> <patch-zero-byte-allocation.txt>