Messages in this thread Patch in this message | | | Date | Sun, 3 Sep 2023 17:04:47 +0200 | Subject | Re: [PATCH] ALSA: usb-audio: Fix a potential memory leak in scarlett2_init_notify() | From | Christophe JAILLET <> |
| |
Le 03/09/2023 à 16:23, Takashi Iwai a écrit : > On Sun, 03 Sep 2023 15:06:00 +0200, > Christophe JAILLET wrote: >> >> If usb_alloc_coherent() or usb_urb_ep_type_check() fail, we should release >> the resources previously allocated. > > Those are freed in the caller side, start_input_streams() instead.
Thanks for the fast review.
Hmpm, If IIUC, resources allocated *before* the ending "ep->num_urbs++" still need to be freed here, otherwise free_midi_urbs() in the caller will not free them.
Do you agree?
If yes, I can send v2 which would look like: usb_alloc_urb() if (err) return -ENOMEM
usb_alloc_coherent() if (err) { usb_free_urb() urb = NULL return -ENOMEM } usb_urb_ep_type_check() if (err) { usb_free_coherent() usb_free_urb() urb = NULL return -err }
Or, if yuo prefer, with an error handling path just like below, but without the final free_midi_urbs() + a comment explaining that the caller does this part of job instead.
CJ
> > > thanks, > > Takashi > >> >> Fixes: ff49d1df79ae ("ALSA: usb-audio: USB MIDI 2.0 UMP support") >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> sound/usb/midi2.c | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/sound/usb/midi2.c b/sound/usb/midi2.c >> index a27e244650c8..4109c82adff6 100644 >> --- a/sound/usb/midi2.c >> +++ b/sound/usb/midi2.c >> @@ -302,7 +302,8 @@ static int alloc_midi_urbs(struct snd_usb_midi2_endpoint *ep) >> ctx->urb = usb_alloc_urb(0, GFP_KERNEL); >> if (!ctx->urb) { >> dev_err(&ep->dev->dev, "URB alloc failed\n"); >> - return -ENOMEM; >> + err = -ENOMEM; >> + goto err_free_all; >> } >> ctx->ep = ep; >> buffer = usb_alloc_coherent(ep->dev, len, GFP_KERNEL, >> @@ -310,7 +311,8 @@ static int alloc_midi_urbs(struct snd_usb_midi2_endpoint *ep) >> if (!buffer) { >> dev_err(&ep->dev->dev, >> "URB buffer alloc failed (size %d)\n", len); >> - return -ENOMEM; >> + err = -ENOMEM; >> + goto err_free_cur_urb; >> } >> if (ep->interval) >> usb_fill_int_urb(ctx->urb, ep->dev, ep->pipe, >> @@ -322,13 +324,22 @@ static int alloc_midi_urbs(struct snd_usb_midi2_endpoint *ep) >> if (err < 0) { >> dev_err(&ep->dev->dev, "invalid MIDI EP %x\n", >> endpoint); >> - return err; >> + goto err_free_cur_dma; >> } >> ctx->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP; >> ep->num_urbs++; >> } >> ep->urb_free = ep->urb_free_mask = GENMASK(ep->num_urbs - 1, 0); >> return 0; >> + >> +err_free_cur_dma: >> + usb_free_coherent(ep->dev, len, buffer, ctx->urb->transfer_dma); >> +err_free_cur_urb: >> + usb_free_urb(ctx->urb); >> + ctx->urb = NULL; >> +err_free_all: >> + free_midi_urbs(ep); >> + return err; >> } >> >> static struct snd_usb_midi2_endpoint * >> -- >> 2.34.1 >> >
| |