On Tue, Sep 24, 2019 at 3:18 AM Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Wed, 18 Sep 2019 at 16:35, Palmer Dabbelt <pal...@sifive.com> wrote: > > > > From: Bin Meng <bmeng...@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...@gmail.com> > > Reviewed-by: Alistair Francis <alistair.fran...@wdc.com> > > Signed-off-by: Palmer Dabbelt <pal...@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 >