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

[Cyril Roelandt]

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.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch-zero-byte-allocation
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111225/d000d2b9/attachment.ksh>