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 <[email protected]>
Signed-off-by: Yunfeng Ye <[email protected]>
Signed-off-by: Steven Rostedt (VMware) <[email protected]>
---
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
…
> +++ 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
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
> 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
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
>
>
> .
>