Re: [Qemu-devel] [PATCH v4 26/26] RISC-V: Fix riscv_isa_string memory size bug

[Peter Maydell]

On 19 March 2018 at 21:18, Michael Clark <address@hidden> wrote:
> This version uses a constant size memory buffer sized for
> the maximum possible ISA string length. It also uses g_new
> instead of g_new0, uses more efficient logic to append
> extensions and adds manual zero termination of the string.
>
> Cc: Palmer Dabbelt <address@hidden>
> Cc: Peter Maydell <address@hidden>
> Signed-off-by: Michael Clark <address@hidden>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  target/riscv/cpu.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 1f25968..c82359f 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -360,16 +360,16 @@ static void riscv_cpu_class_init(ObjectClass *c, void 
> *data)
>  char *riscv_isa_string(RISCVCPU *cpu)
>  {
>      int i;
> -    size_t maxlen = 5 + ctz32(cpu->env.misa);
> -    char *isa_string = g_new0(char, maxlen);
> -    snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS);
> +    const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
> +    char *isa_str = g_new(char, maxlen);
> +    char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
>      for (i = 0; i < sizeof(riscv_exts); i++) {
>          if (cpu->env.misa & RV(riscv_exts[i])) {
> -            isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a';
> -
> +            *p++ = tolower(riscv_exts[i]);

This should be qemu_tolower(). Plain tolower() has an awkward problem
where if the 'char' type is signed and you hand tolower() a char that
happens to be a negative value you get undefined behaviour. This means
you need to cast the argument to 'unsigned char', which is what our
wrapper does. In this specific case the inputs are known constant
ASCII, so tolower() happens to be safe, but for consistency with
the rest of the codebase we should use our wrapper function.

>          }
>      }
> -    return isa_string;
> +    *p = '\0';
> +    return isa_str;
>  }
>
>  typedef struct RISCVCPUListState {

Since this bug is causing the build tests to intermittently fail,
I'm going to apply this patch directly to master, with tolower()
replaced with qemu_tolower().

thanks
-- PMM