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

[Philippe Mathieu-Daudé]

Le mar. 20 mars 2018 12:52, Peter Maydell <address@hidden> a
écrit :

> 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
>

Oh, good to know.

wrapper does. In this specific case the inputs are known constant
> ASCII, so tolower() happens to be safe,


Yep.

but for consistency with
> the rest of the codebase we should use our wrapper function.
>

Ok.


> >          }
> >      }
> > -    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 Peter!


> thanks
> -- PMM
>
>