[PATCH] usbip: tools: Fix read_usb_vudc_device() error path handling

[PATCH] usbip: tools: Fix read_usb_vudc_device() error path handling

[GwanYeong Kim]

cannot be less than 0 - fread() returns 0 on error.

Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
---
 tools/usb/usbip/libsrc/usbip_device_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
index 051d7d3f443b..49760b98aabc 100644
--- a/tools/usb/usbip/libsrc/usbip_device_driver.c
+++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
@@ -79,7 +79,7 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
 	if (!fd)
 		return -1;
 	ret = fread((char *) &descr, sizeof(descr), 1, fd);
-	if (ret < 0)
+	if (ret != sizeof(descr))
 		goto err;
 	fclose(fd);
 
-- 
2.17.1

Re: [PATCH] usbip: tools: Fix read_usb_vudc_device() error path handling

[shuah]

On 10/15/19 7:14 AM, GwanYeong Kim wrote:
> cannot be less than 0 - fread() returns 0 on error.
> 
> Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
> ---
>   tools/usb/usbip/libsrc/usbip_device_driver.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
> index 051d7d3f443b..49760b98aabc 100644
> --- a/tools/usb/usbip/libsrc/usbip_device_driver.c
> +++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
> @@ -79,7 +79,7 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
>   	if (!fd)
>   		return -1;
>   	ret = fread((char *) &descr, sizeof(descr), 1, fd);
> -	if (ret < 0) > +	if (ret != sizeof(descr))

Are you sure this check is correct? fread() returns the number
of elements read, # elements = 1  in this case.

fread() returns 0 when size or # of elements is 0 and in other
error cases it will return < # of elements. I would think you
want to check ret != 1 here.

thanks,
-- Shuah

Re: [PATCH] usbip: tools: Fix read_usb_vudc_device() error path handling

[GwanYeong Kim]

On Tue, 15 Oct 2019 17:14:32 -0600
shuah <shuah@kernel.org> wrote:

> On 10/15/19 7:14 AM, GwanYeong Kim wrote:
> > cannot be less than 0 - fread() returns 0 on error.
> > 
> > Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
> > ---
> >   tools/usb/usbip/libsrc/usbip_device_driver.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c
> > b/tools/usb/usbip/libsrc/usbip_device_driver.c index
> > 051d7d3f443b..49760b98aabc 100644 ---
> > a/tools/usb/usbip/libsrc/usbip_device_driver.c +++
> > b/tools/usb/usbip/libsrc/usbip_device_driver.c @@ -79,7 +79,7 @@
> > int read_usb_vudc_device(struct udev_device *sdev, struct
> > usbip_usb_device *dev) if (!fd) return -1;
> >   	ret = fread((char *) &descr, sizeof(descr), 1, fd);
> > -	if (ret < 0) > +	if (ret != sizeof(descr))
> 
> Are you sure this check is correct? fread() returns the number
> of elements read, # elements = 1  in this case.

Thank you for pointing this out.
Sorry for my mistake.

> fread() returns 0 when size or # of elements is 0 and in other
> error cases it will return < # of elements. I would think you
> want to check ret != 1 here.

You're right.
This should be changed to "ret != 1".

Should I send a new patch?

Regards,
GwanYeong Kim

Re: [PATCH] usbip: tools: Fix read_usb_vudc_device() error path handling

[Greg KH]

On Wed, Oct 16, 2019 at 01:38:25PM +0900, GwanYeong Kim wrote:
> On Tue, 15 Oct 2019 17:14:32 -0600
> shuah <shuah@kernel.org> wrote:
> 
> > On 10/15/19 7:14 AM, GwanYeong Kim wrote:
> > > cannot be less than 0 - fread() returns 0 on error.
> > > 
> > > Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
> > > ---
> > >   tools/usb/usbip/libsrc/usbip_device_driver.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c
> > > b/tools/usb/usbip/libsrc/usbip_device_driver.c index
> > > 051d7d3f443b..49760b98aabc 100644 ---
> > > a/tools/usb/usbip/libsrc/usbip_device_driver.c +++
> > > b/tools/usb/usbip/libsrc/usbip_device_driver.c @@ -79,7 +79,7 @@
> > > int read_usb_vudc_device(struct udev_device *sdev, struct
> > > usbip_usb_device *dev) if (!fd) return -1;
> > >   	ret = fread((char *) &descr, sizeof(descr), 1, fd);
> > > -	if (ret < 0) > +	if (ret != sizeof(descr))
> > 
> > Are you sure this check is correct? fread() returns the number
> > of elements read, # elements = 1  in this case.
> 
> Thank you for pointing this out.
> Sorry for my mistake.
> 
> > fread() returns 0 when size or # of elements is 0 and in other
> > error cases it will return < # of elements. I would think you
> > want to check ret != 1 here.
> 
> You're right.
> This should be changed to "ret != 1".
> 
> Should I send a new patch?

If you want to have it applied, yes.

thanks,

greg k-h

[PATCH v2] usbip: tools: Fix read_usb_vudc_device() error path handling

[GwanYeong Kim]

cannot be less than 0 - fread() returns 0 on error.

Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
---
 tools/usb/usbip/libsrc/usbip_device_driver.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
index 051d7d3f443b..959bb29d0477 100644
--- a/tools/usb/usbip/libsrc/usbip_device_driver.c
+++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
@@ -69,7 +69,7 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
 	FILE *fd = NULL;
 	struct udev_device *plat;
 	const char *speed;
-	int ret = 0;
+	size_t ret = 0;
 
 	plat = udev_device_get_parent(sdev);
 	path = udev_device_get_syspath(plat);
@@ -79,7 +79,7 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
 	if (!fd)
 		return -1;
 	ret = fread((char *) &descr, sizeof(descr), 1, fd);
-	if (ret < 0)
+	if (ret != 1)
 		goto err;
 	fclose(fd);
 
-- 
2.17.1

Re: [PATCH v2] usbip: tools: Fix read_usb_vudc_device() error path handling

[shuah]

On 10/16/19 8:25 PM, GwanYeong Kim wrote:
> cannot be less than 0 - fread() returns 0 on error.
> 

This isn't really accurate right. fread() doesn't always
return 0 in error. It could return < number of elements
and set errno.

Please make changes to reflect that.

> Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
> ---
>   tools/usb/usbip/libsrc/usbip_device_driver.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
> index 051d7d3f443b..959bb29d0477 100644
> --- a/tools/usb/usbip/libsrc/usbip_device_driver.c
> +++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
> @@ -69,7 +69,7 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
>   	FILE *fd = NULL;
>   	struct udev_device *plat;
>   	const char *speed;
> -	int ret = 0;
> +	size_t ret = 0;

You don't need to initialize this.

>   
>   	plat = udev_device_get_parent(sdev);
>   	path = udev_device_get_syspath(plat);
> @@ -79,7 +79,7 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
>   	if (!fd)
>   		return -1;
>   	ret = fread((char *) &descr, sizeof(descr), 1, fd);
> -	if (ret < 0)
> +	if (ret != 1)

Why not print error message?

>   		goto err;
>   	fclose(fd);
>   
> 

thanks,
-- Shuah

Re: [PATCH v2] usbip: tools: Fix read_usb_vudc_device() error path handling

[GwanYeong Kim]

On Wed, 16 Oct 2019 20:33:39 -0600
shuah <shuah@kernel.org> wrote:

> On 10/16/19 8:25 PM, GwanYeong Kim wrote:
> > cannot be less than 0 - fread() returns 0 on error.
> > 
> 
> This isn't really accurate right. fread() doesn't always
> return 0 in error. It could return < number of elements
> and set errno.
> 
> Please make changes to reflect that.

Will reflect this.

> 
> > Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
> > ---
> >   tools/usb/usbip/libsrc/usbip_device_driver.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c
> > b/tools/usb/usbip/libsrc/usbip_device_driver.c index
> > 051d7d3f443b..959bb29d0477 100644 ---
> > a/tools/usb/usbip/libsrc/usbip_device_driver.c +++
> > b/tools/usb/usbip/libsrc/usbip_device_driver.c @@ -69,7 +69,7 @@
> > int read_usb_vudc_device(struct udev_device *sdev, struct
> > usbip_usb_device *dev) FILE *fd = NULL; struct udev_device *plat;
> >   	const char *speed;
> > -	int ret = 0;
> > +	size_t ret = 0;
> 
> You don't need to initialize this.

Exactly, because "ret" variable receives a "fread()" return value,

> 
> >   
> >   	plat = udev_device_get_parent(sdev);
> >   	path = udev_device_get_syspath(plat);
> > @@ -79,7 +79,7 @@ int read_usb_vudc_device(struct udev_device
> > *sdev, struct usbip_usb_device *dev) if (!fd)
> >   		return -1;
> >   	ret = fread((char *) &descr, sizeof(descr), 1, fd);
> > -	if (ret < 0)
> > +	if (ret != 1)
> 
> Why not print error message?


Sorry, I'll add a message.

How about this?

err("Cannot read vudc device descr file");


> 
> >   		goto err;
> >   	fclose(fd);
> >   
> > 
> 
> thanks,
> -- Shuah

Re: [PATCH v2] usbip: tools: Fix read_usb_vudc_device() error path handling

