[PATCH] acpi: uniqueid: Fix a couple of memory leaks

[Alex Hung]

Thanks for fixing this.

On 2021-03-30 3:54 a.m., Colin King wrote:
> From: Colin Ian King <colin.king at canonical.com>
> 
> The uid_name and obj1 memory allocations are being leaked, so fix these
> by making uid_name an object on the stack and adding a free on obj1 if
> it's not being added to the hid_list.  Putting uid_name on the stack also
> fixes a malloc out of memory null pointer dereference issue too.
> Also add name_len variable to remove duplicated strlen calls. Finally,
> bail out if obj1 fails to allocate to avoid a null pointer dereference.
> 
> Addresses-Coverity: ("Resource leak")
> Fixes: 40e5b2edfe89 ("acpi: uniqueid: add tests for _HID/_CID vs. _UID")
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> ---
>  src/acpi/uniqueid/uniqueid.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/src/acpi/uniqueid/uniqueid.c b/src/acpi/uniqueid/uniqueid.c
> index 7dcbf6a6..29692d49 100644
> --- a/src/acpi/uniqueid/uniqueid.c
> +++ b/src/acpi/uniqueid/uniqueid.c
> @@ -64,7 +64,7 @@ static int uniqueid_evaluate_method(fwts_framework *fw,
>  	void *private)
>  {
>  	fwts_list *methods;
> -	size_t name_len = strlen(name);
> +	const size_t name_len = strlen(name);
>  
>  	if ((methods = fwts_acpi_object_get_names()) != NULL) {
>  		fwts_list_link	*item;
> @@ -148,7 +148,8 @@ static void unique_HID_return(
>  	ACPI_OBJECT *obj,
>  	void *private)
>  {
> -	char *uid_name = (char*) malloc(strlen(name) + 1);
> +	const size_t name_len = strlen(name) + 1;
> +	char uid_name[name_len];
>  	ACPI_OBJECT *hid_obj, *uid_obj;
>  	ACPI_BUFFER hid_buf, uid_buf;
>  	fwts_list_link *item;
> @@ -160,8 +161,8 @@ static void unique_HID_return(
>  	FWTS_UNUSED(obj);
>  	FWTS_UNUSED(private);
>  
> -	strlcpy(uid_name, name, strlen(name) + 1);
> -	uid_name[strlen(name) - 3] = 'U';
> +	strlcpy(uid_name, name, name_len);
> +	uid_name[name_len - 3] = 'U';

This should be
	uid_name[name_len - 4] = 'U';
since name_len = strlen(name) + 1

Otherwise it would replace 'I' instead of 'H' such as below:

165		uid_name[name_len - 3] = 'U';
(gdb) print uid_name
$1 = "\\_SB_.PCI0._HID"
(gdb) n
167		status = fwts_acpi_object_evaluate(fw, name, NULL, &hid_buf);
(gdb) print uid_name
$2 = "\\_SB_.PCI0._HUD"





>  
>  	status = fwts_acpi_object_evaluate(fw, name, NULL, &hid_buf);
>  	if (ACPI_FAILURE(status))
> @@ -173,15 +174,17 @@ static void unique_HID_return(
>  		return;
>  	uid_obj = uid_buf.Pointer;
>  
> -	if ((obj1 = (acpi_ids*)calloc(1, sizeof(acpi_ids))) != NULL) {
> -		obj1->hid_name = name;
> -		obj1->hid_obj = hid_obj;
> -		obj1->uid_obj = uid_obj;
> -	}
> +	obj1 = (acpi_ids *)calloc(1, sizeof(acpi_ids));
> +	if (!obj1)
> +		return;
> +
> +	obj1->hid_name = name;
> +	obj1->hid_obj = hid_obj;
> +	obj1->uid_obj = uid_obj;
>  
>  	fwts_list_foreach(item, hid_list) {
>  		acpi_ids *obj2 = fwts_list_data(acpi_ids*, item);
> -		if(is_uniqueid_equal(obj1, obj2)) {
> +		if (is_uniqueid_equal(obj1, obj2)) {
>  			passed = false;
>  			fwts_failed(fw, LOG_LEVEL_HIGH, "HardwareIDNotUnique",
>  				"%s/_UID conflict with %s/_UID", name, obj2->hid_name);
> @@ -189,10 +192,11 @@ static void unique_HID_return(
>  		}
>  	}
>  
> -	free(uid_name);
>  	if (passed) {
>  		fwts_list_append(hid_list, obj1);
>  		fwts_passed(fw, "%s/_UID is unique.", name);
> +	} else {
> +		free(obj1);
>  	}
>  }
>  
> 


-- 
Cheers,
Alex Hung