[PATCH v2] tools/bootconfig: fix resource leak in apply_xbc()

[PATCH v2] tools/bootconfig: fix resource leak in apply_xbc()

[Yunfeng Ye]

Fix the @data and @fd allocations that are leaked in the error path of
apply_xbc().

Fixes: 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")
Fixes: 950313ebf79c ("tools: bootconfig: Add bootconfig command")
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tools/bootconfig/main.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index 16b9a420e6fd..d034f86022b7 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -314,31 +314,33 @@ int apply_xbc(const char *path, const char *xbc_path)
 	ret = delete_xbc(path);
 	if (ret < 0) {
 		pr_err("Failed to delete previous boot config: %d\n", ret);
-		return ret;
+		goto free_data;
 	}

 	/* Apply new one */
 	fd = open(path, O_RDWR | O_APPEND);
 	if (fd < 0) {
 		pr_err("Failed to open %s: %d\n", path, fd);
-		return fd;
+		ret = fd;
+		goto free_data;
 	}
 	/* TODO: Ensure the @path is initramfs/initrd image */
 	ret = write(fd, data, size + 8);
 	if (ret < 0) {
 		pr_err("Failed to apply a boot config: %d\n", ret);
-		return ret;
+		goto close_fd;
 	}
 	/* Write a magic word of the bootconfig */
 	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
-	if (ret < 0) {
+	if (ret < 0)
 		pr_err("Failed to apply a boot config magic: %d\n", ret);
-		return ret;
-	}
+
+close_fd:
 	close(fd);
+free_data:
 	free(data);

-	return 0;
+	return ret;
 }

 int usage(void)
-- 
1.8.3.1

Re: [PATCH v2] tools/bootconfig: Completion of error handling

[Markus Elfring]

…
> +++ b/tools/bootconfig/main.c
> @@ -314,31 +314,33 @@ int apply_xbc(const char *path, const char *xbc_path)
…
> +close_fd:
>  	close(fd);
> +free_data:
>  	free(data);
…

Do you find unchecked return values from calls of functions like close()
and printf() suspicious?
https://cwe.mitre.org/data/definitions/252.html

Regards,
Markus

Re: [PATCH v2] tools/bootconfig: fix resource leak in apply_xbc()

[Dan Carpenter]

On Fri, May 08, 2020 at 02:51:15PM +0800, Yunfeng Ye wrote:
> diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
> index 16b9a420e6fd..d034f86022b7 100644
> --- a/tools/bootconfig/main.c
> +++ b/tools/bootconfig/main.c
> @@ -314,31 +314,33 @@ int apply_xbc(const char *path, const char *xbc_path)
>  	ret = delete_xbc(path);
>  	if (ret < 0) {
>  		pr_err("Failed to delete previous boot config: %d\n", ret);
> -		return ret;
> +		goto free_data;
>  	}
> 
>  	/* Apply new one */
>  	fd = open(path, O_RDWR | O_APPEND);
>  	if (fd < 0) {
>  		pr_err("Failed to open %s: %d\n", path, fd);
> -		return fd;
> +		ret = fd;
> +		goto free_data;
>  	}
>  	/* TODO: Ensure the @path is initramfs/initrd image */
>  	ret = write(fd, data, size + 8);
>  	if (ret < 0) {
>  		pr_err("Failed to apply a boot config: %d\n", ret);
> -		return ret;
> +		goto close_fd;
>  	}
>  	/* Write a magic word of the bootconfig */
>  	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);

write returns the number of bytes written on success

> -	if (ret < 0) {
> +	if (ret < 0)
>  		pr_err("Failed to apply a boot config magic: %d\n", ret);
> -		return ret;
> -	}
> +
> +close_fd:
>  	close(fd);
> +free_data:
>  	free(data);
> 
> -	return 0;
> +	return ret;

But we want to return zero on success.

>  }

Btw, these leaks are totally harmless.  This is a short running user
space program with is going to immediately exit on error so the memory
will be freed anyway.  But the benifit is to silence static checker
warnings so that's useful.

regards,
dan carpenter

Re: [v2] tools/bootconfig: fix resource leak in apply_xbc()

[Markus Elfring]

> Btw, these leaks are totally harmless.  This is a short running user
> space program with is going to immediately exit on error so the memory
> will be freed anyway.

Can such a view mean that the function call “free(data)” should be omitted here
for a quicker program termination?


> But the benifit is to silence static checker warnings so that's useful.

Would you like to extend the commit message for the explanation of
the importance of the proposed change accordingly?

Regards,
Markus

Re: [PATCH v2] tools/bootconfig: fix resource leak in apply_xbc()

[Yunfeng Ye]

On 2020/5/8 17:30, Dan Carpenter wrote:
> On Fri, May 08, 2020 at 02:51:15PM +0800, Yunfeng Ye wrote:
>> diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
>> index 16b9a420e6fd..d034f86022b7 100644
>> --- a/tools/bootconfig/main.c
>> +++ b/tools/bootconfig/main.c
>> @@ -314,31 +314,33 @@ int apply_xbc(const char *path, const char *xbc_path)
>>  	ret = delete_xbc(path);
>>  	if (ret < 0) {
>>  		pr_err("Failed to delete previous boot config: %d\n", ret);
>> -		return ret;
>> +		goto free_data;
>>  	}
>>
>>  	/* Apply new one */
>>  	fd = open(path, O_RDWR | O_APPEND);
>>  	if (fd < 0) {
>>  		pr_err("Failed to open %s: %d\n", path, fd);
>> -		return fd;
>> +		ret = fd;
>> +		goto free_data;
>>  	}
>>  	/* TODO: Ensure the @path is initramfs/initrd image */
>>  	ret = write(fd, data, size + 8);
>>  	if (ret < 0) {
>>  		pr_err("Failed to apply a boot config: %d\n", ret);
>> -		return ret;
>> +		goto close_fd;
>>  	}
>>  	/* Write a magic word of the bootconfig */
>>  	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
> 
> write returns the number of bytes written on success
> 
>> -	if (ret < 0) {
>> +	if (ret < 0)
>>  		pr_err("Failed to apply a boot config magic: %d\n", ret);
>> -		return ret;
>> -	}
>> +
>> +close_fd:
>>  	close(fd);
>> +free_data:
>>  	free(data);
>>
>> -	return 0;
>> +	return ret;
> 
> But we want to return zero on success.
> 
yes, I should set 'ret' to 0 before returning on success. thanks.

>>  }
> 
> Btw, these leaks are totally harmless.  This is a short running user
> space program with is going to immediately exit on error so the memory
> will be freed anyway.  But the benifit is to silence static checker
> warnings so that's useful.
> 
> regards,
> dan carpenter
> 
> 
> .
>