[v1] target/riscv/riscv-qmp-cmds.c: coverity-related fixes

[PATCH] target/riscv/riscv-qmp-cmds.c: coverity-related fixes

Posted by Daniel Henrique Barboza 3 weeks, 2 days ago

Coverity CID 1641401 reports that, in reg_is_ulong_integer(), we're
dereferencing a NULL pointer in "reg1" when using it in strcasecmp()
call. A similar case is reported with CID 1641393.

In theory that will never happen - it's guaranteed that both "reg1" and
"reg2" is non-NULL because we're retrieving them in compile-time from
static arrays. Coverity doesn't know that though.

To make Coverity happier and add a bit more clarity in the code,
g_assert() each token to make it clear that those 2 values aren't
supposed to be NULL ever. Do that in both reg_is_ulong_integer() and
reg_is_u64_fpu().

We're also taking the opportunity to implement suggestions made by Peter
in [1] in both functions:

- use g_strsplit() instead of strtok();
- use g_ascii_strcasecmp() instead of strcasecmp().

[1] https://lore.kernel.org/qemu-devel/CAFEAcA_y4bwd9GANbXnpTy2mv80Vg_jp+A-VkQS5V6f0+BFRAA@mail.gmail.com/

Coverity: CID 1641393, 1641401
Fixes: e06d209aa6 ("target/riscv: implement MonitorDef HMP API")
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/riscv-qmp-cmds.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
index c499f9b9a7..7bde7090ab 100644
--- a/target/riscv/riscv-qmp-cmds.c
+++ b/target/riscv/riscv-qmp-cmds.c
@@ -273,12 +273,13 @@ static bool reg_is_ulong_integer(CPURISCVState *env, const char *name,
     }
 
     for (int i = 0; i < 32; i++) {
-        g_autofree char *reg_name = g_strdup(reg_names[i]);
-        char *reg1 = strtok(reg_name, "/");
-        char *reg2 = strtok(NULL, "/");
+        g_autofree char **reg_name = g_strsplit(reg_names[i], "/", 2);
 
-        if (strcasecmp(reg1, name) == 0 ||
-            (reg2 && strcasecmp(reg2, name) == 0)) {
+        g_assert(reg_name[0]);
+        g_assert(reg_name[1]);
+
+        if (g_ascii_strcasecmp(reg_name[0], name) == 0 ||
+            g_ascii_strcasecmp(reg_name[1], name) == 0) {
             *val = vals[i];
             return true;
         }
@@ -294,12 +295,13 @@ static bool reg_is_u64_fpu(CPURISCVState *env, const char *name, uint64_t *val)
     }
 
     for (int i = 0; i < 32; i++) {
-        g_autofree char *reg_name = g_strdup(riscv_fpr_regnames[i]);
-        char *reg1 = strtok(reg_name, "/");
-        char *reg2 = strtok(NULL, "/");
+        g_autofree char **reg_name = g_strsplit(riscv_fpr_regnames[i], "/", 2);
+
+        g_assert(reg_name[0]);
+        g_assert(reg_name[1]);
 
-        if (strcasecmp(reg1, name) == 0 ||
-            (reg2 && strcasecmp(reg2, name) == 0)) {
+        if (g_ascii_strcasecmp(reg_name[0], name) == 0 ||
+            g_ascii_strcasecmp(reg_name[1], name) == 0) {
             *val = env->fpr[i];
             return true;
         }
-- 
2.51.0

Re: [PATCH] target/riscv/riscv-qmp-cmds.c: coverity-related fixes

Posted by Peter Maydell 3 weeks, 2 days ago

On Wed, 22 Oct 2025 at 12:05, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Coverity CID 1641401 reports that, in reg_is_ulong_integer(), we're
> dereferencing a NULL pointer in "reg1" when using it in strcasecmp()
> call. A similar case is reported with CID 1641393.
>
> In theory that will never happen - it's guaranteed that both "reg1" and
> "reg2" is non-NULL because we're retrieving them in compile-time from
> static arrays. Coverity doesn't know that though.
>
> To make Coverity happier and add a bit more clarity in the code,
> g_assert() each token to make it clear that those 2 values aren't
> supposed to be NULL ever. Do that in both reg_is_ulong_integer() and
> reg_is_u64_fpu().
>
> We're also taking the opportunity to implement suggestions made by Peter
> in [1] in both functions:
>
> - use g_strsplit() instead of strtok();
> - use g_ascii_strcasecmp() instead of strcasecmp().
>
> [1] https://lore.kernel.org/qemu-devel/CAFEAcA_y4bwd9GANbXnpTy2mv80Vg_jp+A-VkQS5V6f0+BFRAA@mail.gmail.com/
>
> Coverity: CID 1641393, 1641401
> Fixes: e06d209aa6 ("target/riscv: implement MonitorDef HMP API")
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/riscv-qmp-cmds.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
> index c499f9b9a7..7bde7090ab 100644
> --- a/target/riscv/riscv-qmp-cmds.c
> +++ b/target/riscv/riscv-qmp-cmds.c
> @@ -273,12 +273,13 @@ static bool reg_is_ulong_integer(CPURISCVState *env, const char *name,
>      }
>
>      for (int i = 0; i < 32; i++) {
> -        g_autofree char *reg_name = g_strdup(reg_names[i]);
> -        char *reg1 = strtok(reg_name, "/");
> -        char *reg2 = strtok(NULL, "/");
> +        g_autofree char **reg_name = g_strsplit(reg_names[i], "/", 2);

g_autofree frees with g_free(), which isn't the right thing for
freeing an array of pointers. To do autofree here we need

 g_auto(GStrv) reg_name = g_strsplit(...);

(GStrv is the glib type for gchar**; the headers give it an
auto-free mechanism that calls g_strfreev().)

thanks
-- PMM

Re: [PATCH] target/riscv/riscv-qmp-cmds.c: coverity-related fixes

Posted by Daniel Henrique Barboza 3 weeks, 2 days ago


On 10/22/25 8:59 AM, Peter Maydell wrote:
> On Wed, 22 Oct 2025 at 12:05, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> Coverity CID 1641401 reports that, in reg_is_ulong_integer(), we're
>> dereferencing a NULL pointer in "reg1" when using it in strcasecmp()
>> call. A similar case is reported with CID 1641393.
>>
>> In theory that will never happen - it's guaranteed that both "reg1" and
>> "reg2" is non-NULL because we're retrieving them in compile-time from
>> static arrays. Coverity doesn't know that though.
>>
>> To make Coverity happier and add a bit more clarity in the code,
>> g_assert() each token to make it clear that those 2 values aren't
>> supposed to be NULL ever. Do that in both reg_is_ulong_integer() and
>> reg_is_u64_fpu().
>>
>> We're also taking the opportunity to implement suggestions made by Peter
>> in [1] in both functions:
>>
>> - use g_strsplit() instead of strtok();
>> - use g_ascii_strcasecmp() instead of strcasecmp().
>>
>> [1] https://lore.kernel.org/qemu-devel/CAFEAcA_y4bwd9GANbXnpTy2mv80Vg_jp+A-VkQS5V6f0+BFRAA@mail.gmail.com/
>>
>> Coverity: CID 1641393, 1641401
>> Fixes: e06d209aa6 ("target/riscv: implement MonitorDef HMP API")
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/riscv-qmp-cmds.c | 22 ++++++++++++----------
>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
>> index c499f9b9a7..7bde7090ab 100644
>> --- a/target/riscv/riscv-qmp-cmds.c
>> +++ b/target/riscv/riscv-qmp-cmds.c
>> @@ -273,12 +273,13 @@ static bool reg_is_ulong_integer(CPURISCVState *env, const char *name,
>>       }
>>
>>       for (int i = 0; i < 32; i++) {
>> -        g_autofree char *reg_name = g_strdup(reg_names[i]);
>> -        char *reg1 = strtok(reg_name, "/");
>> -        char *reg2 = strtok(NULL, "/");
>> +        g_autofree char **reg_name = g_strsplit(reg_names[i], "/", 2);
> 
> g_autofree frees with g_free(), which isn't the right thing for
> freeing an array of pointers. To do autofree here we need
> 
>   g_auto(GStrv) reg_name = g_strsplit(...);
> 
> (GStrv is the glib type for gchar**; the headers give it an
> auto-free mechanism that calls g_strfreev().)

I saw some instances of "g_autofree char **" and figured that it was ok to use
g_autofree in this case too (just git grepping, didn't dive deeper to figure
out context and so on).

I'll send a v2 with g_auto(GSrv). Thanks,


Daniel




  

> 
> thanks
> -- PMM

Re: [PATCH] target/riscv/riscv-qmp-cmds.c: coverity-related fixes

Posted by Peter Maydell 3 weeks, 1 day ago

On Wed, 22 Oct 2025 at 13:39, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
> On 10/22/25 8:59 AM, Peter Maydell wrote:
> > g_autofree frees with g_free(), which isn't the right thing for
> > freeing an array of pointers. To do autofree here we need
> >
> >   g_auto(GStrv) reg_name = g_strsplit(...);
> >
> > (GStrv is the glib type for gchar**; the headers give it an
> > auto-free mechanism that calls g_strfreev().)
>
> I saw some instances of "g_autofree char **" and figured that it was ok to use
> g_autofree in this case too (just git grepping, didn't dive deeper to figure
> out context and so on).

There might be some lurking bugs here -- it's an easy
mistake to make. I did a quick audit on the results of
 git grep 'g_autofree.*\*\*'

It looks like migration/migration-hmp-cmds.c:hmp_migrate_set_parameter()
does the wrong thing when it calls g_shell_parse_argv(),
because that function documents that the vector should
be freed with g_strfreev().

All the others are OK, because the elements of the
array are either manually dereffed or else remain
the ownership of something else and shouldn't be freed.

The migration one is a recent addition so I'll follow
up to the patch where it got added to the codebase.

thanks
-- PMM