On 02/18/2011 07:24 PM, Ted Kremenek wrote: > 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. 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. -Lenny _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
