[PATCH] staging: usbip: stub_main.c: Cleaning up missing null-terminate after strncpy call

[PATCH] staging: usbip: stub_main.c: Cleaning up missing null-terminate after strncpy call

[Rickard Strandqvist]

Added a guaranteed null-terminate after call to strncpy.

This was partly found using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/staging/usbip/stub_main.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/usbip/stub_main.c b/drivers/staging/usbip/stub_main.c
index 9c5832a..f1eb6a2 100644
--- a/drivers/staging/usbip/stub_main.c
+++ b/drivers/staging/usbip/stub_main.c
@@ -166,6 +166,7 @@ static ssize_t store_match_busid(struct device_driver *dev, const char *buf,
 		return -EINVAL;
 
 	strncpy(busid, buf + 4, BUSID_SIZE);
+	busid[BUSID_SIZE - 1] = '\0';
 
 	if (!strncmp(buf, "add ", 4)) {
 		if (add_match_busid(busid) < 0)
-- 
1.7.10.4

Re: [PATCH] staging: usbip: stub_main.c: Cleaning up missing null-terminate after strncpy call

[Dan Carpenter]

On Wed, Jun 04, 2014 at 11:39:49PM +0200, Rickard Strandqvist wrote:
> Added a guaranteed null-terminate after call to strncpy.
> 
> This was partly found using a static code analysis program called cppcheck.
> 

We already knew that the string was NUL terminated because we checked
strnlen() on the lines before.

regards,
dan carpenter

Re: [PATCH] staging: usbip: stub_main.c: Cleaning up missing null-terminate after strncpy call

[Rickard Strandqvist]

Hi

True!
Sorry  :-(

But then one would either operate strcpy outright.

Or use strlcpy then the code would be:

    /* strlcpy() handles not include \0 */
    len = strlcpy(busid, buf + 4, BUSID_SIZE);

    /* busid needs to include \0 termination */
    if (!(len < BUSID_SIZE))
        return -EINVAL;


Or should we just let it be, perhaps?


Kind regards
Rickard Strandqvist


2014-06-10 8:57 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Wed, Jun 04, 2014 at 11:39:49PM +0200, Rickard Strandqvist wrote:
>> Added a guaranteed null-terminate after call to strncpy.
>>
>> This was partly found using a static code analysis program called cppcheck.
>>
>
> We already knew that the string was NUL terminated because we checked
> strnlen() on the lines before.
>
> regards,
> dan carpenter
>

Re: [PATCH] staging: usbip: stub_main.c: Cleaning up missing null-terminate after strncpy call

[Dan Carpenter]

On Tue, Jun 10, 2014 at 10:48:35PM +0200, Rickard Strandqvist wrote:
> Hi
> 
> True!
> Sorry  :-(
> 
> But then one would either operate strcpy outright.
> 
> Or use strlcpy then the code would be:
> 
>     /* strlcpy() handles not include \0 */
>     len = strlcpy(busid, buf + 4, BUSID_SIZE);
> 
>     /* busid needs to include \0 termination */
>     if (!(len < BUSID_SIZE))

I don't like this condition.  Just say (len >= BUSID_SIZE).  The
comments here are obvious and could be left out.

>         return -EINVAL;

I don't have strong feelings about a cleanup patch.  But I think that
cppcheck is not being very sofisticated here with the NUL termination
warning so we should not go out of our way to try to silence the
warning.

regards,
dan carpenter

Re: [PATCH] staging: usbip: stub_main.c: Cleaning up missing null-terminate after strncpy call

[Rickard Strandqvist]

2014-06-11 7:45 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Tue, Jun 10, 2014 at 10:48:35PM +0200, Rickard Strandqvist wrote:
>> Hi
>>
>> True!
>> Sorry  :-(
>>
>> But then one would either operate strcpy outright.
>>
>> Or use strlcpy then the code would be:
>>
>>     /* strlcpy() handles not include \0 */
>>     len = strlcpy(busid, buf + 4, BUSID_SIZE);
>>
>>     /* busid needs to include \0 termination */
>>     if (!(len < BUSID_SIZE))
>
> I don't like this condition.  Just say (len >= BUSID_SIZE).  The
> comments here are obvious and could be left out.
>
>>         return -EINVAL;
>
> I don't have strong feelings about a cleanup patch.  But I think that
> cppcheck is not being very sofisticated here with the NUL termination
> warning so we should not go out of our way to try to silence the
> warning.
>
> regards,
> dan carpenter
>


Hi Dan

I agree that you should not do patches just to silence a static control program.

Concerning (len >= BUSID_SIZE) I agree! But I usually try to change as
little as possible in the patches I do. But perhaps I should not think
that way.



Kind regards
Rickard Strandqvist

Re: [PATCH] staging: usbip: stub_main.c: Cleaning up missing null-terminate after strncpy call

[Dan Carpenter]

On Thu, Jun 12, 2014 at 11:09:20PM +0200, Rickard Strandqvist wrote:
> I agree that you should not do patches just to silence a static control program.
> 
> Concerning (len >= BUSID_SIZE) I agree! But I usually try to change as
> little as possible in the patches I do. But perhaps I should not think
> that way.

The "one thing per patch" rule is tricky for a lot of people when they
start upstream kernel programming.

1) If you are going to need to backport a patch then write the simplest
   version you can and do any cleanup in a later patch.

2) Most of these patches will not need to back ported.  The "one thing"
is about how you describe the patch.  You're allowed to make minor
closely related changes.  In this case the one thing would be, "Clean up
string handling in xxx()".  The commit message would say:

cppcheck has a false positive here.  I looked at the code and it's ok,
but a bit messy.  I have cleaned it up by doing:

	1) use strclpy() or whatever
	2) cleanup a condition
	3) remove obvious comments
	4) blah blah blah

The new code is simpler and doesn't generate a cppcheck warning.

Where checkpatch.pl get into trouble is that they say "I am doing one
thing and it is to fix everything in filename.c".  That's everything and
not "one thing".

regards,
dan carpenter