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

[Dan Carpenter <dan.carpenter@oracle.com>]

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