Looks good to me. Do you have commit access? 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 > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
