[U-Boot] [PATCH v2] gadget: f_thor: Fix memory leaks of usb request and its buffer

[Lukasz Majewski <lukma@denx.de>]

Hi Seung-Woo,

> There are memory leaks of usb request and its buffer for ep0,
> in_ep, and out ep. Fix memory leaks of usb request and its buffer.
> 
> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> ---
> Change from v1
> - remove allocation of out_ep request instead of allocating and
> freeing
> - fix use error path instead of duplicated error handling code
> ---
>  drivers/usb/gadget/f_thor.c |   45
> ++++++++++++++++++++++++++++--------------- 1 file changed, 29
> insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c
> index c8eda05..02d6844 100644
> --- a/drivers/usb/gadget/f_thor.c
> +++ b/drivers/usb/gadget/f_thor.c
> @@ -752,6 +752,13 @@ int thor_handle(void)
>  	return 0;
>  }
>  
> +static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
> +{
> +	if (req->buf)
> +		free(req->buf);
> +	usb_ep_free_request(ep, req);
> +}
> +
>  static int thor_func_bind(struct usb_configuration *c, struct
> usb_function *f) {
>  	struct usb_gadget *gadget = c->cdev->gadget;
> @@ -860,21 +867,18 @@ static int thor_func_bind(struct
> usb_configuration *c, struct usb_function *f) return 0;
>  
>   fail:
> +	if (dev->req)
> +		free_ep_req(gadget->ep0, dev->req);
>  	free(dev);
>  	return status;
>  }
>  
> -static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
> -{
> -	free(req->buf);
> -	usb_ep_free_request(ep, req);
> -}
> -
>  static void thor_unbind(struct usb_configuration *c, struct
> usb_function *f) {
>  	struct f_thor *f_thor = func_to_thor(f);
>  	struct thor_dev *dev = f_thor->dev;
>  
> +	free_ep_req(dev->gadget->ep0, dev->req);

Till this change - no issues.

>  	free(dev);
>  	memset(thor_func, 0, sizeof(*thor_func));
>  	thor_func = NULL;
> @@ -895,8 +899,6 @@ static void thor_func_disable(struct usb_function
> *f) }
>  
>  	if (dev->out_ep->driver_data) {
> -		free(dev->out_req->buf);
> -		dev->out_req->buf = NULL;
	
I think that this setting (to NULL) was needed to be able to ctrl+C
from thor command and then run it again (as some code checks if buf is
NULL).

>  		usb_ep_free_request(dev->out_ep, dev->out_req);
>  		usb_ep_disable(dev->out_ep);
>  		dev->out_ep->driver_data = NULL;
> @@ -924,14 +926,13 @@ static int thor_eps_setup(struct usb_function
> *f) 
>  	result = usb_ep_enable(ep, d);
>  	if (result)
> -		goto exit;
> +		goto err;
>  
>  	ep->driver_data = cdev; /* claim */
>  	req = thor_start_ep(ep);
>  	if (!req) {
> -		usb_ep_disable(ep);
>  		result = -EIO;
> -		goto exit;
> +		goto err_disable_in_ep;
>  	}
>  
>  	dev->in_req = req;
> @@ -941,22 +942,34 @@ static int thor_eps_setup(struct usb_function
> *f) 
>  	result = usb_ep_enable(ep, d);
>  	if (result)
> -		goto exit;
> +		goto err_free_in_req;
>  
>  	ep->driver_data = cdev; /* claim */
> -	req = thor_start_ep(ep);
> +	req = usb_ep_alloc_request(ep, 0);

Is this safe to replace thor_start_ep() - which tunes the ep params -
with generic function?

( I do see the req->complete = thor_rx_tx_complete below ).

If the thor_start_ep can be replaced with generic code, then maybe we
can remove it?

>  	if (!req) {
> -		usb_ep_disable(ep);
>  		result = -EIO;
> -		goto exit;
> +		goto err_disable_out_ep;
>  	}
>  
> +	req->complete = thor_rx_tx_complete;
>  	dev->out_req = req;
>  	/* ACM control EP */
>  	ep = dev->int_ep;
>  	ep->driver_data = cdev;	/* claim */
>  
> - exit:
> +	return 0;
> +
> + err_disable_out_ep:
> +	usb_ep_disable(dev->out_ep);
> +
> + err_free_in_req:
> +	free_ep_req(dev->in_ep, dev->in_req);
> +	dev->in_req = NULL;
> +
> + err_disable_in_ep:
> +	usb_ep_disable(dev->in_ep);
> +
> + err:
>  	return result;
>  }
>  




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180525/618f82d7/attachment.sig>