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

[Colin Ian King]

On 01/04/2021 06:30, Alex Hung wrote:
> 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"

Good catch! I'll send a V2.
> 
> 
> 
> 
> 
>>  
>>  	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);
>>  	}
>>  }
>>  
>>
> 
>