diff mbox series

[v2] usbip: tools: fix GCC8 warning for strncpy

Message ID 20190725132209.GA27590@jerryopenix (mailing list archive)
State Mainlined
Commit 6389a62ff798e781567645c0b0ca3dd7b8a4289d
Headers show
Series [v2] usbip: tools: fix GCC8 warning for strncpy | expand

Commit Message

Liu, Changcheng July 25, 2019, 1:22 p.m. UTC 
GCC8 started emitting warning about using strncpy with number of bytes
exactly equal destination size which could lead to non-zero terminated
string being copied. Use "SYSFS_PATH_MAX - 1" & "SYSFS_BUS_ID_SIZE - 1"
as number of bytes to ensure name is always zero-terminated.

Signed-off-by: Changcheng Liu <changcheng.liu@aliyun.com>
---
v1 -> v2:
 * Correct array tail index
---
 tools/usb/usbip/libsrc/usbip_common.c        | 6 ++++--
 tools/usb/usbip/libsrc/usbip_device_driver.c | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

Comments

Shuah July 25, 2019, 2:19 p.m. UTC | #1 
On 7/25/19 7:22 AM, Liu, Changcheng wrote:
> GCC8 started emitting warning about using strncpy with number of bytes
> exactly equal destination size which could lead to non-zero terminated
> string being copied. Use "SYSFS_PATH_MAX - 1" & "SYSFS_BUS_ID_SIZE - 1"
> as number of bytes to ensure name is always zero-terminated.
> 
> Signed-off-by: Changcheng Liu <changcheng.liu@aliyun.com>
> ---
> v1 -> v2:
>   * Correct array tail index
> ---
>   tools/usb/usbip/libsrc/usbip_common.c        | 6 ++++--
>   tools/usb/usbip/libsrc/usbip_device_driver.c | 6 ++++--
>   2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/usb/usbip/libsrc/usbip_common.c b/tools/usb/usbip/libsrc/usbip_common.c
> index bb424638d75b..b8d7d480595a 100644
> --- a/tools/usb/usbip/libsrc/usbip_common.c
> +++ b/tools/usb/usbip/libsrc/usbip_common.c
> @@ -226,8 +226,10 @@ int read_usb_device(struct udev_device *sdev, struct usbip_usb_device *udev)
>   	path = udev_device_get_syspath(sdev);
>   	name = udev_device_get_sysname(sdev);
>   
> -	strncpy(udev->path,  path,  SYSFS_PATH_MAX);
> -	strncpy(udev->busid, name, SYSFS_BUS_ID_SIZE);
> +	strncpy(udev->path,  path,  SYSFS_PATH_MAX - 1);
> +	udev->path[SYSFS_PATH_MAX - 1] = '\0';
> +	strncpy(udev->busid, name, SYSFS_BUS_ID_SIZE - 1);
> +	udev->busid[SYSFS_BUS_ID_SIZE - 1] = '\0';

strlcpy() would be better choice here. Any reason to not use that?

>   
>   	sscanf(name, "%u-%u", &busnum, &devnum);
>   	udev->busnum = busnum;
> diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
> index 5a3726eb44ab..051d7d3f443b 100644
> --- a/tools/usb/usbip/libsrc/usbip_device_driver.c
> +++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
> @@ -91,7 +91,8 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
>   	copy_descr_attr16(dev, &descr, idProduct);
>   	copy_descr_attr16(dev, &descr, bcdDevice);
>   
> -	strncpy(dev->path, path, SYSFS_PATH_MAX);
> +	strncpy(dev->path, path, SYSFS_PATH_MAX - 1);
> +	dev->path[SYSFS_PATH_MAX - 1] = '\0';
>   
>   	dev->speed = USB_SPEED_UNKNOWN;
>   	speed = udev_device_get_sysattr_value(sdev, "current_speed");
> @@ -110,7 +111,8 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
>   	dev->busnum = 0;
>   
>   	name = udev_device_get_sysname(plat);
> -	strncpy(dev->busid, name, SYSFS_BUS_ID_SIZE);
> +	strncpy(dev->busid, name, SYSFS_BUS_ID_SIZE - 1);
> +	dev->busid[SYSFS_BUS_ID_SIZE - 1] = '\0';

strlcpy() would be better choice here. Any reason to not use that?

>   	return 0;
>   err:
>   	fclose(fd);
> 

