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]>
---
v2 -> v3:
- set 'ret' to 0 before returning on success
v1 -> v2:
- complete the error handling at other error path
- add "Fixes" tag
tools/bootconfig/main.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index 16b9a420e6fd..17a9837dcfaa 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -314,31 +314,35 @@ 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;
- }
+
+ ret = 0;
+
+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,35 @@ int apply_xbc(const char *path, const char *xbc_path)
…
> +free_data:
> free(data);
…
Would any software users prefer to omit the memory release for
a quicker program termination?
Can the commit message become clearer about the explanation for
the importance of the proposed change?
Regards,
Markus
On 2020/5/8 19:03, Markus Elfring wrote:
> …
>> +++ b/tools/bootconfig/main.c
>> @@ -314,31 +314,35 @@ int apply_xbc(const char *path, const char *xbc_path)
> …
>> +free_data:
>> free(data);
> …
>
> Would any software users prefer to omit the memory release for
> a quicker program termination?
>
> Can the commit message become clearer about the explanation for
> the importance of the proposed change?
>
ok, thanks. this change can fix the warning of tools.
> Regards,
> Markus
>
>
> this change can fix the warning of tools.
Would you like to point any specific source code analysis tools out
for this issue?
(Can a corresponding attribution become relevant for a clearer
change description?)
Regards,
Markus
On 2020/5/8 19:30, Markus Elfring wrote:
>> this change can fix the warning of tools.
>
> Would you like to point any specific source code analysis tools out
> for this issue?
> (Can a corresponding attribution become relevant for a clearer
> change description?)
>
The tools we used is not for open source. it point out some error description like
"Memory leak: data" and "Resource leak: fd" in tools/bootconfig/main.c.
Can I only description:
"Memory and resource leak is found by a static code analysis tools" ? thanks.
> Regards,
> Markus
>
>
On Fri, 8 May 2020 19:42:56 +0800
Yunfeng Ye <[email protected]> wrote:
> On 2020/5/8 19:30, Markus Elfring wrote:
> >> this change can fix the warning of tools.
> >
> > Would you like to point any specific source code analysis tools out
> > for this issue?
> > (Can a corresponding attribution become relevant for a clearer
> > change description?)
> >
> The tools we used is not for open source. it point out some error description like
> "Memory leak: data" and "Resource leak: fd" in tools/bootconfig/main.c.
>
> Can I only description:
> "Memory and resource leak is found by a static code analysis tools" ? thanks.
Markus please stop! Your suggestions are just your preferences that are not
required for the kernel.
Yunfeng, your v2 was fine and has already landed in Linus's tree. Feel free
to ignore Markus's suggestions in the future.
See commit 8842604446d1f005abcbf8c63c12eabdb5695094
-- Steve
> Markus please stop! Your suggestions are just your preferences that are not
> required for the kernel.
Do our imaginations and expectations differ for a good commit message?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=79dede78c0573618e3137d3d8cbf78c84e25fabd#n102
Regards,
Markus
On Fri, May 08, 2020 at 09:00:53AM -0400, Steven Rostedt wrote:
> On Fri, 8 May 2020 19:42:56 +0800
> Yunfeng Ye <[email protected]> wrote:
>
> > On 2020/5/8 19:30, Markus Elfring wrote:
> > >> this change can fix the warning of tools.
> > >
> > > Would you like to point any specific source code analysis tools out
> > > for this issue?
> > > (Can a corresponding attribution become relevant for a clearer
> > > change description?)
> > >
> > The tools we used is not for open source. it point out some error description like
> > "Memory leak: data" and "Resource leak: fd" in tools/bootconfig/main.c.
> >
> > Can I only description:
> > "Memory and resource leak is found by a static code analysis tools" ? thanks.
>
> Markus please stop! Your suggestions are just your preferences that are not
> required for the kernel.
>
> Yunfeng, your v2 was fine and has already landed in Linus's tree. Feel free
> to ignore Markus's suggestions in the future.
There was actually a bug in v2. It exits with a non-zero instead of
zero on success so it will mess up people's scripts.
regards,
dan carpenter
On Fri, 8 May 2020 17:45:27 +0300
Dan Carpenter <[email protected]> wrote:
> > Yunfeng, your v2 was fine and has already landed in Linus's tree. Feel free
> > to ignore Markus's suggestions in the future.
>
> There was actually a bug in v2. It exits with a non-zero instead of
> zero on success so it will mess up people's scripts.
Thanks, I just fixed it. And because of the distraction of Markus's
bikesheding, it was missed :-(
-- Steve
>> There was actually a bug in v2. It exits with a non-zero instead of
>> zero on success so it will mess up people's scripts.
>
> Thanks, I just fixed it. And because of the distraction of Markus's
> bikesheding, it was missed :-(
I am curious how the software development attention will evolve further.
tools/bootconfig: Fix apply_xbc() to return zero on success
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/patchwork/patch/1239092/
https://lkml.org/lkml/2020/5/8/1342
Will additional change possibilities be taken into account?
Regards,
Markus