All of
 help / color / mirror / Atom feed
From: Dan Carpenter <>
To: Edmundo Carmona Antoranz <>
Cc: Christophe JAILLET <>,
Subject: Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path
Date: Mon, 10 May 2021 08:39:58 +0300	[thread overview]
Message-ID: <20210510053958.GO1955@kadam> (raw)
In-Reply-To: <>

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
> <> 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;


	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.

dan carpenter

  parent reply	other threads:[~2021-05-10  5:40 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-08  7:23 Christophe JAILLET
2021-05-08 14:50 ` Edmundo Carmona Antoranz
2021-05-08 18:47   ` Christophe JAILLET
2021-05-10  5:39   ` Dan Carpenter [this message]
     [not found]     ` <>
2021-05-10 13:55       ` Dan Carpenter
2021-05-10 12:18 ` Andy Shevchenko
2021-05-10 17:04   ` Rafael J. Wysocki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210510053958.GO1955@kadam \ \ \ \ \
    --subject='Re: [PATCH] ACPI: scan: Fix a memory leak in an error handling path' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.