[cfe-commits] strncpy checker - proposed patch

[Ted Kremenek]

Committed: r126188

On Feb 21, 2011, at 6:27 PM, Lenny Maiorani wrote:

> 
> On Feb 21, 2011, at 11:44 AM, Lenny Maiorani wrote:
> 
>>> I think the general question with warnings is whether it flags an issue that a developer cares about.  In general, I think there are two kinds of warnings here:
>>> 
>>> 1) Calling strncpy will overflow the destination buffer because both the size of the source string and the 3rd argument exceed the capacity of the destination.  This is an actual buffer overlow.
>>> 
>>> 2) Calling strncpy will possibly overflow the destination buffer because we don't know the size of the source string (which could be larger than the destination) and that the 3rd argument exceeds the capacity of the destination buffer, thus not properly guarding against a buffer overflow.
>>> 
>>> I think (2) is just a less constrained version of (1).  The whole purpose of the 3rd argument is to guard against buffer overflows, so it seems something worth reporting.
>>> 
>>> The other possibility is:
>>> 
>>> 3) The source buffer is<= the size of the destination buffer, but the 3rd argument is greater than the size of the destination buffer.
>>> 
>>> I'd argue that (3) is worth reporting as well, but it's a less severe issue.  It's not a buffer overflow, but a time bomb waiting to go off in the user's code if the destination buffer happens to shrink or the source string gets bigger.  In the case of (3), things are just as safe as if we had called strcpy() where the buffer sizes happened to line up, so it's basically a poor use of the API.
>>> 
>>> One thing to note is that this checker isn't actively being run on a lot of code.  Deciding whether or not a check is good also involves running it over a ton of code, and seeing the typical violations it flags.
>> Ted,
>> 
>> I agree with your assessment. You said is much more clearly than I did, but case 3 above is the exact thing I was attempting to describe. I will submit a patch for review which includes the solution for case 1. As for case 2, I am ignoring that at the moment. Case 3 I need to re-look at my patch.
> 
> Attached is a patch implemented case 1 above. This checker validates that the number of bytes to be copied to the destination buffer is less than or equal to (<=) the length of the destination buffer. The number of bytes to be copied is determined by finding the lesser of the length of the source buffer and the value of the 3rd argument to strncpy().
> 
> Please review.
> 
> -Lenny
> <strncpy-checker.diff>
> 
> 
> --
> the definition of open: "mkdir android ; cd android ; repo init -i git://android.git.kernel.org/platform/manifest.git ; repo sync ; make" - Andy Rubin
>