[PATCH v2] staging/wlan-ng: remove strcpy() use in favor of strscpy()

[PATCH v2] staging/wlan-ng: remove strcpy() use in favor of strscpy()

[Calvince Otieno]

In response to the suggestion by Dan Carpenter on the initial patch,
this patch provides a correct usage of the strscpy() in place of the
current strcpy() implementation.

strscpy() copies characters from the source buffer to the destination
buffer until one of the following conditions is met:
	- null-terminator ('\0') is encountered in the source string.
	- specified maximum length of the destination buffer is reached.
	- source buffer is exhausted.
Example:
	char dest[11];
	const char *PRISM2_USB_FWFILE = "prism2_ru.fw";
	strscpy(dest, PRISM2_USB_FWFILE, sizeof(dest));

	In this case, strscpy copies the first 10 characters of src into dest
	and add a null-terminator. dest will then contain "prism2_ru.f" with
	proper null-termination.

Since the specified length of the dest buffer is not derived from the
dest buffer itself and rather form plug length (s3plug[i].len),
replacing strcpy() with strscpy() is a better option because it will
ensures that the destination string is always properly terminated.

Signed-off-by: Calvince Otieno <calvncce@gmail.com>
---
 drivers/staging/wlan-ng/prism2fw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wlan-ng/prism2fw.c b/drivers/staging/wlan-ng/prism2fw.c
