Re: [PATCH] ALSA: usb-audio: Fix a potential memory leak in scarlett2_init_notify()

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