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

[Takashi Iwai <tiwai@suse.de>]

On Sun, 03 Sep 2023 17:04:47 +0200,
Christophe JAILLET wrote:
> 
> 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.

Indeed.  The fix would be rather a oneliner like below, though:

--- a/sound/usb/midi2.c
+++ b/sound/usb/midi2.c
@@ -265,7 +265,7 @@ static void free_midi_urbs(struct snd_usb_midi2_endpoint *ep)
 
 	if (!ep)
 		return;
-	for (i = 0; i < ep->num_urbs; ++i) {
+	for (i = 0; i < NUM_URBS; ++i) {
 		ctx = &ep->urbs[i];
 		if (!ctx->urb)
 			break;

That was the intended behavior of free_midi_urbs().


Takashi