[cfe-commits] [PATCH] review request - strcat modeling addition to CStringChecker

[Ted Kremenek]

Hi Lenny,

I think this looks good.  One high-level comment I have is that should we plan on modeling (eventually, in a later patch) the actually copy as well?  For strcat(), you return the new size of the combined string, but it would be great if we actually modeled that the new string was indeed a combination of the two other strings.  To model this efficiently, we will likely need to introduce a new SVal that represents the concatenation (so we don't literally copy the values of strings, and also have the ability to treat string concatenations symbolically).

Ted

On Apr 6, 2011, at 1:32 PM, Lenny Maiorani wrote:

> On 04/05/2011 01:29 PM, Ted Kremenek wrote:
>> Hi Lenny,
>> 
>> I'm a bit dubious about the following:
>> 
>> +  // ultimately contain both.
>> +  if (isAppending) {
>> +    // Get the string length of the destination, or give up.
>> +    SVal dstStrLength = getCStringLength(C, state, Dst, DstVal);
>> +    if (dstStrLength.isUndef())
>> +      return;
>> +
>> +    NonLoc *srcStrLengthNL = dyn_cast<NonLoc>(&strLength);
>> +    NonLoc *dstStrLengthNL = dyn_cast<NonLoc>(&dstStrLength);
>> +
>> +    QualType addTy = C.getSValBuilder().getContext().IntTy;
>> +
>> +    strLength = C.getSValBuilder().evalBinOpNN(state, BO_Add,
>> +                                               *srcStrLengthNL, *dstStrLengthNL,
>> +                                               addTy);
>> +  }
>> 
>> The dyn_cast<>  followed by the unguarded call to 'evalBinOpNN' looks wrong.  There is no guarantee that those values are non-null (which is why I assume you used a dyn_cast<>).  This looks like a potential null dereference.
> Ted,
> 
> Thanks for reviewing. Turns out there is another bug too. addTy shouldn't be IntTy, it should be getSizeType(). This wasn't causing a problem on 64-bit OSX, but before you had replied here, I switched machines to 64-bit Linux and it did matter. So, that is fixed along with your suggestion.
> 
> This patch now implements both strcat() and strncat() modeling since strncat() was a very small addition.
> 
> Please re-review. :)
> 
> -Lenny
> 
> <strcat-and-strncat-modeling-checker.diff>