Message ID | 20210524173644.GA116662@hyeyoo (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ALSA: line6: Improve poor error handling in line6_init_cap_control | expand |
added an error handling code. It builds well, but I don't have the device. So please review it (and test if you have one) if you think I can improve something, please tell me. I'll send v2. Thanks, Hyeonggon
On Mon, 24 May 2021 19:36:44 +0200, Hyeonggon Yoo wrote: > > line6_init_cap_control doesn't free resources properly when allocations > like kmalloc, usb_alloc_urb fails. It can cause memory leak when some > allocations succeed, and then an error occurs. > > This patch makes line6_init_cap_control to properly free the resources, > freeing previously allocated resources when there's an error. > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > --- > sound/usb/line6/driver.c | 36 +++++++++++++++++++++++++++--------- > 1 file changed, 27 insertions(+), 9 deletions(-) > > diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c > index 9602929b7de9..6991cb855023 100644 > --- a/sound/usb/line6/driver.c > +++ b/sound/usb/line6/driver.c > @@ -688,34 +688,52 @@ static int line6_init_cap_control(struct usb_line6 *line6) > > /* initialize USB buffers: */ > line6->buffer_listen = kmalloc(LINE6_BUFSIZE_LISTEN, GFP_KERNEL); > - if (!line6->buffer_listen) > - return -ENOMEM; > + if (!line6->buffer_listen) { > + ret = -ENOMEM; > + goto fail_ret; > + } > > line6->urb_listen = usb_alloc_urb(0, GFP_KERNEL); > - if (!line6->urb_listen) > - return -ENOMEM; > + if (!line6->urb_listen) { > + ret = -ENOMEM; > + goto fail1; > + } > > if (line6->properties->capabilities & LINE6_CAP_CONTROL_MIDI) { > line6->buffer_message = kmalloc(LINE6_MIDI_MESSAGE_MAXLEN, GFP_KERNEL); > - if (!line6->buffer_message) > - return -ENOMEM; > + if (!line6->buffer_message) { > + ret = -ENOMEM; > + goto fail2; > + } > > ret = line6_init_midi(line6); > if (ret < 0) > - return ret; > + goto fail3; > } else { > ret = line6_hwdep_init(line6); > if (ret < 0) > - return ret; > + goto fail2; > } > > ret = line6_start_listen(line6); > if (ret < 0) { > dev_err(line6->ifcdev, "cannot start listening: %d\n", ret); > - return ret; > + if (line6->properties->capabilities & LINE6_CAP_CONTROL_MIDI) > + goto fail3; > + else > + goto fail2; > } > > return 0; > + > +fail3: > + kfree(line6->buffer_message); > +fail2: > + usb_free_urb(line6->urb_listen); > +fail1: > + kfree(line6->buffer_listen); > +fail_ret: > + return ret; > } Those resources are freed in the common destructor of the card instance, line6_destruct(), at disconnect callback, so it's redundant to release them here; even worse, since you haven't re-initialize with NULL, this change would lead to double-free. thanks, Takashi
On Tue, May 25, 2021 at 09:05:13AM +0200, Takashi Iwai wrote: > Those resources are freed in the common destructor of the card > instance, line6_destruct(), Oh my god, I missed line6_destruct. > at disconnect callback, so it's redundant > to release them here; even worse, since you haven't re-initialize with > NULL, this change would lead to double-free. Yes then it can cause double-free.. > > > thanks, > > Takashi I made a mistake, I'm so sorry to take your time... Thanks, Hyeonggon
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index 9602929b7de9..6991cb855023 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -688,34 +688,52 @@ static int line6_init_cap_control(struct usb_line6 *line6) /* initialize USB buffers: */ line6->buffer_listen = kmalloc(LINE6_BUFSIZE_LISTEN, GFP_KERNEL); - if (!line6->buffer_listen) - return -ENOMEM; + if (!line6->buffer_listen) { + ret = -ENOMEM; + goto fail_ret; + } line6->urb_listen = usb_alloc_urb(0, GFP_KERNEL); - if (!line6->urb_listen) - return -ENOMEM; + if (!line6->urb_listen) { + ret = -ENOMEM; + goto fail1; + } if (line6->properties->capabilities & LINE6_CAP_CONTROL_MIDI) { line6->buffer_message = kmalloc(LINE6_MIDI_MESSAGE_MAXLEN, GFP_KERNEL); - if (!line6->buffer_message) - return -ENOMEM; + if (!line6->buffer_message) { + ret = -ENOMEM; + goto fail2; + } ret = line6_init_midi(line6); if (ret < 0) - return ret; + goto fail3; } else { ret = line6_hwdep_init(line6); if (ret < 0) - return ret; + goto fail2; } ret = line6_start_listen(line6); if (ret < 0) { dev_err(line6->ifcdev, "cannot start listening: %d\n", ret); - return ret; + if (line6->properties->capabilities & LINE6_CAP_CONTROL_MIDI) + goto fail3; + else + goto fail2; } return 0; + +fail3: + kfree(line6->buffer_message); +fail2: + usb_free_urb(line6->urb_listen); +fail1: + kfree(line6->buffer_listen); +fail_ret: + return ret; } static void line6_startup_work(struct work_struct *work)
line6_init_cap_control doesn't free resources properly when allocations like kmalloc, usb_alloc_urb fails. It can cause memory leak when some allocations succeed, and then an error occurs. This patch makes line6_init_cap_control to properly free the resources, freeing previously allocated resources when there's an error. Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> --- sound/usb/line6/driver.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-)