[PULL,11/48] riscv: Resolve full path of the given bios image

From: Bin Meng <bmeng.cn@gmail.com>

At present when "-bios image" is supplied, we just use the straight
path without searching for the configured data directories. Like
"-bios default", we add the same logic so that "-L" actually works.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 hw/riscv/boot.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

On Wed, 18 Sep 2019 at 16:35, Palmer Dabbelt <palmer@sifive.com> wrote:
>
> From: Bin Meng <bmeng.cn@gmail.com>
>
> At present when "-bios image" is supplied, we just use the straight
> path without searching for the configured data directories. Like
> "-bios default", we add the same logic so that "-L" actually works.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> ---
>  hw/riscv/boot.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 10f7991490..2e92fb0680 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -72,14 +72,14 @@ void riscv_find_and_load_firmware(MachineState *machine,
>          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);
> +        }
>      }
>
>      if (strcmp(firmware_filename, "none")) {
>          /* If not "none" load the firmware */
>          riscv_load_firmware(firmware_filename, firmware_load_addr);
> -    }
> -
> -    if (!strcmp(machine->firmware, "default")) {
>          g_free(firmware_filename);
>      }
>  }

Hi; Coverity (CID 1405786) thinks this introduces a possible
memory leak, because it's not sure that memory allocated
and returned by the call to riscv_find_firmware() is
guaranteed to be freed before the end of the function.
I think it might be a false positive, but I wasn't entirely
sure, so maybe this code could be rephrased to be clearer?
I think the root of the problem is that we have a local
variable 'firmware_filename' which might point to memory
allocated-and-to-be-freed, or might point to memory which
must not be freed (machine->firmware), and then you have
to check the flow of logic through the code quite carefully
to figure out whether the condition under which we choose
to call g_free() is exactly equivalent to the condition
where we set firmware_filename to point to allocated memory...

thanks
-- PMM

On Tue, Sep 24, 2019 at 3:18 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 18 Sep 2019 at 16:35, Palmer Dabbelt <palmer@sifive.com> wrote:
> >
> > From: Bin Meng <bmeng.cn@gmail.com>
> >
> > At present when "-bios image" is supplied, we just use the straight
> > path without searching for the configured data directories. Like
> > "-bios default", we add the same logic so that "-L" actually works.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> > ---
> >  hw/riscv/boot.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 10f7991490..2e92fb0680 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -72,14 +72,14 @@ void riscv_find_and_load_firmware(MachineState *machine,
> >          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);
> > +        }
> >      }
> >
> >      if (strcmp(firmware_filename, "none")) {
> >          /* If not "none" load the firmware */
> >          riscv_load_firmware(firmware_filename, firmware_load_addr);
> > -    }
> > -
> > -    if (!strcmp(machine->firmware, "default")) {
> >          g_free(firmware_filename);
> >      }
> >  }
>
> Hi; Coverity (CID 1405786) thinks this introduces a possible
> memory leak, because it's not sure that memory allocated
> and returned by the call to riscv_find_firmware() is
> guaranteed to be freed before the end of the function.
> I think it might be a false positive, but I wasn't entirely
> sure, so maybe this code could be rephrased to be clearer?
> I think the root of the problem is that we have a local
> variable 'firmware_filename' which might point to memory
> allocated-and-to-be-freed, or might point to memory which
> must not be freed (machine->firmware), and then you have
> to check the flow of logic through the code quite carefully
> to figure out whether the condition under which we choose
> to call g_free() is exactly equivalent to the condition
> where we set firmware_filename to point to allocated memory...

Patch sent!

Alistair

>
> thanks
> -- PMM
>