[cfe-commits] strncpy checker - proposed patch

[Ted Kremenek]

On Feb 16, 2011, at 6:39 PM, Lenny Maiorani wrote:

> Ted,
> 
> This checker certainly is getting a bit complicated. Maybe it is time for a good scrubbing. Sorry about that patch that didn't apply cleanly to TOT. There have been some big changes in that section, so I am not surprised. More comments inline below.
> 
> -Lenny
> 
> 
> On Feb 11, 2011, at 9:46 PM, Ted Kremenek wrote:
> 
>> Hi Lenny,
>> 
>> This is looking better.  The patch doesn't apply cleanly to TOT, so would it be possible to regenerate it?  Some of the patch doesn't really match with the current contents of the checker, so it's hard to evaluate.
>> 
>> A few comments:
>> 
>> - Could you add comments about what the 'IsPotential' flag is for?  This checker is really lacking in comments, and the logic is starting to look really complicated.
> 
> In addition to adding some comments, I have a question. With strncpy(), obviously the dest buffer needs to be large enough for the number of bytes being copied. The number of bytes to be copied is the lesser of strlen(src) and the max value, size_t n, the 3rd argument to strnlen(). Should we also have another check to ensure that the size_t n (3rd argument) is always less than or equal to (<=) the size of the destination buffer? 
> 
> This is what I was adding and using the IsPotential flag to indicate that it is a different bug needs to be reported since this is only a potential future bug, and not a current problem. Do we even want that? It is making the code more confusing, certainly.
> 

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.