Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33298
Change subject: lib: Prevent memory leak on error path ......................................................................
lib: Prevent memory leak on error path
Free the tree before returning to prevent a leak.
Change-Id: I1132c0e7404eec1af3adc19a83257f28563f8a58 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1401799 --- M src/lib/device_tree.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/33298/1
diff --git a/src/lib/device_tree.c b/src/lib/device_tree.c index a5021ca..ed878a2 100644 --- a/src/lib/device_tree.c +++ b/src/lib/device_tree.c @@ -259,11 +259,13 @@
if (magic != FDT_HEADER_MAGIC) { printk(BIOS_DEBUG, "Invalid device tree magic %#.8x!\n", magic); + free(tree); return NULL; } if (last_comp_version > FDT_SUPPORTED_VERSION) { printk(BIOS_DEBUG, "Unsupported device tree version %u(>=%u)\n", version, last_comp_version); + free(tree); return NULL; } if (version > FDT_SUPPORTED_VERSION)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33298 )
Change subject: lib: Prevent memory leak on error path ......................................................................
Patch Set 1: Code-Review+2
Not objecting to this patch specifically, but note that free() is currently a no-op in coreboot and even if we added a real allocator we'd probably never get into a situation where a leak like this matters (since coreboot isn't really a program that persists and runs stuff repeatedly... it boots once, finds a payload, tries to load that and if it fails it hangs -- if we run into the error path here, it's unlikely we'll ever need to allocate anything again).
So I'm fine with adding free() in simple cases like this but the DT/FIT code leaks plenty more memory elsewhere where it would be much more complicated to correctly free it all up again in all cases, and I don't think we should add complex code to cover those cases.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33298 )
Change subject: lib: Prevent memory leak on error path ......................................................................
lib: Prevent memory leak on error path
Free the tree before returning to prevent a leak.
Change-Id: I1132c0e7404eec1af3adc19a83257f28563f8a58 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1401799 Reviewed-on: https://review.coreboot.org/c/coreboot/+/33298 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/lib/device_tree.c 1 file changed, 2 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/lib/device_tree.c b/src/lib/device_tree.c index a5021ca..ed878a2 100644 --- a/src/lib/device_tree.c +++ b/src/lib/device_tree.c @@ -259,11 +259,13 @@
if (magic != FDT_HEADER_MAGIC) { printk(BIOS_DEBUG, "Invalid device tree magic %#.8x!\n", magic); + free(tree); return NULL; } if (last_comp_version > FDT_SUPPORTED_VERSION) { printk(BIOS_DEBUG, "Unsupported device tree version %u(>=%u)\n", version, last_comp_version); + free(tree); return NULL; } if (version > FDT_SUPPORTED_VERSION)
-
Jacob Garber (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)