Re: [Qemu-devel] [PATCH v8 16/23] RISC-V Spike Machines

[Peter Maydell <peter.maydell@linaro.org>]

On 2 March 2018 at 13:51, Michael Clark <mjc@sifive.com> wrote:
> RISC-V machines compatble with Spike aka riscv-isa-sim, the RISC-V
> Instruction Set Simulator. The following machines are implemented:
>
> - 'spike_v1.9.1'; HTIF console, config-string, Privileged ISA Version 1.9.1
> - 'spike_v1.10'; HTIF console, device-tree, Privileged ISA Version 1.10
>
> Acked-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Signed-off-by: Michael Clark <mjc@sifive.com>

Hi; Coverity (CID 1391015) thinks there's a memory leak here:

> +    /* part one of config string - before memory size specified */
> +    const char *config_string_tmpl =
> +        "platform {\n"
> +        "  vendor ucb;\n"
> +        "  arch spike;\n"
> +        "};\n"
> +        "rtc {\n"
> +        "  addr 0x%" PRIx64 "x;\n"
> +        "};\n"
> +        "ram {\n"
> +        "  0 {\n"
> +        "    addr 0x%" PRIx64 "x;\n"
> +        "    size 0x%" PRIx64 "x;\n"
> +        "  };\n"
> +        "};\n"
> +        "core {\n"
> +        "  0" " {\n"
> +        "    " "0 {\n"
> +        "      isa %s;\n"
> +        "      timecmp 0x%" PRIx64 "x;\n"
> +        "      ipi 0x%" PRIx64 "x;\n"
> +        "    };\n"
> +        "  };\n"
> +        "};\n";
> +
> +    /* build config string with supplied memory size */
> +    char *isa = riscv_isa_string(&s->soc.harts[0]);
> +    size_t config_string_size = strlen(config_string_tmpl) + 48;
> +    char *config_string = malloc(config_string_size);

We malloc() config_string here...

> +    snprintf(config_string, config_string_size, config_string_tmpl,
> +        (uint64_t)memmap[SPIKE_CLINT].base + SIFIVE_TIME_BASE,
> +        (uint64_t)memmap[SPIKE_DRAM].base,
> +        (uint64_t)ram_size, isa,
> +        (uint64_t)memmap[SPIKE_CLINT].base + SIFIVE_TIMECMP_BASE,
> +        (uint64_t)memmap[SPIKE_CLINT].base + SIFIVE_SIP_BASE);
> +    g_free(isa);
> +    size_t config_string_len = strlen(config_string);
> +
> +    /* copy in the reset vector */
> +    copy_le32_to_phys(memmap[SPIKE_MROM].base, reset_vec, sizeof(reset_vec));
> +
> +    /* copy in the config string */
> +    cpu_physical_memory_write(memmap[SPIKE_MROM].base + sizeof(reset_vec),
> +        config_string, config_string_len);

...and finish using it here, but we never free it.

We also don't check that malloc() succeeded, so we'll crash if it
returns NULL. The recommended fix for this is to use g_malloc()
instead.

thanks
-- PMM