[PATCH] ACPI: scan: Fix a memory leak in an error handling path

[PATCH] ACPI: scan: Fix a memory leak in an error handling path

[Christophe JAILLET]

If 'acpi_device_set_name()' fails, we must free
'acpi_device_bus_id->bus_id' or there is a (potential) memory leak.

Fixes: eb50aaf960e3 ("ACPI: scan: Use unique number for instance_no")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/acpi/scan.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index a22778e880c2..651a431e2bbf 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -700,6 +700,7 @@ int acpi_device_add(struct acpi_device *device,
 
 		result = acpi_device_set_name(device, acpi_device_bus_id);
 		if (result) {
+			kfree_const(acpi_device_bus_id->bus_id);
 			kfree(acpi_device_bus_id);
 			goto err_unlock;
 		}
-- 
2.30.2

Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path

[Edmundo Carmona Antoranz]

On Sat, May 8, 2021 at 1:23 AM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> If 'acpi_device_set_name()' fails, we must free
> 'acpi_device_bus_id->bus_id' or there is a (potential) memory leak.

This is a question about the style used in kernel. This function
(acpi_device_add) initializes acpi_device_bus_id and it could also
initialize acpi_device_bus_id->bus_id and any of those might fail. And
they will have to be freed if they are initialized and so on. I see
that there are kfrees used for them in different places before using a
goto err_unlock; I wonder if it is accepted practice to avoid doing
these kfrees and have them in a single place instead, something like:

err_onlock:
    if (acpi_device_bus_id) {
        if (acpi_device_bus_id->bus_id)
            kfree(acpi_device_bus_id->bus_id);
        kfree(acpi_device_bus_id);
    }
    mutex_unlock(&acpi_device_lock);
.
.
.

Thanks in advance.

Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path

[Christophe JAILLET]

Le 08/05/2021 à 16:50, Edmundo Carmona Antoranz a écrit :
> On Sat, May 8, 2021 at 1:23 AM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
>>
>> If 'acpi_device_set_name()' fails, we must free
>> 'acpi_device_bus_id->bus_id' or there is a (potential) memory leak.
> 
> This is a question about the style used in kernel. This function
> (acpi_device_add) initializes acpi_device_bus_id and it could also
> initialize acpi_device_bus_id->bus_id and any of those might fail. And
> they will have to be freed if they are initialized and so on. I see
> that there are kfrees used for them in different places before using a
> goto err_unlock; I wonder if it is accepted practice to avoid doing
> these kfrees and have them in a single place instead, something like:
> 
> err_onlock:
>      if (acpi_device_bus_id) {
>          if (acpi_device_bus_id->bus_id)
>              kfree(acpi_device_bus_id->bus_id);
>          kfree(acpi_device_bus_id);
>      }
>      mutex_unlock(&acpi_device_lock);
> .
> .
> .
> 
> Thanks in advance.
> 

Yes, usually, the preferred style is to have a error handling path which 
releases all that need to be released.
This is more future proof and usually more readable.

In this particular case, I don't think it would be a good idea because 
there are several different error handling pathd and it would look like 
spaghetti code.

First in your example, acpi_device_bus_id should be set to NULL in the 
declaration of this variable.

But, it is likely that the kfree and kfree_cont are not needed after the 
list_add_tail around line 707.


So, leaving these small pieces of resources freeing where they are now 
looks, IMHO, easier to understand.

But, as you say, it is sometimes a matter of taste, and if someone can 
propose something that is straightforward, it would be welcomed, I guess.

CJ

Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path

[Dan Carpenter]

On Sat, May 08, 2021 at 08:50:44AM -0600, Edmundo Carmona Antoranz wrote:
> On Sat, May 8, 2021 at 1:23 AM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
> >
> > If 'acpi_device_set_name()' fails, we must free
> > 'acpi_device_bus_id->bus_id' or there is a (potential) memory leak.
> 
> This is a question about the style used in kernel. This function
> (acpi_device_add) initializes acpi_device_bus_id and it could also
> initialize acpi_device_bus_id->bus_id and any of those might fail. And
> they will have to be freed if they are initialized and so on. I see
> that there are kfrees used for them in different places before using a
> goto err_unlock; I wonder if it is accepted practice to avoid doing
> these kfrees and have them in a single place instead, something like:
> 
> err_onlock:
>     if (acpi_device_bus_id) {
>         if (acpi_device_bus_id->bus_id)
>             kfree(acpi_device_bus_id->bus_id);
>         kfree(acpi_device_bus_id);
>     }
>     mutex_unlock(&acpi_device_lock);

This would introduce some bugs so it's not possible.  And it's also
really ugly.

What you want is that when you're reading the code from top down, it
should all make sense without scrolling back and forth, up and down.  So
choose label names which make sense.  "err_unlock" shouldn't free a
bunch of stuff.  You don't want a big nest of if statements at the
bottom.  The if statements in the unwind code should be a mirror image
of the if statements in the allocation code.

The best way to write error handling is "free the last resource style"
where you free the most recent resouce that was successfully allocated.
That way in your head you just have to keep track of one thing, instead
of tracking everything.

	a = alloc();
	if (!a)
		return -ENOMEM;

	b = alloc();
	if (!b) {
		ret = -ENOMEM;
		goto free_a;
	}

	c = alloc();
	if (!c) {
		ret = -ENOMEM;
		goto free_b;
	}

	return 0;

free_b:
	free(b);
free_a:
	free(a);

	return ret;

The error handling is just "goto free_b;" which is immediate.  Error
happens and, boom, dealt with.

It's not like in some kernel code where it's error happens, "goto out;",
scroll down to the bottom to see what "goto out;" does.  Then if calls a
the drivers release function which frees all the driver resources
whether or not they have been allocated.  So now you have to trace
through a bunch of if statements and no-op free functions.  Then if it's
correct you have to try remember what code you were looking at
originally and find your place again.

regards,
dan carpenter

Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path

[Andy Shevchenko]

On Sat, May 08, 2021 at 09:23:09AM +0200, Christophe JAILLET wrote:
> If 'acpi_device_set_name()' fails, we must free
> 'acpi_device_bus_id->bus_id' or there is a (potential) memory leak.

Good catch!
I guess I have lost it somewhere in the middle of developing the mentioned fix.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks!

> Fixes: eb50aaf960e3 ("ACPI: scan: Use unique number for instance_no")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/acpi/scan.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index a22778e880c2..651a431e2bbf 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -700,6 +700,7 @@ int acpi_device_add(struct acpi_device *device,
>  
>  		result = acpi_device_set_name(device, acpi_device_bus_id);
>  		if (result) {
> +			kfree_const(acpi_device_bus_id->bus_id);
>  			kfree(acpi_device_bus_id);
>  			goto err_unlock;
>  		}
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path

[Dan Carpenter]

On Mon, May 10, 2021 at 07:18:14AM -0600, Edmundo Carmona Antoranz wrote:
> On Sun, May 9, 2021 at 11:40 PM Dan Carpenter <dan.carpenter@oracle.com>
> wrote:
> 
> >
> > The best way to write error handling is "free the last resource style"
> > where you free the most recent resouce that was successfully allocated.
> > That way in your head you just have to keep track of one thing, instead
> > of tracking everything.
> >
> 
> IC. So the answer is rather: single place to free stuff? Yes..... _but_ in
> a layered fashion (through tags) instead of having a single tag to take
> care of everything.
> 
> Follow up question: is it worth making a patch to try to simplify a
> function to have error handling set up this way for code that is already
> working? Or not really?
> 

No, absolutely not.  This is just my preference and probably the most
common way to write error handling but it's not an official standard.

Plus it super easy to find error handling code which is not working and
fix that instead.

regards,
dan carpenter

Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path

[Rafael J. Wysocki]

On Mon, May 10, 2021 at 3:17 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Sat, May 08, 2021 at 09:23:09AM +0200, Christophe JAILLET wrote:
> > If 'acpi_device_set_name()' fails, we must free
> > 'acpi_device_bus_id->bus_id' or there is a (potential) memory leak.
>
> Good catch!
> I guess I have lost it somewhere in the middle of developing the mentioned fix.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Thanks!
>
> > Fixes: eb50aaf960e3 ("ACPI: scan: Use unique number for instance_no")
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> >  drivers/acpi/scan.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index a22778e880c2..651a431e2bbf 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -700,6 +700,7 @@ int acpi_device_add(struct acpi_device *device,
> >
> >               result = acpi_device_set_name(device, acpi_device_bus_id);
> >               if (result) {
> > +                     kfree_const(acpi_device_bus_id->bus_id);
> >                       kfree(acpi_device_bus_id);
> >                       goto err_unlock;
> >               }
> > --

Applied as 5.13-rc material, thanks!