[PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)

[PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)

[Kevin Wolf]

When I restricted the section where the current monitor is set to only
the command handler, I missed that monitor_parse_arguments() can use it
indirectly, too, when evaluating register variables. These cases get
NULL now and crash (easy to reproduce with "x $pc").

This series passes the right monitor object down instead of using
monitor_cur(), which fixes the crash.

Kevin Wolf (3):
  hmp: Pass monitor to mon_get_cpu()
  hmp: Pass monitor to MonitorDef.get_value()
  hmp: Pass monitor to mon_get_cpu_env()

 include/monitor/hmp-target.h |  7 ++++---
 monitor/monitor-internal.h   |  2 +-
 monitor/hmp.c                |  2 +-
 monitor/misc.c               | 24 ++++++++++++------------
 target/i386/monitor.c        | 11 ++++++-----
 target/m68k/monitor.c        |  2 +-
 target/nios2/monitor.c       |  2 +-
 target/ppc/monitor.c         | 22 +++++++++++++---------
 target/riscv/monitor.c       |  2 +-
 target/sh4/monitor.c         |  2 +-
 target/sparc/monitor.c       | 12 +++++++-----
 target/xtensa/monitor.c      |  2 +-
 12 files changed, 49 insertions(+), 41 deletions(-)

-- 
2.28.0

[PATCH for-5.2 1/3] hmp: Pass monitor to mon_get_cpu()

[Kevin Wolf]

mon_get_cpu() is indirectly called monitor_parse_arguments() where
the current monitor isn't set yet. Instead of using monitor_cur(),
explicitly pass the Monitor pointer to the function.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/monitor/hmp-target.h |  2 +-
 monitor/monitor-internal.h   |  2 +-
 monitor/hmp.c                |  2 +-
 monitor/misc.c               | 18 +++++++++---------
 target/i386/monitor.c        |  2 +-
 5 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 8b7820a3ad..519616d1fb 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -41,7 +41,7 @@ const MonitorDef *target_monitor_defs(void);
 int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
 
 CPUArchState *mon_get_cpu_env(void);
-CPUState *mon_get_cpu(void);
+CPUState *mon_get_cpu(Monitor *mon);
 
 void hmp_info_mem(Monitor *mon, const QDict *qdict);
 void hmp_info_tlb(Monitor *mon, const QDict *qdict);
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index ad2e64be13..a6131554da 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -178,7 +178,7 @@ void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
 void monitor_data_destroy_qmp(MonitorQMP *mon);
 void coroutine_fn monitor_qmp_dispatcher_co(void *data);
 
-int get_monitor_def(int64_t *pval, const char *name);
+int get_monitor_def(Monitor *mon, int64_t *pval, const char *name);
 void help_cmd(Monitor *mon, const char *name);
 void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
 int hmp_compare_cmd(const char *name, const char *list);
diff --git a/monitor/hmp.c b/monitor/hmp.c
index c5cd9d372b..1204233999 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -384,7 +384,7 @@ static int64_t expr_unary(Monitor *mon)
                 pch++;
             }
             *q = 0;
-            ret = get_monitor_def(&reg, buf);
+            ret = get_monitor_def(mon, &reg, buf);
             if (ret < 0) {
                 expr_error(mon, "unknown register");
             }
diff --git a/monitor/misc.c b/monitor/misc.c
index 32e6a8c13d..c918d6bd08 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -289,14 +289,14 @@ static CPUState *mon_get_cpu_sync(Monitor *mon, bool synchronize)
     return cpu;
 }
 
-CPUState *mon_get_cpu(void)
+CPUState *mon_get_cpu(Monitor *mon)
 {
-    return mon_get_cpu_sync(monitor_cur(), true);
+    return mon_get_cpu_sync(mon, true);
 }
 
 CPUArchState *mon_get_cpu_env(void)
 {
-    CPUState *cs = mon_get_cpu();
+    CPUState *cs = mon_get_cpu(monitor_cur());
 
     return cs ? cs->env_ptr : NULL;
 }
