| Message ID | 20190918145640.17349-12-palmer@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,01/48] riscv: sifive_u: Add support for loading initrd | expand |
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 >
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); } }