[PATCH 1/2] USB: core: Fix build warning in usb_get_configuration()

[PATCH 1/2] USB: core: Fix build warning in usb_get_configuration()

[Tiezhu Yang]

There is no functional issue, just fix the following build warning:

  CC      drivers/usb/core/config.o
drivers/usb/core/config.c: In function ‘usb_get_configuration’:
drivers/usb/core/config.c:868:6: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  int result;
      ^

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 drivers/usb/core/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index b7918f6..bb63ee0 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -865,7 +865,7 @@ int usb_get_configuration(struct usb_device *dev)
 	unsigned int cfgno, length;
 	unsigned char *bigbuffer;
 	struct usb_config_descriptor *desc;
-	int result;
+	int result = 0;
 
 	if (ncfg > USB_MAXCONFIG) {
 		dev_warn(ddev, "too many configurations: %d, "
-- 
2.1.0

[PATCH 2/2] USB: core: Fix potential memory leak in usb_get_configuration()

[Tiezhu Yang]

Make sure to free all the allocated memory before exiting from the function
usb_get_configuration() when an error is encountered.

Additionally, just initialize the variable "bigbuffer" with NULL to avoid
the following build warning:

  CC      drivers/usb/core/config.o
drivers/usb/core/config.c: In function ‘usb_get_configuration’:
drivers/usb/core/config.c:956:2: warning: ‘bigbuffer’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  kfree(bigbuffer);
  ^

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 drivers/usb/core/config.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index bb63ee0..3763390 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -863,7 +863,7 @@ int usb_get_configuration(struct usb_device *dev)
 	struct device *ddev = &dev->dev;
 	int ncfg = dev->descriptor.bNumConfigurations;
 	unsigned int cfgno, length;
-	unsigned char *bigbuffer;
+	unsigned char *bigbuffer = NULL;
 	struct usb_config_descriptor *desc;
 	int result = 0;
 
@@ -885,12 +885,16 @@ int usb_get_configuration(struct usb_device *dev)
 
 	length = ncfg * sizeof(char *);
 	dev->rawdescriptors = kzalloc(length, GFP_KERNEL);
-	if (!dev->rawdescriptors)
-		return -ENOMEM;
+	if (!dev->rawdescriptors) {
+		result = -ENOMEM;
+		goto err_rawdescriptors;
+	}
 
 	desc = kmalloc(USB_DT_CONFIG_SIZE, GFP_KERNEL);
-	if (!desc)
-		return -ENOMEM;
+	if (!desc) {
+		result = -ENOMEM;
+		goto err_desc;
+	}
 
 	for (cfgno = 0; cfgno < ncfg; cfgno++) {
 		/* We grab just the first descriptor so we know how long
@@ -930,8 +934,7 @@ int usb_get_configuration(struct usb_device *dev)
 		if (result < 0) {
 			dev_err(ddev, "unable to read config index %d "
 			    "descriptor/%s\n", cfgno, "all");
-			kfree(bigbuffer);
-			goto err;
+			goto out;
 		}
 		if (result < length) {
 			dev_warn(ddev, "config index %d descriptor too short "
@@ -945,13 +948,19 @@ int usb_get_configuration(struct usb_device *dev)
 		    &dev->config[cfgno], bigbuffer, length);
 		if (result < 0) {
 			++cfgno;
-			goto err;
+			goto out;
 		}
 	}
 
+out:
+	kfree(bigbuffer);
 err:
 	kfree(desc);
 	dev->descriptor.bNumConfigurations = cfgno;
+err_desc:
+	kfree(dev->rawdescriptors);
+err_rawdescriptors:
+	kfree(dev->config);
 
 	return result;
 }
-- 
2.1.0

Re: [PATCH 1/2] USB: core: Fix build warning in usb_get_configuration()

[Johan Hovold]

On Wed, Feb 26, 2020 at 02:15:22PM +0800, Tiezhu Yang wrote:
> There is no functional issue, just fix the following build warning:
> 
>   CC      drivers/usb/core/config.o
> drivers/usb/core/config.c: In function ‘usb_get_configuration’:
> drivers/usb/core/config.c:868:6: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   int result;
>       ^

What compiler are you using? The warning is clearly bogus and it hasn't
been seen with any recent gcc at least.

> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  drivers/usb/core/config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index b7918f6..bb63ee0 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -865,7 +865,7 @@ int usb_get_configuration(struct usb_device *dev)
>  	unsigned int cfgno, length;
>  	unsigned char *bigbuffer;
>  	struct usb_config_descriptor *desc;
> -	int result;
> +	int result = 0;
>  
>  	if (ncfg > USB_MAXCONFIG) {
>  		dev_warn(ddev, "too many configurations: %d, "

Johan

Re: [PATCH 2/2] USB: core: Fix potential memory leak in usb_get_configuration()

[Johan Hovold]

On Wed, Feb 26, 2020 at 02:15:23PM +0800, Tiezhu Yang wrote:
> Make sure to free all the allocated memory before exiting from the function
> usb_get_configuration() when an error is encountered.

There's no leak in this function as far as I can tell. Any allocated
memory is released in usb_destroy_configuration() when the last
reference to the struct usb_device is dropped.

> Additionally, just initialize the variable "bigbuffer" with NULL to avoid
> the following build warning:
> 
>   CC      drivers/usb/core/config.o
> drivers/usb/core/config.c: In function ‘usb_get_configuration’:
> drivers/usb/core/config.c:956:2: warning: ‘bigbuffer’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   kfree(bigbuffer);
>   ^

No need to mention warnings that you introduce yourself while creating
your patch. It can give the false impression that your addressing an
existing issue.

Johan

Re: [PATCH 1/2] USB: core: Fix build warning in usb_get_configuration()

[Tiezhu Yang]

On 02/26/2020 04:04 PM, Johan Hovold wrote:
> On Wed, Feb 26, 2020 at 02:15:22PM +0800, Tiezhu Yang wrote:
>> There is no functional issue, just fix the following build warning:
>>
>>    CC      drivers/usb/core/config.o
>> drivers/usb/core/config.c: In function ‘usb_get_configuration’:
>> drivers/usb/core/config.c:868:6: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>    int result;
>>        ^
> What compiler are you using? The warning is clearly bogus and it hasn't
> been seen with any recent gcc at least.

[yangtiezhu@linux ~]$ gcc --version
gcc (GCC) 4.9.4 20160726 (Red Hat 4.9.4-14)
Copyright (C) 2015 Free Software Foundation, Inc.

The gcc version I used maybe too old,
if the warning is bogus, please ignore this patch.

Thanks,

Tiezhu Yang

>
>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>> ---
>>   drivers/usb/core/config.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
>> index b7918f6..bb63ee0 100644
>> --- a/drivers/usb/core/config.c
>> +++ b/drivers/usb/core/config.c
>> @@ -865,7 +865,7 @@ int usb_get_configuration(struct usb_device *dev)
>>   	unsigned int cfgno, length;
>>   	unsigned char *bigbuffer;
>>   	struct usb_config_descriptor *desc;
>> -	int result;
>> +	int result = 0;
>>   
>>   	if (ncfg > USB_MAXCONFIG) {
>>   		dev_warn(ddev, "too many configurations: %d, "
> Johan

Re: [PATCH 2/2] USB: core: Fix potential memory leak in usb_get_configuration()

[Tiezhu Yang]

On 02/26/2020 04:09 PM, Johan Hovold wrote:
> On Wed, Feb 26, 2020 at 02:15:23PM +0800, Tiezhu Yang wrote:
>> Make sure to free all the allocated memory before exiting from the function
>> usb_get_configuration() when an error is encountered.
> There's no leak in this function as far as I can tell. Any allocated
> memory is released in usb_destroy_configuration() when the last
> reference to the struct usb_device is dropped.

Yes, you are right, the allocated memory in usb_get_configuration()
will be released in usb_destroy_configuration().

By the way, is it better to release the allocated memory as early as 
possible
in usb_get_configuration()? Just like this:

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index bb63ee0..dd4ebeb 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -885,12 +885,17 @@ int usb_get_configuration(struct usb_device *dev)

         length = ncfg * sizeof(char *);
         dev->rawdescriptors = kzalloc(length, GFP_KERNEL);
-       if (!dev->rawdescriptors)
+       if (!dev->rawdescriptors) {
+               kfree(dev->config);
                 return -ENOMEM;
+       }

         desc = kmalloc(USB_DT_CONFIG_SIZE, GFP_KERNEL);
-       if (!desc)
+       if (!desc) {
+               kfree(dev->config);
+               kfree(dev->rawdescriptors);
                 return -ENOMEM;
+       }

         for (cfgno = 0; cfgno < ncfg; cfgno++) {
                 /* We grab just the first descriptor so we know how long

Thanks,

Tiezhu Yang

>
>> Additionally, just initialize the variable "bigbuffer" with NULL to avoid
>> the following build warning:
>>
>>    CC      drivers/usb/core/config.o
>> drivers/usb/core/config.c: In function ‘usb_get_configuration’:
>> drivers/usb/core/config.c:956:2: warning: ‘bigbuffer’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>    kfree(bigbuffer);
>>    ^
> No need to mention warnings that you introduce yourself while creating
> your patch. It can give the false impression that your addressing an
> existing issue.
>
> Johan

Re: [PATCH 1/2] USB: core: Fix build warning in usb_get_configuration()

[Johan Hovold]

On Wed, Feb 26, 2020 at 04:35:48PM +0800, Tiezhu Yang wrote:
> On 02/26/2020 04:04 PM, Johan Hovold wrote:
> > On Wed, Feb 26, 2020 at 02:15:22PM +0800, Tiezhu Yang wrote:
> >> There is no functional issue, just fix the following build warning:
> >>
> >>    CC      drivers/usb/core/config.o
> >> drivers/usb/core/config.c: In function ‘usb_get_configuration’:
> >> drivers/usb/core/config.c:868:6: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >>    int result;
> >>        ^
> > What compiler are you using? The warning is clearly bogus and it hasn't
> > been seen with any recent gcc at least.
> 
> [yangtiezhu@linux ~]$ gcc --version
> gcc (GCC) 4.9.4 20160726 (Red Hat 4.9.4-14)
> Copyright (C) 2015 Free Software Foundation, Inc.
> 
> The gcc version I used maybe too old,
> if the warning is bogus, please ignore this patch.

Hmm. I even tried installing 4.9.4 and still don't see that warning (on
x86).

Are you sure that's the compiler version you use?

We've silenced bogus maybe-uninitialized warnings for 4.9 and newer
before, but we shouldn't be adding compiler workarounds unless we have
to.

Johan

Re: [PATCH 2/2] USB: core: Fix potential memory leak in usb_get_configuration()

[Johan Hovold]

On Wed, Feb 26, 2020 at 04:42:55PM +0800, Tiezhu Yang wrote:
> On 02/26/2020 04:09 PM, Johan Hovold wrote:
> > On Wed, Feb 26, 2020 at 02:15:23PM +0800, Tiezhu Yang wrote:
> >> Make sure to free all the allocated memory before exiting from the function
> >> usb_get_configuration() when an error is encountered.
> > There's no leak in this function as far as I can tell. Any allocated
> > memory is released in usb_destroy_configuration() when the last
> > reference to the struct usb_device is dropped.
> 
> Yes, you are right, the allocated memory in usb_get_configuration()
> will be released in usb_destroy_configuration().
> 
> By the way, is it better to release the allocated memory as early as 
> possible
> in usb_get_configuration()? Just like this:
> 
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index bb63ee0..dd4ebeb 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -885,12 +885,17 @@ int usb_get_configuration(struct usb_device *dev)
> 
>          length = ncfg * sizeof(char *);
>          dev->rawdescriptors = kzalloc(length, GFP_KERNEL);
> -       if (!dev->rawdescriptors)
> +       if (!dev->rawdescriptors) {
> +               kfree(dev->config);
>                  return -ENOMEM;
> +       }

No, there's no point in that. And just like your original proposal, this
would also introduce a double free.

Johan