@@ -319,7 +319,7 @@ static void hmp_info_registers(Monitor *mon, const QDict *qdict)
             cpu_dump_state(cs, NULL, CPU_DUMP_FPU);
         }
     } else {
-        cs = mon_get_cpu();
+        cs = mon_get_cpu(mon);
 
         if (!cs) {
             monitor_printf(mon, "No CPU available\n");
@@ -381,7 +381,7 @@ static void hmp_info_history(Monitor *mon, const QDict *qdict)
 
 static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
 {
-    CPUState *cs = mon_get_cpu();
+    CPUState *cs = mon_get_cpu(mon);
 
     if (!cs) {
         monitor_printf(mon, "No CPU available\n");
@@ -546,7 +546,7 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
     int l, line_size, i, max_digits, len;
     uint8_t buf[16];
     uint64_t v;
-    CPUState *cs = mon_get_cpu();
+    CPUState *cs = mon_get_cpu(mon);
 
     if (!cs && (format == 'i' || !is_physical)) {
         monitor_printf(mon, "Can not dump without CPU\n");
@@ -711,7 +711,7 @@ static void hmp_gva2gpa(Monitor *mon, const QDict *qdict)
 {
     target_ulong addr = qdict_get_int(qdict, "addr");
     MemTxAttrs attrs;
-    CPUState *cs = mon_get_cpu();
+    CPUState *cs = mon_get_cpu(mon);
     hwaddr gpa;
 
     if (!cs) {
@@ -1663,10 +1663,10 @@ HMPCommand hmp_cmds[] = {
  * Set @pval to the value in the register identified by @name.
  * return 0 if OK, -1 if not found
  */
-int get_monitor_def(int64_t *pval, const char *name)
+int get_monitor_def(Monitor *mon, int64_t *pval, const char *name)
 {
     const MonitorDef *md = target_monitor_defs();
-    CPUState *cs = mon_get_cpu();
+    CPUState *cs = mon_get_cpu(mon);
     void *ptr;
     uint64_t tmp = 0;
     int ret;
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 7abae3c8df..5ca3cab4ec 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -656,7 +656,7 @@ void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
         int id = qdict_get_try_int(qdict, "apic-id", 0);
         cs = cpu_by_arch_id(id);
     } else {
-        cs = mon_get_cpu();
+        cs = mon_get_cpu(mon);
     }
 
 
-- 
2.28.0

[PATCH for-5.2 2/3] hmp: Pass monitor to MonitorDef.get_value()

[Kevin Wolf]

All of these callbacks use mon_get_cpu_env(). Pass the Monitor
pointer to them it in preparation for adding a monitor argument to
mon_get_cpu_env().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/monitor/hmp-target.h |  3 ++-
 monitor/misc.c               |  2 +-
 target/i386/monitor.c        |  3 ++-
 target/ppc/monitor.c         | 12 ++++++++----
 target/sparc/monitor.c       |  6 ++++--
 5 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 519616d1fb..385fb18664 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -33,7 +33,8 @@
 struct MonitorDef {
     const char *name;
     int offset;
-    target_long (*get_value)(const struct MonitorDef *md, int val);
+    target_long (*get_value)(Monitor *mon, const struct MonitorDef *md,
+                             int val);
     int type;
 };
 
diff --git a/monitor/misc.c b/monitor/misc.c
index c918d6bd08..f566e28174 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -1678,7 +1678,7 @@ int get_monitor_def(Monitor *mon, int64_t *pval, const char *name)
     for(; md->name != NULL; md++) {
         if (hmp_compare_cmd(name, md->name)) {
             if (md->get_value) {
-                *pval = md->get_value(md, md->offset);
+                *pval = md->get_value(mon, md, md->offset);
             } else {
                 CPUArchState *env = mon_get_cpu_env();
                 ptr = (uint8_t *)env + md->offset;
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 5ca3cab4ec..fed4606aeb 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -601,7 +601,8 @@ void hmp_mce(Monitor *mon, const QDict *qdict)
     }
 }
 
-static target_long monitor_get_pc(const struct MonitorDef *md, int val)
+static target_long monitor_get_pc(Monitor *mon, const struct MonitorDef *md,
+                                  int val)
 {
     CPUArchState *env = mon_get_cpu_env();
     return env->eip + env->segs[R_CS].base;
diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c
index a5a177d717..9c0fc2b8c3 100644
--- a/target/ppc/monitor.c
+++ b/target/ppc/monitor.c
@@ -29,7 +29,8 @@
 #include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
 
-static target_long monitor_get_ccr(const struct MonitorDef *md, int val)
+static target_long monitor_get_ccr(Monitor *mon, const struct MonitorDef *md,
+                                   int val)
 {
     CPUArchState *env = mon_get_cpu_env();
     unsigned int u;
@@ -43,19 +44,22 @@ static target_long monitor_get_ccr(const struct MonitorDef *md, int val)
     return u;
 }
 
-static target_long monitor_get_decr(const struct MonitorDef *md, int val)
+static target_long monitor_get_decr(Monitor *mon, const struct MonitorDef *md,
+                                    int val)
 {
     CPUArchState *env = mon_get_cpu_env();
     return cpu_ppc_load_decr(env);
 }
 
-static target_long monitor_get_tbu(const struct MonitorDef *md, int val)
+static target_long monitor_get_tbu(Monitor *mon, const struct MonitorDef *md,
+                                   int val)
 {
     CPUArchState *env = mon_get_cpu_env();
     return cpu_ppc_load_tbu(env);
 }
 
-static target_long monitor_get_tbl(const struct MonitorDef *md, int val)
+static target_long monitor_get_tbl(Monitor *mon, const struct MonitorDef *md,
+                                   int val)
 {
     CPUArchState *env = mon_get_cpu_env();
     return cpu_ppc_load_tbl(env);
diff --git a/target/sparc/monitor.c b/target/sparc/monitor.c
index a7ea287cbc..bf979d6520 100644
--- a/target/sparc/monitor.c
+++ b/target/sparc/monitor.c
@@ -40,7 +40,8 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 }
 
 #ifndef TARGET_SPARC64
-static target_long monitor_get_psr (const struct MonitorDef *md, int val)
+static target_long monitor_get_psr(Monitor *mon, const struct MonitorDef *md,
+                                   int val)
 {
     CPUArchState *env = mon_get_cpu_env();
 
@@ -48,7 +49,8 @@ static target_long monitor_get_psr (const struct MonitorDef *md, int val)
 }
 #endif
 
-static target_long monitor_get_reg(const struct MonitorDef *md, int val)
+static target_long monitor_get_reg(Monitor *mon, const struct MonitorDef *md,
+                                   int val)
 {
     CPUArchState *env = mon_get_cpu_env();
     return env->regwptr[val];
-- 
2.28.0

[PATCH for-5.2 3/3] hmp: Pass monitor to mon_get_cpu_env()

[Kevin Wolf]

mon_get_cpu_env() is indirectly called monitor_parse_arguments() where
the current monitor isn't set yet. Instead of using monitor_cur_env(),
explicitly pass the Monitor pointer to the function.

Without this fix, an HMP command like "x $pc" crashes like this:

  #0  0x0000555555caa01f in mon_get_cpu_sync (mon=0x0, synchronize=true) at ../monitor/misc.c:270
  #1  0x0000555555caa141 in mon_get_cpu (mon=0x0) at ../monitor/misc.c:294
  #2  0x0000555555caa158 in mon_get_cpu_env () at ../monitor/misc.c:299
  #3  0x0000555555b19739 in monitor_get_pc (mon=0x555556ad2de0, md=0x5555565d2d40 <monitor_defs+1152>, val=0) at ../target/i386/monitor.c:607
  #4  0x0000555555cadbec in get_monitor_def (mon=0x555556ad2de0, pval=0x7fffffffc208, name=0x7fffffffc220 "pc") at ../monitor/misc.c:1681
  #5  0x000055555582ec4f in expr_unary (mon=0x555556ad2de0) at ../monitor/hmp.c:387
  #6  0x000055555582edbb in expr_prod (mon=0x555556ad2de0) at ../monitor/hmp.c:421
  #7  0x000055555582ee79 in expr_logic (mon=0x555556ad2de0) at ../monitor/hmp.c:455
  #8  0x000055555582eefe in expr_sum (mon=0x555556ad2de0) at ../monitor/hmp.c:484
  #9  0x000055555582efe8 in get_expr (mon=0x555556ad2de0, pval=0x7fffffffc418, pp=0x7fffffffc408) at ../monitor/hmp.c:511
  #10 0x000055555582fcd4 in monitor_parse_arguments (mon=0x555556ad2de0, endp=0x7fffffffc890, cmd=0x555556675b50 <hmp_cmds+7920>) at ../monitor/hmp.c:876
  #11 0x00005555558306a8 in handle_hmp_command (mon=0x555556ad2de0, cmdline=0x555556ada452 "$pc") at ../monitor/hmp.c:1087
  #12 0x000055555582df14 in monitor_command_cb (opaque=0x555556ad2de0, cmdline=0x555556ada450 "x $pc", readline_opaque=0x0) at ../monitor/hmp.c:47

After this fix, nothing is left in monitor_parse_arguments() that can
indirectly call monitor_cur(), so the fix is complete.

Fixes: ff04108a0e36e822519c517bd3bddbc1c7747c18
Reported-by: lichun <lichun@ruijie.com.cn>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/monitor/hmp-target.h |  2 +-
 monitor/misc.c               |  6 +++---
 target/i386/monitor.c        |  6 +++---
 target/m68k/monitor.c        |  2 +-
 target/nios2/monitor.c       |  2 +-
 target/ppc/monitor.c         | 10 +++++-----
 target/riscv/monitor.c       |  2 +-
 target/sh4/monitor.c         |  2 +-
 target/sparc/monitor.c       |  6 +++---
 target/xtensa/monitor.c      |  2 +-
 10 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 385fb18664..60fc92722a 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -41,7 +41,7 @@ struct MonitorDef {
 const MonitorDef *target_monitor_defs(void);
 int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
 
-CPUArchState *mon_get_cpu_env(void);
+CPUArchState *mon_get_cpu_env(Monitor *mon);
 CPUState *mon_get_cpu(Monitor *mon);
 
 void hmp_info_mem(Monitor *mon, const QDict *qdict);
diff --git a/monitor/misc.c b/monitor/misc.c
index f566e28174..398211a034 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -294,9 +294,9 @@ CPUState *mon_get_cpu(Monitor *mon)
     return mon_get_cpu_sync(mon, true);
 }
 
-CPUArchState *mon_get_cpu_env(void)
+CPUArchState *mon_get_cpu_env(Monitor *mon)
 {
-    CPUState *cs = mon_get_cpu(monitor_cur());
+    CPUState *cs = mon_get_cpu(mon);
 
     return cs ? cs->env_ptr : NULL;
 }
@@ -1680,7 +1680,7 @@ int get_monitor_def(Monitor *mon, int64_t *pval, const char *name)
             if (md->get_value) {
                 *pval = md->get_value(mon, md, md->offset);
             } else {
-                CPUArchState *env = mon_get_cpu_env();
+                CPUArchState *env = mon_get_cpu_env(mon);
                 ptr = (uint8_t *)env + md->offset;
                 switch(md->type) {
                 case MD_I32:
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index fed4606aeb..9f9e1c42f4 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -222,7 +222,7 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env;
 
-    env = mon_get_cpu_env();
+    env = mon_get_cpu_env(mon);
     if (!env) {
         monitor_printf(mon, "No CPU available\n");
         return;
@@ -550,7 +550,7 @@ void hmp_info_mem(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env;
 
-    env = mon_get_cpu_env();
+    env = mon_get_cpu_env(mon);
     if (!env) {
         monitor_printf(mon, "No CPU available\n");
         return;
@@ -604,7 +604,7 @@ void hmp_mce(Monitor *mon, const QDict *qdict)
 static target_long monitor_get_pc(Monitor *mon, const struct MonitorDef *md,
                                   int val)
 {
-    CPUArchState *env = mon_get_cpu_env();
+    CPUArchState *env = mon_get_cpu_env(mon);
     return env->eip + env->segs[R_CS].base;
 }
 
diff --git a/target/m68k/monitor.c b/target/m68k/monitor.c
index 2055fe8a00..2bdf6acae0 100644
--- a/target/m68k/monitor.c
+++ b/target/m68k/monitor.c
@@ -12,7 +12,7 @@
 
 void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 {
-    CPUArchState *env1 = mon_get_cpu_env();
+    CPUArchState *env1 = mon_get_cpu_env(mon);
 
     if (!env1) {
         monitor_printf(mon, "No CPU available\n");
diff --git a/target/nios2/monitor.c b/target/nios2/monitor.c
index 6646836df5..0152dec3fa 100644
--- a/target/nios2/monitor.c
+++ b/target/nios2/monitor.c
@@ -29,7 +29,7 @@
 
 void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 {
-    CPUArchState *env1 = mon_get_cpu_env();
+    CPUArchState *env1 = mon_get_cpu_env(mon);
 
     dump_mmu(env1);
 }
diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c
index 9c0fc2b8c3..a475108b2d 100644
--- a/target/ppc/monitor.c
+++ b/target/ppc/monitor.c
@@ -32,7 +32,7 @@
 static target_long monitor_get_ccr(Monitor *mon, const struct MonitorDef *md,
                                    int val)
 {
-    CPUArchState *env = mon_get_cpu_env();
+    CPUArchState *env = mon_get_cpu_env(mon);
     unsigned int u;
     int i;
 
@@ -47,27 +47,27 @@ static target_long monitor_get_ccr(Monitor *mon, const struct MonitorDef *md,
 static target_long monitor_get_decr(Monitor *mon, const struct MonitorDef *md,
                                     int val)
 {
-    CPUArchState *env = mon_get_cpu_env();
+    CPUArchState *env = mon_get_cpu_env(mon);
     return cpu_ppc_load_decr(env);
 }
 
 static target_long monitor_get_tbu(Monitor *mon, const struct MonitorDef *md,
                                    int val)
 {
-    CPUArchState *env = mon_get_cpu_env();
+    CPUArchState *env = mon_get_cpu_env(mon);
     return cpu_ppc_load_tbu(env);
 }
 
 static target_long monitor_get_tbl(Monitor *mon, const struct MonitorDef *md,
                                    int val)
 {
-    CPUArchState *env = mon_get_cpu_env();
+    CPUArchState *env = mon_get_cpu_env(mon);
     return cpu_ppc_load_tbl(env);
 }
 
 void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 {
-    CPUArchState *env1 = mon_get_cpu_env();
+    CPUArchState *env1 = mon_get_cpu_env(mon);
 
     if (!env1) {
         monitor_printf(mon, "No CPU available\n");
diff --git a/target/riscv/monitor.c b/target/riscv/monitor.c
index b569f08387..e51188f919 100644
--- a/target/riscv/monitor.c
+++ b/target/riscv/monitor.c
@@ -204,7 +204,7 @@ void hmp_info_mem(Monitor *mon, const QDict *qdict)
 {
     CPUArchState *env;
 
-    env = mon_get_cpu_env();
+    env = mon_get_cpu_env(mon);
     if (!env) {
         monitor_printf(mon, "No CPU available\n");
         return;
diff --git a/target/sh4/monitor.c b/target/sh4/monitor.c
index 918a5ccfc6..2da6a5426e 100644
--- a/target/sh4/monitor.c
+++ b/target/sh4/monitor.c
@@ -41,7 +41,7 @@ static void print_tlb(Monitor *mon, int idx, tlb_t *tlb)
 
 void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 {
-    CPUArchState *env = mon_get_cpu_env();
+    CPUArchState *env = mon_get_cpu_env(mon);
     int i;
 
     if (!env) {
diff --git a/target/sparc/monitor.c b/target/sparc/monitor.c
index bf979d6520..318413686a 100644
--- a/target/sparc/monitor.c
+++ b/target/sparc/monitor.c
@@ -30,7 +30,7 @@
 
 void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 {
-    CPUArchState *env1 = mon_get_cpu_env();
+    CPUArchState *env1 = mon_get_cpu_env(mon);
 
     if (!env1) {
         monitor_printf(mon, "No CPU available\n");
@@ -43,7 +43,7 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 static target_long monitor_get_psr(Monitor *mon, const struct MonitorDef *md,
                                    int val)
 {
-    CPUArchState *env = mon_get_cpu_env();
+    CPUArchState *env = mon_get_cpu_env(mon);
 
     return cpu_get_psr(env);
 }
@@ -52,7 +52,7 @@ static target_long monitor_get_psr(Monitor *mon, const struct MonitorDef *md,
 static target_long monitor_get_reg(Monitor *mon, const struct MonitorDef *md,
                                    int val)
 {
-    CPUArchState *env = mon_get_cpu_env();
+    CPUArchState *env = mon_get_cpu_env(mon);
     return env->regwptr[val];
 }
 
diff --git a/target/xtensa/monitor.c b/target/xtensa/monitor.c
index 608173c238..fbf60d5553 100644
--- a/target/xtensa/monitor.c
+++ b/target/xtensa/monitor.c
@@ -29,7 +29,7 @@
 
 void hmp_info_tlb(Monitor *mon, const QDict *qdict)
 {
-    CPUArchState *env1 = mon_get_cpu_env();
+    CPUArchState *env1 = mon_get_cpu_env(mon);
 
     if (!env1) {
         monitor_printf(mon, "No CPU available\n");
-- 
2.28.0

Re: [PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> When I restricted the section where the current monitor is set to only
> the command handler, I missed that monitor_parse_arguments() can use it
> indirectly, too, when evaluating register variables. These cases get
> NULL now and crash (easy to reproduce with "x $pc").
> 
> This series passes the right monitor object down instead of using
> monitor_cur(), which fixes the crash.

Why didn't the test-hmp.c find this?  It has a 'p $pc + 8'

Dave


> Kevin Wolf (3):
>   hmp: Pass monitor to mon_get_cpu()
>   hmp: Pass monitor to MonitorDef.get_value()
>   hmp: Pass monitor to mon_get_cpu_env()
> 
>  include/monitor/hmp-target.h |  7 ++++---
>  monitor/monitor-internal.h   |  2 +-
>  monitor/hmp.c                |  2 +-
>  monitor/misc.c               | 24 ++++++++++++------------
>  target/i386/monitor.c        | 11 ++++++-----
>  target/m68k/monitor.c        |  2 +-
>  target/nios2/monitor.c       |  2 +-
>  target/ppc/monitor.c         | 22 +++++++++++++---------
>  target/riscv/monitor.c       |  2 +-
>  target/sh4/monitor.c         |  2 +-
>  target/sparc/monitor.c       | 12 +++++++-----
>  target/xtensa/monitor.c      |  2 +-
>  12 files changed, 49 insertions(+), 41 deletions(-)
> 
> -- 
> 2.28.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)

[Kevin Wolf]

Am 13.11.2020 um 13:13 hat Dr. David Alan Gilbert geschrieben:
> * Kevin Wolf (kwolf@redhat.com) wrote:
> > When I restricted the section where the current monitor is set to only
> > the command handler, I missed that monitor_parse_arguments() can use it
> > indirectly, too, when evaluating register variables. These cases get
> > NULL now and crash (easy to reproduce with "x $pc").
> > 
> > This series passes the right monitor object down instead of using
> > monitor_cur(), which fixes the crash.
> 
> Why didn't the test-hmp.c find this?  It has a 'p $pc + 8'

Good question, a manual 'p $pc + 8' crashes for me on master.

Aha, it doesn't use a real HMP monitor, but QMP human-monitor-command.
Then it would just get the wrong monitor (the QMP one instead of the
temporary HMP monitor) and not NULL. The accessed CPU is even the same
because neither QMP nor the temporary HMP monitor have a current CPU
set, so even if the test case did check the result, it wouldn't catch
this.

Only if the test case were using multiple CPUs and cpu-index had been
set for human-monitor-command (to something other than the default), we
would get a wrong result. But of course, it still wouldn't crash.

Kevin

Re: [PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Am 13.11.2020 um 13:13 hat Dr. David Alan Gilbert geschrieben:
> > * Kevin Wolf (kwolf@redhat.com) wrote:
> > > When I restricted the section where the current monitor is set to only
> > > the command handler, I missed that monitor_parse_arguments() can use it
> > > indirectly, too, when evaluating register variables. These cases get
> > > NULL now and crash (easy to reproduce with "x $pc").
> > > 
> > > This series passes the right monitor object down instead of using
> > > monitor_cur(), which fixes the crash.
> > 
> > Why didn't the test-hmp.c find this?  It has a 'p $pc + 8'
> 
> Good question, a manual 'p $pc + 8' crashes for me on master.
> 
> Aha, it doesn't use a real HMP monitor, but QMP human-monitor-command.
> Then it would just get the wrong monitor (the QMP one instead of the
> temporary HMP monitor) and not NULL. The accessed CPU is even the same
> because neither QMP nor the temporary HMP monitor have a current CPU
> set, so even if the test case did check the result, it wouldn't catch
> this.
> 
> Only if the test case were using multiple CPUs and cpu-index had been
> set for human-monitor-command (to something other than the default), we
> would get a wrong result. But of course, it still wouldn't crash.

Ah, fair enough.

Dave

> Kevin
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> When I restricted the section where the current monitor is set to only
> the command handler, I missed that monitor_parse_arguments() can use it
> indirectly, too, when evaluating register variables. These cases get
> NULL now and crash (easy to reproduce with "x $pc").
> 
> This series passes the right monitor object down instead of using
> monitor_cur(), which fixes the crash.
> 

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

and queued; I'll send out an HMP pull shortly.

> Kevin Wolf (3):
>   hmp: Pass monitor to mon_get_cpu()
>   hmp: Pass monitor to MonitorDef.get_value()
>   hmp: Pass monitor to mon_get_cpu_env()
> 
>  include/monitor/hmp-target.h |  7 ++++---
>  monitor/monitor-internal.h   |  2 +-
>  monitor/hmp.c                |  2 +-
>  monitor/misc.c               | 24 ++++++++++++------------
>  target/i386/monitor.c        | 11 ++++++-----
>  target/m68k/monitor.c        |  2 +-
>  target/nios2/monitor.c       |  2 +-
>  target/ppc/monitor.c         | 22 +++++++++++++---------
>  target/riscv/monitor.c       |  2 +-
>  target/sh4/monitor.c         |  2 +-
>  target/sparc/monitor.c       | 12 +++++++-----
>  target/xtensa/monitor.c      |  2 +-
>  12 files changed, 49 insertions(+), 41 deletions(-)
> 
> -- 
> 2.28.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK