| Message ID | 1515757619-29651-1-git-send-email-wangxiongfeng2@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | mptfusion: use strlcpy() instead of strncpy() | expand |
On Fri, Jan 12, 2018 at 1:46 PM, Xiongfeng Wang <wangxiongfeng2@huawei.com> wrote: > From: Xiongfeng Wang <xiongfeng.wang@linaro.org> > > 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(&karg.serial_number, " ", 24); // leave 24 for best performance of memset karg.serial_number[24-1] = '\0'; > if (mpt_config(ioc, &cfg) == 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
On Fri, 2018-01-12 at 15:45 +0200, Andy Shevchenko wrote: > On Fri, Jan 12, 2018 at 1:46 PM, Wang <wangxiongfeng2@huawei.com> 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(&karg.serial_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.
On Fri, Jan 12, 2018 at 5:09 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Fri, 2018-01-12 at 15:45 +0200, Andy Shevchenko wrote: >> On Fri, Jan 12, 2018 at 1:46 PM, Wang <wangxiongfeng2@huawei.com> 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(&karg.serial_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
On 2018/1/16 3:34, Andy Shevchenko wrote: > On Fri, Jan 12, 2018 at 5:09 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: >> On Fri, 2018-01-12 at 15:45 +0200, Andy Shevchenko wrote: >>> On Fri, Jan 12, 2018 at 1:46 PM, Wang <wangxiongfeng2@huawei.com> 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(&karg.serial_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
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, &cfg) == 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, &cfg) == 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);