[PATCH] media: staging: greybus: Release memory obtained by kasprintf

[PATCH] media: staging: greybus: Release memory obtained by kasprintf

[Arvind Yadav]

Free memory region, if gb_lights_channel_config is not successful.

Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
 drivers/staging/greybus/light.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
index 3f4148c..b00d47c 100644
--- a/drivers/staging/greybus/light.c
+++ b/drivers/staging/greybus/light.c
@@ -984,7 +984,7 @@ static int gb_lights_channel_config(struct gb_light *light,
 
 	ret = channel_attr_groups_set(channel, cdev);
 	if (ret < 0)
-		return ret;
+		goto err;
 
 	gb_lights_led_operations_set(channel, cdev);
 
@@ -994,15 +994,18 @@ static int gb_lights_channel_config(struct gb_light *light,
 	 * configurations.
 	 */
 	if (!is_channel_flash(channel))
-		return ret;
+		goto err;
 
 	light->has_flash = true;
 
 	ret = gb_lights_channel_flash_config(channel);
 	if (ret < 0)
-		return ret;
+		goto err;
 
 	return ret;
+err:
+	kfree(cdev->name);
+	return ret;
 }
 
 static int gb_lights_light_config(struct gb_lights *glights, u8 id)
-- 
1.9.1

Re: [PATCH] media: staging: greybus: Release memory obtained by kasprintf

[Dan Carpenter]

On Thu, Sep 21, 2017 at 05:05:27PM +0530, Arvind Yadav wrote:
> Free memory region, if gb_lights_channel_config is not successful.
> 

The question I have is do we free this on module unload?  I don't see
that we do.  I feel like we should do a free after calling
__gb_lights_led_unregister().  But that's awkward because we call
__gb_lights_led_unregister() when this function fails so it would end
up being a double free.

> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
>  drivers/staging/greybus/light.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index 3f4148c..b00d47c 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -984,7 +984,7 @@ static int gb_lights_channel_config(struct gb_light *light,
>  
>  	ret = channel_attr_groups_set(channel, cdev);
>  	if (ret < 0)
> -		return ret;
> +		goto err;
>  
>  	gb_lights_led_operations_set(channel, cdev);
>  
> @@ -994,15 +994,18 @@ static int gb_lights_channel_config(struct gb_light *light,
>  	 * configurations.
>  	 */
>  	if (!is_channel_flash(channel))
> -		return ret;
> +		goto err;

"ret" is zero here.  This is actually a success return.  It would be
cleaner to just write "return 0;".  Anyway, this patch introduces a use
after free so that doesn't work.

Also it's better to choose a label name which says what the label does
so in this case it would be "goto err_free_name" or "goto err_cdev_name"
or whatever, but something to indicate that it's to do with freeing
the cdev->name.  Just "err" is too ambiguous.

>  
>  	light->has_flash = true;
>  
>  	ret = gb_lights_channel_flash_config(channel);
>  	if (ret < 0)
> -		return ret;
> +		goto err;
>  
>  	return ret;
        ^^^^^^^^^^
Here as well, change this from "return ret;" to "return 0;".

regards,
dan carpenter

Re: [PATCH] media: staging: greybus: Release memory obtained by kasprintf

[Rui Miguel Silva]

Hi,
On Thu, Sep 21, 2017 at 03:59:18PM +0300, Dan Carpenter wrote:
> On Thu, Sep 21, 2017 at 05:05:27PM +0530, Arvind Yadav wrote:
> > Free memory region, if gb_lights_channel_config is not successful.

Arvind, thanks for patch and good catch.
But please look at the subject of other patches applied to this
file and try to stick with the labels, staging: greybus: light:

> > 
> 
> The question I have is do we free this on module unload?  I don't see
> that we do.  I feel like we should do a free after calling
> __gb_lights_led_unregister().  But that's awkward because we call
> __gb_lights_led_unregister() when this function fails so it would end
> up being a double free.

Yes Dan, You are correct, this should be free in
__gb_lights_led_unregister(), and not here.

> 
> > Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> > ---
> >  drivers/staging/greybus/light.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> > index 3f4148c..b00d47c 100644
> > --- a/drivers/staging/greybus/light.c
> > +++ b/drivers/staging/greybus/light.c
> > @@ -984,7 +984,7 @@ static int gb_lights_channel_config(struct gb_light *light,
> >  
> >  	ret = channel_attr_groups_set(channel, cdev);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto err;
> >  
> >  	gb_lights_led_operations_set(channel, cdev);
> >  
> > @@ -994,15 +994,18 @@ static int gb_lights_channel_config(struct gb_light *light,
> >  	 * configurations.
> >  	 */
> >  	if (!is_channel_flash(channel))
> > -		return ret;
> > +		goto err;
> 
> "ret" is zero here.  This is actually a success return.  It would be
> cleaner to just write "return 0;".  Anyway, this patch introduces a use
> after free so that doesn't work.
> 
> Also it's better to choose a label name which says what the label does
> so in this case it would be "goto err_free_name" or "goto err_cdev_name"
> or whatever, but something to indicate that it's to do with freeing
> the cdev->name.  Just "err" is too ambiguous.
> 
> >  
> >  	light->has_flash = true;
> >  
> >  	ret = gb_lights_channel_flash_config(channel);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto err;
> >  
> >  	return ret;
>         ^^^^^^^^^^
> Here as well, change this from "return ret;" to "return 0;".

It should be return 0; from the start, you are right, but that
would be a complete different change than the actual fix that now
goes far away from this code.

Thank both,
---
Cheers,
	Rui