thanks,
-- Shuah
Liu, Changcheng July 25, 2019, 2:44 p.m. UTC | #2 
On 08:19 Thu 25 Jul, shuah wrote:
> On 7/25/19 7:22 AM, Liu, Changcheng wrote:
> > GCC8 started emitting warning about using strncpy with number of bytes
> > exactly equal destination size which could lead to non-zero terminated
> > string being copied. Use "SYSFS_PATH_MAX - 1" & "SYSFS_BUS_ID_SIZE - 1"
> > as number of bytes to ensure name is always zero-terminated.
> > 
> > Signed-off-by: Changcheng Liu <changcheng.liu@aliyun.com>
> > ---
> > v1 -> v2:
> >   * Correct array tail index
> > ---
> >   tools/usb/usbip/libsrc/usbip_common.c        | 6 ++++--
> >   tools/usb/usbip/libsrc/usbip_device_driver.c | 6 ++++--
> >   2 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/usb/usbip/libsrc/usbip_common.c b/tools/usb/usbip/libsrc/usbip_common.c
> > index bb424638d75b..b8d7d480595a 100644
> > --- a/tools/usb/usbip/libsrc/usbip_common.c
> > +++ b/tools/usb/usbip/libsrc/usbip_common.c
> > @@ -226,8 +226,10 @@ int read_usb_device(struct udev_device *sdev, struct usbip_usb_device *udev)
> >   	path = udev_device_get_syspath(sdev);
> >   	name = udev_device_get_sysname(sdev);
> > -	strncpy(udev->path,  path,  SYSFS_PATH_MAX);
> > -	strncpy(udev->busid, name, SYSFS_BUS_ID_SIZE);
> > +	strncpy(udev->path,  path,  SYSFS_PATH_MAX - 1);
> > +	udev->path[SYSFS_PATH_MAX - 1] = '\0';
> > +	strncpy(udev->busid, name, SYSFS_BUS_ID_SIZE - 1);
> > +	udev->busid[SYSFS_BUS_ID_SIZE - 1] = '\0';
> 
> strlcpy() would be better choice here. Any reason to not use that?
> 
@Shuah: linux tools link with libc which doesn't implment strlcpy yet.
So tools source code can't use strlcpy function like other kernel source
code.

> >   	sscanf(name, "%u-%u", &busnum, &devnum);
> >   	udev->busnum = busnum;
> > diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
> > index 5a3726eb44ab..051d7d3f443b 100644
> > --- a/tools/usb/usbip/libsrc/usbip_device_driver.c
> > +++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
> > @@ -91,7 +91,8 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
> >   	copy_descr_attr16(dev, &descr, idProduct);
> >   	copy_descr_attr16(dev, &descr, bcdDevice);
> > -	strncpy(dev->path, path, SYSFS_PATH_MAX);
> > +	strncpy(dev->path, path, SYSFS_PATH_MAX - 1);
> > +	dev->path[SYSFS_PATH_MAX - 1] = '\0';
> >   	dev->speed = USB_SPEED_UNKNOWN;
> >   	speed = udev_device_get_sysattr_value(sdev, "current_speed");
> > @@ -110,7 +111,8 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
> >   	dev->busnum = 0;
> >   	name = udev_device_get_sysname(plat);
> > -	strncpy(dev->busid, name, SYSFS_BUS_ID_SIZE);
> > +	strncpy(dev->busid, name, SYSFS_BUS_ID_SIZE - 1);
> > +	dev->busid[SYSFS_BUS_ID_SIZE - 1] = '\0';
> 
> strlcpy() would be better choice here. Any reason to not use that?
> 
> >   	return 0;
> >   err:
> >   	fclose(fd);
> > 
> 
> thanks,
> -- Shuah
Shuah July 25, 2019, 4:06 p.m. UTC | #3 
On 7/25/19 8:44 AM, Liu, Changcheng wrote:
> On 08:19 Thu 25 Jul, shuah wrote:
>> On 7/25/19 7:22 AM, Liu, Changcheng wrote:
>>> GCC8 started emitting warning about using strncpy with number of bytes
>>> exactly equal destination size which could lead to non-zero terminated
>>> string being copied. Use "SYSFS_PATH_MAX - 1" & "SYSFS_BUS_ID_SIZE - 1"
>>> as number of bytes to ensure name is always zero-terminated.
>>>
>>> Signed-off-by: Changcheng Liu <changcheng.liu@aliyun.com>
>>> ---
>>> v1 -> v2:
>>>    * Correct array tail index
>>> ---
>>>    tools/usb/usbip/libsrc/usbip_common.c        | 6 ++++--
>>>    tools/usb/usbip/libsrc/usbip_device_driver.c | 6 ++++--
>>>    2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/usb/usbip/libsrc/usbip_common.c b/tools/usb/usbip/libsrc/usbip_common.c
>>> index bb424638d75b..b8d7d480595a 100644
>>> --- a/tools/usb/usbip/libsrc/usbip_common.c
>>> +++ b/tools/usb/usbip/libsrc/usbip_common.c
>>> @@ -226,8 +226,10 @@ int read_usb_device(struct udev_device *sdev, struct usbip_usb_device *udev)
>>>    	path = udev_device_get_syspath(sdev);
>>>    	name = udev_device_get_sysname(sdev);
>>> -	strncpy(udev->path,  path,  SYSFS_PATH_MAX);
>>> -	strncpy(udev->busid, name, SYSFS_BUS_ID_SIZE);
>>> +	strncpy(udev->path,  path,  SYSFS_PATH_MAX - 1);
>>> +	udev->path[SYSFS_PATH_MAX - 1] = '\0';
>>> +	strncpy(udev->busid, name, SYSFS_BUS_ID_SIZE - 1);
>>> +	udev->busid[SYSFS_BUS_ID_SIZE - 1] = '\0';
>>
>> strlcpy() would be better choice here. Any reason to not use that?
>>
> @Shuah: linux tools link with libc which doesn't implment strlcpy yet.
> So tools source code can't use strlcpy function like other kernel source
> code.

