1. Patchew
  2. QEMU
  3. View series

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

Daniel Henrique Barboza posted 1 patch 3 weeks, 2 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20251022125643.588947-1-dbarboza@ventanamicro.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
target/riscv/riscv-qmp-cmds.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
[PATCH v2] 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>
---

Changes from v1:
- use g_auto(GStrv) instead of g_autofree
- v1 link: https://lore.kernel.org/qemu-riscv/20251022110524.483588-1-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..d5e9bec0f8 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_auto(GStrv) 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_auto(GStrv) 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 v2] target/riscv/riscv-qmp-cmds.c: coverity-related fixes
Posted by Alistair Francis 3 weeks, 1 day ago 
On Wed, Oct 22, 2025 at 10:57 PM 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>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>
> Changes from v1:
> - use g_auto(GStrv) instead of g_autofree
> - v1 link: https://lore.kernel.org/qemu-riscv/20251022110524.483588-1-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..d5e9bec0f8 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_auto(GStrv) 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_auto(GStrv) 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 v2] target/riscv/riscv-qmp-cmds.c: coverity-related fixes
Posted by Alistair Francis 3 weeks, 1 day ago 
On Wed, Oct 22, 2025 at 10:57 PM 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>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>
> Changes from v1:
> - use g_auto(GStrv) instead of g_autofree
> - v1 link: https://lore.kernel.org/qemu-riscv/20251022110524.483588-1-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..d5e9bec0f8 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_auto(GStrv) 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_auto(GStrv) 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
>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.