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

[Michael Clark]

On Tue, Mar 20, 2018 at 1:51 PM, Philippe Mathieu-Daudé <address@hidden>
wrote:

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

Okay. I'll drop the patch from my post-merge spec conformance fixes series.

I've addressed all feedback on the post-merge spec conformance series so
i'll make a PR for it...

... these are the extra commits that were erroneously included in the v8.2
pull. They now all have sign-offs... they've been in the riscv tree for a
while and have had a lot of testing...