Re: [PATCH] mptfusion: use strlcpy() instead of strncpy()

[Xiongfeng Wang]

On 2018/1/16 3:34, Andy Shevchenko wrote:
> On Fri, Jan 12, 2018 at 5:09 PM, Bart Van Assche  
> wrote:
>> On Fri, 2018-01-12 at 15:45 +0200, Andy Shevchenko wrote:
>>> On Fri, Jan 12, 2018 at 1:46 PM,  Wang  wrote:
> 
>>> This one is false positive.
> 
> 
 +   strlcpy (karg.name, ioc->name, MPT_MAX_NAME);
 +   strlcpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);
>>>
>>> These two as well.
>>
>> But using strlcpy() makes the code easier to read.
> 
> This is another story, not mentioned in the original commit message.
> 
>> Xiongfeng, if you want to continue with this patch, please change the third
>> argument of all strlcpy() calls into a sizeof() expression. That make the
>> code easier to verify.
>>
 -   strncpy(karg.serial_number, " ", 24);
 +   strlcpy(karg.serial_number, " ", 24);
>>>
>>> This one is interesting indeed.
>>> Though the fix would be rather something like
>>>
>>> memset(_number, " ", 24); // leave 24 for best performance of 
>>> memset
>>> karg.serial_number[24-1] = '\0';
>>
>> Does performance really matter in this context?
> 
> Nope, but still good to understand this aspect. On some architectures
> unaligned access is expensive.
> 
>> How about the following instead:
>>
>> snprintf(karg.serial_number, sizeof(karg.serial_number), "%*s", 
>> (int)(karg.serial_number) - 1, "");
> 
> ...and you is talking about "easy to read"?!
> 
> So, my view on the patch is:
> 1) fix a real issue w/o touching everything around;


> 2) (optionally) move to strlcpy() with a correct selling point.
> 

The code is right. There are no issues. It's just that the new compiler gcc-8 
prints the warnings.
I use strlcpy() instead of strncpy() just to suppress the warnings.

Thanks,
Xiongfeng

Re: [PATCH] mptfusion: use strlcpy() instead of strncpy()

[Andy Shevchenko]

On Fri, Jan 12, 2018 at 5:09 PM, Bart Van Assche  wrote:
> On Fri, 2018-01-12 at 15:45 +0200, Andy Shevchenko wrote:
>> On Fri, Jan 12, 2018 at 1:46 PM,  Wang  wrote:

>> This one is false positive.


>> > +   strlcpy (karg.name, ioc->name, MPT_MAX_NAME);
>> > +   strlcpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);
>>
>> These two as well.
>
> But using strlcpy() makes the code easier to read.

This is another story, not mentioned in the original commit message.

> Xiongfeng, if you want to continue with this patch, please change the third
> argument of all strlcpy() calls into a sizeof() expression. That make the
> code easier to verify.
>
>> > -   strncpy(karg.serial_number, " ", 24);
>> > +   strlcpy(karg.serial_number, " ", 24);
>>
>> This one is interesting indeed.
>> Though the fix would be rather something like
>>
>> memset(_number, " ", 24); // leave 24 for best performance of 
>> memset
>> karg.serial_number[24-1] = '\0';
>
> Does performance really matter in this context?

Nope, but still good to understand this aspect. On some architectures
unaligned access is expensive.

> How about the following instead:
>
> snprintf(karg.serial_number, sizeof(karg.serial_number), "%*s", 
> (int)(karg.serial_number) - 1, "");

...and you is talking about "easy to read"?!

So, my view on the patch is:
1) fix a real issue w/o touching everything around;
2) (optionally) move to strlcpy() with a correct selling point.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] mptfusion: use strlcpy() instead of strncpy()

[Bart Van Assche]

On Fri, 2018-01-12 at 15:45 +0200, Andy Shevchenko wrote:
> On Fri, Jan 12, 2018 at 1:46 PM,  Wang  wrote:
> > 
> > -   strncpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, 
> > MPT_IOCTL_VERSION_LENGTH);
> > -   karg->driverVersion[MPT_IOCTL_VERSION_LENGTH-1]='\0';
> > +   strlcpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, 
> > MPT_IOCTL_VERSION_LENGTH);
> 
> This one is false positive.
> 
> > -   strncpy (karg.name, ioc->name, MPT_MAX_NAME);
> > -   karg.name[MPT_MAX_NAME-1]='\0';
> > -   strncpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);
> > -   karg.product[MPT_PRODUCT_LENGTH-1]='\0';
> > +   strlcpy (karg.name, ioc->name, MPT_MAX_NAME);
> > +   strlcpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);
> 
> These two as well.

But using strlcpy() makes the code easier to read.

Xiongfeng, if you want to continue with this patch, please change the third
argument of all strlcpy() calls into a sizeof() expression. That make the
code easier to verify.

> > -   strncpy(karg.serial_number, " ", 24);
> > +   strlcpy(karg.serial_number, " ", 24);
> 
> This one is interesting indeed.
> Though the fix would be rather something like
> 
> memset(_number, " ", 24); // leave 24 for best performance of 
> memset
> karg.serial_number[24-1] = '\0';

Does performance really matter in this context? How about the following instead:

snprintf(karg.serial_number, sizeof(karg.serial_number), "%*s", 
(int)(karg.serial_number) - 1, "");

