| Message ID | 20201113114326.97663-1-kwolf@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hmp: Fix arg evaluation crash (regression) | expand |
* 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 >
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
* 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
* 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 >