[v4,26/26] RISC-V: Fix riscv_isa_string memory size bug

Commit Message

Michael Clark March 19, 2018, 9:18 p.m. UTC

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 <[e-mail]>
Cc: Peter Maydell <[e-mail]>
Signed-off-by: Michael Clark <[e-mail]>
Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
---
 target/riscv/cpu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Peter Maydell March 20, 2018, 11:51 a.m. UTC | #1

On 19 March 2018 at 21:18, Michael Clark <[e-mail]> 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 <[e-mail]>
> Cc: Peter Maydell <[e-mail]>
> Signed-off-by: Michael Clark <[e-mail]>
> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> ---
>  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

Philippe Mathieu-Daudé March 20, 2018, 8:51 p.m. UTC | #2

Le mar. 20 mars 2018 12:52, Peter Maydell <[e-mail]> a
écrit :

> On 19 March 2018 at 21:18, Michael Clark <[e-mail]> 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 <[e-mail]>
> > Cc: Peter Maydell <[e-mail]>
> > Signed-off-by: Michael Clark <[e-mail]>
> > Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> > ---
> >  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
>
>

Michael Clark March 20, 2018, 9:35 p.m. UTC | #3

On Tue, Mar 20, 2018 at 1:51 PM, Philippe Mathieu-Daudé <[e-mail]>
wrote:

> Le mar. 20 mars 2018 12:52, Peter Maydell <[e-mail]> a
> écrit :
>
>> On 19 March 2018 at 21:18, Michael Clark <[e-mail]> 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 <[e-mail]>
>> > Cc: Peter Maydell <[e-mail]>
>> > Signed-off-by: Michael Clark <[e-mail]>
>> > Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
>> > ---
>> >  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...

Patch

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]);
         }
     }
-    return isa_string;
+    *p = '\0';
+    return isa_str;
 }
 
 typedef struct RISCVCPUListState {