Right I keep forgetting that. :)
Thinking about it, udev_device_get_syspath() could return
null. Not related to this change anyway.

Thanks for fixing this.

Greg! Please pick this up:

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah
diff mbox series

Patch

diff --git a/tools/usb/usbip/libsrc/usbip_common.c b/tools/usb/usbip/libsrc/usbip_common.c
index bb424638d75b..b8d7d480595a 100644
--- a/tools/usb/usbip/libsrc/usbip_common.c
+++ b/tools/usb/usbip/libsrc/usbip_common.c
@@ -226,8 +226,10 @@  int read_usb_device(struct udev_device *sdev, struct usbip_usb_device *udev)
 	path = udev_device_get_syspath(sdev);
 	name = udev_device_get_sysname(sdev);
 
-	strncpy(udev->path,  path,  SYSFS_PATH_MAX);
-	strncpy(udev->busid, name, SYSFS_BUS_ID_SIZE);
+	strncpy(udev->path,  path,  SYSFS_PATH_MAX - 1);
+	udev->path[SYSFS_PATH_MAX - 1] = '\0';
+	strncpy(udev->busid, name, SYSFS_BUS_ID_SIZE - 1);
+	udev->busid[SYSFS_BUS_ID_SIZE - 1] = '\0';
 
 	sscanf(name, "%u-%u", &busnum, &devnum);
 	udev->busnum = busnum;
diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
index 5a3726eb44ab..051d7d3f443b 100644
--- a/tools/usb/usbip/libsrc/usbip_device_driver.c
+++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
@@ -91,7 +91,8 @@  int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
 	copy_descr_attr16(dev, &descr, idProduct);
 	copy_descr_attr16(dev, &descr, bcdDevice);
 
-	strncpy(dev->path, path, SYSFS_PATH_MAX);
+	strncpy(dev->path, path, SYSFS_PATH_MAX - 1);
+	dev->path[SYSFS_PATH_MAX - 1] = '\0';
 
 	dev->speed = USB_SPEED_UNKNOWN;
 	speed = udev_device_get_sysattr_value(sdev, "current_speed");
@@ -110,7 +111,8 @@  int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
 	dev->busnum = 0;
 
 	name = udev_device_get_sysname(plat);
-	strncpy(dev->busid, name, SYSFS_BUS_ID_SIZE);
+	strncpy(dev->busid, name, SYSFS_BUS_ID_SIZE - 1);
+	dev->busid[SYSFS_BUS_ID_SIZE - 1] = '\0';
 	return 0;
 err:
 	fclose(fd);