[PATCH v1 1/1] riscv/boot: Fix possible memory leak

[PATCH v1 1/1] riscv/boot: Fix possible memory leak

[Alistair Francis]

Coverity (CID 1405786) thinks that there is a possible memory leak as
we don't guarentee that the memory allocatd from riscv_find_firmware()
is freed. This is a false positive, but let's tidy up the code to fix
the warning.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/boot.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 2e92fb0680..7fee98d2f8 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -38,7 +38,7 @@ void riscv_find_and_load_firmware(MachineState *machine,
                                   const char *default_machine_firmware,
                                   hwaddr firmware_load_addr)
 {
-    char *firmware_filename;
+    char *firmware_filename = NULL;
 
     if (!machine->firmware) {
         /*
@@ -70,14 +70,11 @@ void riscv_find_and_load_firmware(MachineState *machine,
          * if no -bios option is set without breaking anything.
          */
         firmware_filename = riscv_find_firmware(default_machine_firmware);
-    } else {
-        firmware_filename = machine->firmware;
-        if (strcmp(firmware_filename, "none")) {
-            firmware_filename = riscv_find_firmware(firmware_filename);
-        }
+    } else if (strcmp(machine->firmware, "none")) {
+        firmware_filename = riscv_find_firmware(machine->firmware);
     }
 
-    if (strcmp(firmware_filename, "none")) {
+    if (firmware_filename) {
         /* If not "none" load the firmware */
         riscv_load_firmware(firmware_filename, firmware_load_addr);
         g_free(firmware_filename);
-- 
2.23.0

Re: [PATCH v1 1/1] riscv/boot: Fix possible memory leak

[Richard Henderson]

On 10/2/19 2:34 PM, Alistair Francis wrote:
> Coverity (CID 1405786) thinks that there is a possible memory leak as
> we don't guarentee that the memory allocatd from riscv_find_firmware()
> is freed. This is a false positive, but let's tidy up the code to fix
> the warning.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/boot.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v1 1/1] riscv/boot: Fix possible memory leak

[Bin Meng]

On Thu, Oct 3, 2019 at 5:38 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Coverity (CID 1405786) thinks that there is a possible memory leak as
> we don't guarentee that the memory allocatd from riscv_find_firmware()
> is freed. This is a false positive, but let's tidy up the code to fix
> the warning.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/boot.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Thanks for the patch. I am not sure how I can easily run Coverity to
verify the fix though.

Regards,
Bin

Re: [PATCH v1 1/1] riscv/boot: Fix possible memory leak

[Philippe Mathieu-Daudé]

On 10/2/19 11:34 PM, Alistair Francis wrote:
> Coverity (CID 1405786) thinks that there is a possible memory leak as
> we don't guarentee that the memory allocatd from riscv_find_firmware()

typos: 'guarantee', 'allocated'

> is freed. This is a false positive, but let's tidy up the code to fix
> the warning.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   hw/riscv/boot.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 2e92fb0680..7fee98d2f8 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -38,7 +38,7 @@ void riscv_find_and_load_firmware(MachineState *machine,
>                                     const char *default_machine_firmware,
>                                     hwaddr firmware_load_addr)
>   {
> -    char *firmware_filename;
> +    char *firmware_filename = NULL;
>   
>       if (!machine->firmware) {
>           /*
> @@ -70,14 +70,11 @@ void riscv_find_and_load_firmware(MachineState *machine,
>            * if no -bios option is set without breaking anything.
>            */
>           firmware_filename = riscv_find_firmware(default_machine_firmware);
> -    } else {
> -        firmware_filename = machine->firmware;
> -        if (strcmp(firmware_filename, "none")) {
> -            firmware_filename = riscv_find_firmware(firmware_filename);
> -        }
> +    } else if (strcmp(machine->firmware, "none")) {
> +        firmware_filename = riscv_find_firmware(machine->firmware);
>       }
>   
> -    if (strcmp(firmware_filename, "none")) {
> +    if (firmware_filename) {
>           /* If not "none" load the firmware */
>           riscv_load_firmware(firmware_filename, firmware_load_addr);
>           g_free(firmware_filename);
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v1 1/1] riscv/boot: Fix possible memory leak

[Peter Maydell]

On Thu, 3 Oct 2019 at 01:53, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Oct 3, 2019 at 5:38 AM Alistair Francis
> <alistair.francis@wdc.com> wrote:
> >
> > Coverity (CID 1405786) thinks that there is a possible memory leak as
> > we don't guarentee that the memory allocatd from riscv_find_firmware()
> > is freed. This is a false positive, but let's tidy up the code to fix
> > the warning.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  hw/riscv/boot.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>
> Thanks for the patch. I am not sure how I can easily run Coverity to
> verify the fix though.

You can't; we use the free 'coverity scan' service which just
checks QEMU git master. So we make the fixes we think will
deal with the issues, commit them to master in the usual way,
and then when the scan is rerun we can have another go if
the coverity issue hasn't gone away.

thanks
-- PMM