* [PATCH 1/2] USB: core: Fix build warning in usb_get_configuration()
@ 2020-02-26 6:15 Tiezhu Yang
2020-02-26 6:15 ` [PATCH 2/2] USB: core: Fix potential memory leak " Tiezhu Yang
2020-02-26 8:04 ` [PATCH 1/2] USB: core: Fix build warning " Johan Hovold
0 siblings, 2 replies; 8+ messages in thread
From: Tiezhu Yang @ 2020-02-26 6:15 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel, Xuefeng Li
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] USB: core: Fix potential memory leak in usb_get_configuration()
2020-02-26 6:15 [PATCH 1/2] USB: core: Fix build warning in usb_get_configuration() Tiezhu Yang
@ 2020-02-26 6:15 ` Tiezhu Yang
2020-02-26 8:09 ` Johan Hovold
2020-02-26 8:04 ` [PATCH 1/2] USB: core: Fix build warning " Johan Hovold
1 sibling, 1 reply; 8+ messages in thread
From: Tiezhu Yang @ 2020-02-26 6:15 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel, Xuefeng Li
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] USB: core: Fix build warning in usb_get_configuration()
2020-02-26 6:15 [PATCH 1/2] USB: core: Fix build warning in usb_get_configuration() Tiezhu Yang
2020-02-26 6:15 ` [PATCH 2/2] USB: core: Fix potential memory leak " Tiezhu Yang
@ 2020-02-26 8:04 ` Johan Hovold
2020-02-26 8:35 ` Tiezhu Yang
1 sibling, 1 reply; 8+ messages in thread
From: Johan Hovold @ 2020-02-26 8:04 UTC (permalink / raw)
To: Tiezhu Yang; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Xuefeng Li
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] USB: core: Fix potential memory leak in usb_get_configuration()
2020-02-26 6:15 ` [PATCH 2/2] USB: core: Fix potential memory leak " Tiezhu Yang
@ 2020-02-26 8:09 ` Johan Hovold
2020-02-26 8:42 ` Tiezhu Yang
0 siblings, 1 reply; 8+ messages in thread
From: Johan Hovold @ 2020-02-26 8:09 UTC (permalink / raw)
To: Tiezhu Yang; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Xuefeng Li
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] USB: core: Fix build warning in usb_get_configuration()
2020-02-26 8:04 ` [PATCH 1/2] USB: core: Fix build warning " Johan Hovold
@ 2020-02-26 8:35 ` Tiezhu Yang
2020-02-26 10:09 ` Johan Hovold
0 siblings, 1 reply; 8+ messages in thread
From: Tiezhu Yang @ 2020-02-26 8:35 UTC (permalink / raw)
To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Xuefeng Li
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] USB: core: Fix potential memory leak in usb_get_configuration()
2020-02-26 8:09 ` Johan Hovold
@ 2020-02-26 8:42 ` Tiezhu Yang
2020-02-26 10:12 ` Johan Hovold
0 siblings, 1 reply; 8+ messages in thread
From: Tiezhu Yang @ 2020-02-26 8:42 UTC (permalink / raw)
To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Xuefeng Li
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] USB: core: Fix build warning in usb_get_configuration()
2020-02-26 8:35 ` Tiezhu Yang
@ 2020-02-26 10:09 ` Johan Hovold
0 siblings, 0 replies; 8+ messages in thread
From: Johan Hovold @ 2020-02-26 10:09 UTC (permalink / raw)
To: Tiezhu Yang
Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel, Xuefeng Li
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] USB: core: Fix potential memory leak in usb_get_configuration()
2020-02-26 8:42 ` Tiezhu Yang
@ 2020-02-26 10:12 ` Johan Hovold
0 siblings, 0 replies; 8+ messages in thread
From: Johan Hovold @ 2020-02-26 10:12 UTC (permalink / raw)
To: Tiezhu Yang
Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel, Xuefeng Li
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-02-26 10:12 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-26 6:15 [PATCH 1/2] USB: core: Fix build warning in usb_get_configuration() Tiezhu Yang
2020-02-26 6:15 ` [PATCH 2/2] USB: core: Fix potential memory leak " Tiezhu Yang
2020-02-26 8:09 ` Johan Hovold
2020-02-26 8:42 ` Tiezhu Yang
2020-02-26 10:12 ` Johan Hovold
2020-02-26 8:04 ` [PATCH 1/2] USB: core: Fix build warning " Johan Hovold
2020-02-26 8:35 ` Tiezhu Yang
2020-02-26 10:09 ` Johan Hovold
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).