[shuah]

On 10/16/19 11:26 PM, GwanYeong Kim wrote:
> On Wed, 16 Oct 2019 20:33:39 -0600
> shuah <shuah@kernel.org> wrote:
> 
>> On 10/16/19 8:25 PM, GwanYeong Kim wrote:
>>> cannot be less than 0 - fread() returns 0 on error.
>>>
>>
>> This isn't really accurate right. fread() doesn't always
>> return 0 in error. It could return < number of elements
>> and set errno.
>>
>> Please make changes to reflect that.
> 
> Will reflect this.
> 
>>
>>> Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
>>> ---
>>>    tools/usb/usbip/libsrc/usbip_device_driver.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c
>>> b/tools/usb/usbip/libsrc/usbip_device_driver.c index
>>> 051d7d3f443b..959bb29d0477 100644 ---
>>> a/tools/usb/usbip/libsrc/usbip_device_driver.c +++
>>> b/tools/usb/usbip/libsrc/usbip_device_driver.c @@ -69,7 +69,7 @@
>>> int read_usb_vudc_device(struct udev_device *sdev, struct
>>> usbip_usb_device *dev) FILE *fd = NULL; struct udev_device *plat;
>>>    	const char *speed;
>>> -	int ret = 0;
>>> +	size_t ret = 0;
>>
>> You don't need to initialize this.
> 
> Exactly, because "ret" variable receives a "fread()" return value,
> 
>>
>>>    
>>>    	plat = udev_device_get_parent(sdev);
>>>    	path = udev_device_get_syspath(plat);
>>> @@ -79,7 +79,7 @@ int read_usb_vudc_device(struct udev_device
>>> *sdev, struct usbip_usb_device *dev) if (!fd)
>>>    		return -1;
>>>    	ret = fread((char *) &descr, sizeof(descr), 1, fd);
>>> -	if (ret < 0)
>>> +	if (ret != 1)
>>
>> Why not print error message?
> 
> 
> Sorry, I'll add a message.
> 
> How about this?
> 
> err("Cannot read vudc device descr file");

Using strerror() with errno gives you more information. Add that
to them essage you proposed.

thanks,
-- Shuah

[PATCH v3] usbip: tools: Fix read_usb_vudc_device() error path handling

[GwanYeong Kim]

This isn't really accurate right. fread() doesn't always
return 0 in error. It could return < number of elements
and set errno.

Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
---
 tools/usb/usbip/libsrc/usbip_device_driver.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
index 051d7d3f443b..927a151fa9aa 100644
--- a/tools/usb/usbip/libsrc/usbip_device_driver.c
+++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
@@ -69,7 +69,7 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
 	FILE *fd = NULL;
 	struct udev_device *plat;
 	const char *speed;
-	int ret = 0;
+	size_t ret;
 
 	plat = udev_device_get_parent(sdev);
 	path = udev_device_get_syspath(plat);
@@ -79,8 +79,10 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
 	if (!fd)
 		return -1;
 	ret = fread((char *) &descr, sizeof(descr), 1, fd);
-	if (ret < 0)
+	if (ret != 1) {
+		err("Cannot read vudc device descr file: %s", strerror(errno));
 		goto err;
+	}
 	fclose(fd);
 
 	copy_descr_attr(dev, &descr, bDeviceClass);
-- 
2.17.1

Re: [PATCH v3] usbip: tools: Fix read_usb_vudc_device() error path handling

[shuah]

On 10/17/19 9:22 PM, GwanYeong Kim wrote:
> This isn't really accurate right. fread() doesn't always
> return 0 in error. It could return < number of elements
> and set errno.
> 
> Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
> ---
>   tools/usb/usbip/libsrc/usbip_device_driver.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
> index 051d7d3f443b..927a151fa9aa 100644
> --- a/tools/usb/usbip/libsrc/usbip_device_driver.c
> +++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
> @@ -69,7 +69,7 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
>   	FILE *fd = NULL;
>   	struct udev_device *plat;
>   	const char *speed;
> -	int ret = 0;
> +	size_t ret;
>   
>   	plat = udev_device_get_parent(sdev);
>   	path = udev_device_get_syspath(plat);
> @@ -79,8 +79,10 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
>   	if (!fd)
>   		return -1;
>   	ret = fread((char *) &descr, sizeof(descr), 1, fd);
> -	if (ret < 0)
> +	if (ret != 1) {
> +		err("Cannot read vudc device descr file: %s", strerror(errno));
>   		goto err;
> +	}
>   	fclose(fd);
>   
>   	copy_descr_attr(dev, &descr, bDeviceClass);
> 

Looks good.

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

thanks,
-- Shuah