Change in coreboot[master]: acpi: Avoid freeing a device twice

[Simon Glass (Code Review)]

Simon Glass has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42892 )

Change subject: acpi: Avoid freeing a device twice
......................................................................

acpi: Avoid freeing a device twice

The current implementation of acpi_dp_write() frees the node after it has
written it.

If the structure contains a ACPI_DP_TYPE_CHILD then a recursive call to
acpi_dp_write() frees the child and then frees it again when returning
from the call. This results in a double free.

Split the implementation into two steps, one that ones and one that frees.
This is easier to understand and fixes the bug.

Note: This likely has no effect in coreboot since it doesn't seem to have
a proper free() implementation. But it might gain one one day.

BUG=none

Signed-off-by: Simon Glass 
Change-Id: Ife3917af10bc35a3c3eee38d8292f927ef15409d
---
M src/acpi/device.c
1 file changed, 6 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/42892/1

diff --git a/src/acpi/device.c b/src/acpi/device.c
index 9ce86eb..49e1d46 100644
--- a/src/acpi/device.c
+++ b/src/acpi/device.c
@@ -770,7 +770,7 @@
    return false;
 }

-void acpi_dp_write(struct acpi_dp *table)
+static void acpi_dp_write_(struct acpi_dp *table)
 {
    struct acpi_dp *dp, *prop;
    char *dp_count;
@@ -827,6 +827,11 @@
    for (dp = prop; dp; dp = dp->next)
    	if (dp->type == ACPI_DP_TYPE_CHILD)
    		acpi_dp_write(dp->child);
+}
+
+void acpi_dp_write(struct acpi_dp *table)
+{
+	acpi_dp_write_(table);

/* Clean up */
    acpi_dp_free(table);

[Simon Glass (Code Review)]

Hello Patrick Georgi, Duncan Laurie,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42892

to look at the new patch set (#2).

Change subject: acpi: Avoid freeing a device twice
......................................................................

acpi: Avoid freeing a device twice

The current implementation of acpi_dp_write() frees the node after it
has written it.

If the structure contains a ACPI_DP_TYPE_CHILD then a recursive call to
acpi_dp_write() frees the child and then frees it again when returning
from the call. This results in a double free.

Split the implementation into two steps, one that ones and one that
frees. This is easier to understand and fixes the bug.

Note: This likely has no effect in coreboot since it doesn't seem to
have a proper free() implementation. But it might gain one one day.

BUG=none

Signed-off-by: Simon Glass 
Change-Id: Ife3917af10bc35a3c3eee38d8292f927ef15409d
---
M src/acpi/device.c
1 file changed, 6 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/42892/2

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42892 )

Change subject: acpi: Avoid freeing a device twice
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42892 )

Change subject: acpi: Avoid freeing a device twice
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42892/2/src/acpi/device.c 
File src/acpi/device.c:

https://review.coreboot.org/c/coreboot/+/42892/2/src/acpi/device.c@829 
PS2, Line 829: 			acpi_dp_write(dp->child);
Right now the code seems to do exactly the same as before, just in two functions. Don't you have to change this to call acpi_dp_write_?

[Simon Glass (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Duncan Laurie, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42892

to look at the new patch set (#3).

Change subject: acpi: Avoid freeing a device twice
......................................................................

acpi: Avoid freeing a device twice

The current implementation of acpi_dp_write() frees the node after it
has written it.

If the structure contains a ACPI_DP_TYPE_CHILD then a recursive call to
acpi_dp_write() frees the child and then frees it again when returning
from the call. This results in a double free.

Split the implementation into two steps, one that ones and one that
frees. This is easier to understand and fixes the bug.

Note: This likely has no effect in coreboot since it doesn't seem to
have a proper free() implementation. But it might gain one one day.

BUG=none

Signed-off-by: Simon Glass 
Change-Id: Ife3917af10bc35a3c3eee38d8292f927ef15409d
---
M src/acpi/device.c
1 file changed, 7 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/42892/3

[Simon Glass (Code Review)]

Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42892 )

Change subject: acpi: Avoid freeing a device twice
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42892/2/src/acpi/device.c 
File src/acpi/device.c:

https://review.coreboot.org/c/coreboot/+/42892/2/src/acpi/device.c@829 
PS2, Line 829: 			acpi_dp_write(dp->child);
> Right now the code seems to do exactly the same as before, just in two functions. […]

Er, yes. Fixed, thanks.

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42892 )

Change subject: acpi: Avoid freeing a device twice
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/42892/2/src/acpi/device.c 
File src/acpi/device.c:

https://review.coreboot.org/c/coreboot/+/42892/2/src/acpi/device.c@829 
PS2, Line 829: 			acpi_dp_write(dp->child);
> Er, yes. Fixed, thanks.

Done

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42892 )

Change subject: acpi: Avoid freeing a device twice
......................................................................

acpi: Avoid freeing a device twice

The current implementation of acpi_dp_write() frees the node after it
has written it.

If the structure contains a ACPI_DP_TYPE_CHILD then a recursive call to
acpi_dp_write() frees the child and then frees it again when returning
from the call. This results in a double free.

Split the implementation into two steps, one that ones and one that
frees. This is easier to understand and fixes the bug.

Note: This likely has no effect in coreboot since it doesn't seem to
have a proper free() implementation. But it might gain one one day.

BUG=none

Signed-off-by: Simon Glass 
Change-Id: Ife3917af10bc35a3c3eee38d8292f927ef15409d
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42892
Reviewed-by: Patrick Georgi 
Tested-by: build bot (Jenkins) 
---
M src/acpi/device.c
1 file changed, 7 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved

diff --git a/src/acpi/device.c b/src/acpi/device.c
index 9ce86eb..b119abd 100644
--- a/src/acpi/device.c
+++ b/src/acpi/device.c
@@ -770,7 +770,7 @@
    return false;
 }

-void acpi_dp_write(struct acpi_dp *table)
+static void acpi_dp_write_(struct acpi_dp *table)
 {
    struct acpi_dp *dp, *prop;
    char *dp_count;
@@ -826,7 +826,12 @@
    /* Recursively parse children into separate tables */
    for (dp = prop; dp; dp = dp->next)
    	if (dp->type == ACPI_DP_TYPE_CHILD)
-			acpi_dp_write(dp->child);
+			acpi_dp_write_(dp->child);
+}
+
+void acpi_dp_write(struct acpi_dp *table)
+{
+	acpi_dp_write_(table);

/* Clean up */
    acpi_dp_free(table);