index 5d03b2b9aab4..3ccd11041646 100644
--- a/drivers/staging/wlan-ng/prism2fw.c
+++ b/drivers/staging/wlan-ng/prism2fw.c
@@ -725,7 +725,7 @@ static int plugimage(struct imgchunk *fchunk, unsigned int nfchunks,
 
 		if (j == -1) {	/* plug the filename */
 			memset(dest, 0, s3plug[i].len);
-			strncpy(dest, PRISM2_USB_FWFILE, s3plug[i].len - 1);
+			strscpy(dest, PRISM2_USB_FWFILE, s3plug[i].len);
 		} else {	/* plug a PDR */
 			memcpy(dest, &pda->rec[j]->data, s3plug[i].len);
 		}
-- 
2.34.1

Re: [PATCH v2] staging/wlan-ng: remove strcpy() use in favor of strscpy()

[Greg Kroah-Hartman]

On Thu, Oct 12, 2023 at 05:01:57PM +0300, Calvince Otieno wrote:
> In response to the suggestion by Dan Carpenter on the initial patch,
> this patch provides a correct usage of the strscpy() in place of the
> current strcpy() implementation.
> 
> strscpy() copies characters from the source buffer to the destination
> buffer until one of the following conditions is met:
> 	- null-terminator ('\0') is encountered in the source string.
> 	- specified maximum length of the destination buffer is reached.
> 	- source buffer is exhausted.
> Example:
> 	char dest[11];
> 	const char *PRISM2_USB_FWFILE = "prism2_ru.fw";
> 	strscpy(dest, PRISM2_USB_FWFILE, sizeof(dest));
> 
> 	In this case, strscpy copies the first 10 characters of src into dest
> 	and add a null-terminator. dest will then contain "prism2_ru.f" with
> 	proper null-termination.
> 
> Since the specified length of the dest buffer is not derived from the
> dest buffer itself and rather form plug length (s3plug[i].len),
> replacing strcpy() with strscpy() is a better option because it will
> ensures that the destination string is always properly terminated.
> 
> Signed-off-by: Calvince Otieno <calvncce@gmail.com>
> ---
>  drivers/staging/wlan-ng/prism2fw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/wlan-ng/prism2fw.c b/drivers/staging/wlan-ng/prism2fw.c
> index 5d03b2b9aab4..3ccd11041646 100644
> --- a/drivers/staging/wlan-ng/prism2fw.c
> +++ b/drivers/staging/wlan-ng/prism2fw.c
> @@ -725,7 +725,7 @@ static int plugimage(struct imgchunk *fchunk, unsigned int nfchunks,
>  
>  		if (j == -1) {	/* plug the filename */
>  			memset(dest, 0, s3plug[i].len);
> -			strncpy(dest, PRISM2_USB_FWFILE, s3plug[i].len - 1);
> +			strscpy(dest, PRISM2_USB_FWFILE, s3plug[i].len);
>  		} else {	/* plug a PDR */
>  			memcpy(dest, &pda->rec[j]->data, s3plug[i].len);
>  		}
> -- 
> 2.34.1
> 
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH v2] staging/wlan-ng: remove strcpy() use in favor of strscpy()

[Calvince Otieno]

On Thu, Oct 12, 2023 at 7:42 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Oct 12, 2023 at 05:01:57PM +0300, Calvince Otieno wrote:
> > In response to the suggestion by Dan Carpenter on the initial patch,
> > this patch provides a correct usage of the strscpy() in place of the
> > current strcpy() implementation.
> >
> > strscpy() copies characters from the source buffer to the destination
> > buffer until one of the following conditions is met:
> >       - null-terminator ('\0') is encountered in the source string.
> >       - specified maximum length of the destination buffer is reached.
> >       - source buffer is exhausted.
> > Example:
> >       char dest[11];
> >       const char *PRISM2_USB_FWFILE = "prism2_ru.fw";
> >       strscpy(dest, PRISM2_USB_FWFILE, sizeof(dest));
> >
> >       In this case, strscpy copies the first 10 characters of src into dest
> >       and add a null-terminator. dest will then contain "prism2_ru.f" with
> >       proper null-termination.
> >
> > Since the specified length of the dest buffer is not derived from the
> > dest buffer itself and rather form plug length (s3plug[i].len),
> > replacing strcpy() with strscpy() is a better option because it will
> > ensures that the destination string is always properly terminated.
> >
> > Signed-off-by: Calvince Otieno <calvncce@gmail.com>
> > ---
> >  drivers/staging/wlan-ng/prism2fw.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/wlan-ng/prism2fw.c b/drivers/staging/wlan-ng/prism2fw.c
> > index 5d03b2b9aab4..3ccd11041646 100644
> > --- a/drivers/staging/wlan-ng/prism2fw.c
> > +++ b/drivers/staging/wlan-ng/prism2fw.c
> > @@ -725,7 +725,7 @@ static int plugimage(struct imgchunk *fchunk, unsigned int nfchunks,
> >
> >               if (j == -1) {  /* plug the filename */
> >                       memset(dest, 0, s3plug[i].len);
> > -                     strncpy(dest, PRISM2_USB_FWFILE, s3plug[i].len - 1);
> > +                     strscpy(dest, PRISM2_USB_FWFILE, s3plug[i].len);
> >               } else {        /* plug a PDR */
> >                       memcpy(dest, &pda->rec[j]->data, s3plug[i].len);
> >               }
> > --
> > 2.34.1
> >
> >
>
> Hi,
>
> This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> a patch that has triggered this response.  He used to manually respond
> to these common problems, but in order to save his sanity (he kept
> writing the same thing over and over, yet to different people), I was
> created.  Hopefully you will not take offence and will fix the problem
> in your patch and resubmit it so that it can be accepted into the Linux
> kernel tree.
>
> You are receiving this message because of the following common error(s)
> as indicated below:
>
> - This looks like a new version of a previously submitted patch, but you
>   did not list below the --- line any changes from the previous version.
>   Please read the section entitled "The canonical patch format" in the
>   kernel file, Documentation/process/submitting-patches.rst for what
>   needs to be done here to properly describe this.
>
> If you wish to discuss this problem further, or you have questions about
> how to resolve this issue, please feel free to respond to this email and
> Greg will reply once he has dug out from the pending patches received
> from other developers.
>
> thanks,
>
> greg k-h's patch email bot



Hello Greg,

I did amend my first commit

I used the command: git commit --amend -v
The result of this commit action is what I sent over.
-- 
Kind regards,
Calvince Otieno

Re: [PATCH v2] staging/wlan-ng: remove strcpy() use in favor of strscpy()

[Philipp Hortmann]

On 10/12/23 19:47, Calvince Otieno wrote:
> On Thu, Oct 12, 2023 at 7:42 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>>
>> On Thu, Oct 12, 2023 at 05:01:57PM +0300, Calvince Otieno wrote:
>>> In response to the suggestion by Dan Carpenter on the initial patch,
>>> this patch provides a correct usage of the strscpy() in place of the
>>> current strcpy() implementation.
>>>
>>> strscpy() copies characters from the source buffer to the destination
>>> buffer until one of the following conditions is met:
>>>        - null-terminator ('\0') is encountered in the source string.
>>>        - specified maximum length of the destination buffer is reached.
>>>        - source buffer is exhausted.
>>> Example:
>>>        char dest[11];
>>>        const char *PRISM2_USB_FWFILE = "prism2_ru.fw";
>>>        strscpy(dest, PRISM2_USB_FWFILE, sizeof(dest));
>>>
>>>        In this case, strscpy copies the first 10 characters of src into dest
>>>        and add a null-terminator. dest will then contain "prism2_ru.f" with
>>>        proper null-termination.
>>>
>>> Since the specified length of the dest buffer is not derived from the
>>> dest buffer itself and rather form plug length (s3plug[i].len),
>>> replacing strcpy() with strscpy() is a better option because it will
>>> ensures that the destination string is always properly terminated.
>>>
>>> Signed-off-by: Calvince Otieno <calvncce@gmail.com>
>>> ---
Hi,

Greg wants you to add a changelog here below the "---". Can look like this:
v2 : description of changes

Bye Philipp


>>>   drivers/staging/wlan-ng/prism2fw.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/wlan-ng/prism2fw.c b/drivers/staging/wlan-ng/prism2fw.c
>>> index 5d03b2b9aab4..3ccd11041646 100644
>>> --- a/drivers/staging/wlan-ng/prism2fw.c
>>> +++ b/drivers/staging/wlan-ng/prism2fw.c
>>> @@ -725,7 +725,7 @@ static int plugimage(struct imgchunk *fchunk, unsigned int nfchunks,
>>>
>>>                if (j == -1) {  /* plug the filename */
>>>                        memset(dest, 0, s3plug[i].len);
>>> -                     strncpy(dest, PRISM2_USB_FWFILE, s3plug[i].len - 1);
>>> +                     strscpy(dest, PRISM2_USB_FWFILE, s3plug[i].len);
>>>                } else {        /* plug a PDR */
>>>                        memcpy(dest, &pda->rec[j]->data, s3plug[i].len);
>>>                }
>>> --
>>> 2.34.1
>>>
>>>
>>
>> Hi,
>>
>> This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
>> a patch that has triggered this response.  He used to manually respond
>> to these common problems, but in order to save his sanity (he kept
>> writing the same thing over and over, yet to different people), I was
>> created.  Hopefully you will not take offence and will fix the problem
>> in your patch and resubmit it so that it can be accepted into the Linux
>> kernel tree.
>>
>> You are receiving this message because of the following common error(s)
>> as indicated below:
>>
>> - This looks like a new version of a previously submitted patch, but you
>>    did not list below the --- line any changes from the previous version.
>>    Please read the section entitled "The canonical patch format" in the
>>    kernel file, Documentation/process/submitting-patches.rst for what
>>    needs to be done here to properly describe this.
>>
>> If you wish to discuss this problem further, or you have questions about
>> how to resolve this issue, please feel free to respond to this email and
>> Greg will reply once he has dug out from the pending patches received
>> from other developers.
>>
>> thanks,
>>
>> greg k-h's patch email bot
> 
> 
> 
> Hello Greg,
> 
> I did amend my first commit
> 
> I used the command: git commit --amend -v
> The result of this commit action is what I sent over.

Re: [PATCH v2] staging/wlan-ng: remove strcpy() use in favor of strscpy()

[Dan Carpenter]

On Thu, Oct 12, 2023 at 05:01:57PM +0300, Calvince Otieno wrote:
> In response to the suggestion by Dan Carpenter on the initial patch,
> this patch provides a correct usage of the strscpy() in place of the
> current strcpy() implementation.
> 
> strscpy() copies characters from the source buffer to the destination
> buffer until one of the following conditions is met:
> 	- null-terminator ('\0') is encountered in the source string.
> 	- specified maximum length of the destination buffer is reached.
> 	- source buffer is exhausted.
> Example:
> 	char dest[11];
> 	const char *PRISM2_USB_FWFILE = "prism2_ru.fw";
> 	strscpy(dest, PRISM2_USB_FWFILE, sizeof(dest));
> 
> 	In this case, strscpy copies the first 10 characters of src into dest
> 	and add a null-terminator. dest will then contain "prism2_ru.f" with
> 	proper null-termination.
> 
> Since the specified length of the dest buffer is not derived from the
> dest buffer itself and rather form plug length (s3plug[i].len),
> replacing strcpy() with strscpy() is a better option because it will
> ensures that the destination string is always properly terminated.

Ok, the original code also ensured that the destination string was NUL
terminated.

Here is what I want this commit message to look like:
===
Checkpatch encourages everyone to use strscpy() instead of strncpy().
The advantages are that it always adds a NUL terminator and it prevents
a read overflow if the src string is not properly terminated.  One
potential disadvantage is that it doesn't zero pad the string like
strncpy() does.

In this code, strscpy() and strncpy() are equivalent and it does not
affect runtime behavior.  The string is zeroed on the line before
using memset().  The resulting string was always NUL terminated and
PRISM2_USB_FWFILE is string literal "prism2_ru.fw" so it's NUL
terminated.

However, even though using strscpy() does not fix any bugs, it's
still nicer and makes checkpatch happy.
===

> 
> Signed-off-by: Calvince Otieno <calvncce@gmail.com>
> ---

v2:  Update the commit message.  Remove the " - 1" from the strcpy().

regards,
dan carpenter