Bart.

Re: [PATCH] mptfusion: use strlcpy() instead of strncpy()

[Andy Shevchenko]

On Fri, Jan 12, 2018 at 1:46 PM, Xiongfeng Wang
 wrote:
> From: Xiongfeng Wang 
>
> drivers/message/fusion/mptctl.c: In function '__mptctl_ioctl.isra.3':
> ./include/linux/string.h:245:9: warning: '__builtin_strncpy' specified
> bound 12 equals destination size [-Wstringop-truncation]
>
> The compiler requires that the destination size should be greater than
> the length we copy to make sure the dest string is nul-terminated. We
> can just use strlcpy() to avoid this warning.

Are you sure it's a best approach in this case?

> -   strncpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, 
> MPT_IOCTL_VERSION_LENGTH);
> -   karg->driverVersion[MPT_IOCTL_VERSION_LENGTH-1]='\0';
> +   strlcpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, 
> MPT_IOCTL_VERSION_LENGTH);

This one is false positive.

> -   strncpy (karg.name, ioc->name, MPT_MAX_NAME);
> -   karg.name[MPT_MAX_NAME-1]='\0';
> -   strncpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);
> -   karg.product[MPT_PRODUCT_LENGTH-1]='\0';
> +   strlcpy (karg.name, ioc->name, MPT_MAX_NAME);
> +   strlcpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);

These two as well.

> -   strncpy(karg.serial_number, " ", 24);
> +   strlcpy(karg.serial_number, " ", 24);

This one is interesting indeed.
Though the fix would be rather something like

memset(_number, " ", 24); // leave 24 for best performance of memset
karg.serial_number[24-1] = '\0';

> if (mpt_config(ioc, ) == 0) {
> ManufacturingPage0_t *pdata = 
> (ManufacturingPage0_t *) pbuf;

> if (strlen(pdata->BoardTracerNumber) 
> > 1) {
> -   strncpy(karg.serial_number,   
>   
> pdata->BoardTracerNumber, 24);
> -   karg.serial_number[24-1]='\0';
> +   strlcpy(karg.serial_number,
> +   
> pdata->BoardTracerNumber, 24);
> }

...and here you don't need to touch anything.

-- 
With Best Regards,
Andy Shevchenko

[PATCH] mptfusion: use strlcpy() instead of strncpy()

[Xiongfeng Wang]

From: Xiongfeng Wang 

drivers/message/fusion/mptctl.c: In function '__mptctl_ioctl.isra.3':
./include/linux/string.h:245:9: warning: '__builtin_strncpy' specified
bound 12 equals destination size [-Wstringop-truncation]

The compiler requires that the destination size should be greater than
the length we copy to make sure the dest string is nul-terminated. We
can just use strlcpy() to avoid this warning.

Signed-off-by: Xiongfeng Wang 
---
 drivers/message/fusion/mptctl.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index 7b3b413..15f2aca 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -1353,8 +1353,7 @@ static int mptctl_do_reset(unsigned long arg)
 
/* Set the Version Strings.
 */
-   strncpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, 
MPT_IOCTL_VERSION_LENGTH);
-   karg->driverVersion[MPT_IOCTL_VERSION_LENGTH-1]='\0';
+   strlcpy (karg->driverVersion, MPT_LINUX_PACKAGE_NAME, 
MPT_IOCTL_VERSION_LENGTH);
 
karg->busChangeEvent = 0;
karg->hostId = ioc->pfacts[port].PortSCSIID;
@@ -1542,10 +1541,8 @@ static int mptctl_do_reset(unsigned long arg)
 #else
karg.chip_type = ioc->pcidev->device;
 #endif
-   strncpy (karg.name, ioc->name, MPT_MAX_NAME);
-   karg.name[MPT_MAX_NAME-1]='\0';
-   strncpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);
-   karg.product[MPT_PRODUCT_LENGTH-1]='\0';
+   strlcpy (karg.name, ioc->name, MPT_MAX_NAME);
+   strlcpy (karg.product, ioc->prod_name, MPT_PRODUCT_LENGTH);
 
/* Copy the data from kernel memory to user memory
 */
@@ -2513,7 +2510,7 @@ static int mptctl_do_reset(unsigned long arg)
cfg.dir = 0;/* read */
cfg.timeout = 10;
 
-   strncpy(karg.serial_number, " ", 24);
+   strlcpy(karg.serial_number, " ", 24);
if (mpt_config(ioc, ) == 0) {
if (cfg.cfghdr.hdr->PageLength > 0) {
/* Issue the second config page request */
@@ -2525,8 +2522,8 @@ static int mptctl_do_reset(unsigned long arg)
if (mpt_config(ioc, ) == 0) {
ManufacturingPage0_t *pdata = 
(ManufacturingPage0_t *) pbuf;
if (strlen(pdata->BoardTracerNumber) > 
1) {
-   strncpy(karg.serial_number, 

pdata->BoardTracerNumber, 24);
-   karg.serial_number[24-1]='\0';
+   strlcpy(karg.serial_number,
+   
pdata->BoardTracerNumber, 24);
}
}
pci_free_consistent(ioc->pcidev, hdr.PageLength 
* 4, pbuf, buf_dma);
-- 
1.8.3.1