Thomas Huth <th...@redhat.com> writes: > On 12.01.2017 17:22, Markus Armbruster wrote: >> Thomas Huth <th...@redhat.com> writes: >> >>> When running certain HMP commands ("info registers", "info cpustats", >>> "nmi", "memsave" or dumping virtual memory) with the "none" machine, >>> QEMU crashes with a segmentation fault. This happens because the "none" >>> machine does not have any CPUs by default, but these HMP commands did >>> not check for a valid CPU pointer yet. Add such checks now, so we get >>> an error message about the missing CPU instead. >>> >>> Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com> >>> Signed-off-by: Thomas Huth <th...@redhat.com> >>> --- >>> v3: >>> - Use UNASSIGNED_CPU_INDEX instead of hard-coded -1 >>> v2: >>> - Added more checks to cover "nmi" and "memsave", too >>> >>> hmp.c | 8 +++++++- >>> monitor.c | 37 +++++++++++++++++++++++++++++++------ >>> 2 files changed, 38 insertions(+), 7 deletions(-) >>> >>> diff --git a/hmp.c b/hmp.c >>> index b869617..b1c503a 100644 >>> --- a/hmp.c >>> +++ b/hmp.c >>> @@ -1013,8 +1013,14 @@ void hmp_memsave(Monitor *mon, const QDict *qdict) >>> const char *filename = qdict_get_str(qdict, "filename"); >>> uint64_t addr = qdict_get_int(qdict, "val"); >>> Error *err = NULL; >>> + int cpu_index = monitor_get_cpu_index(); >>> >>> - qmp_memsave(addr, size, filename, true, monitor_get_cpu_index(), &err); >>> + if (cpu_index < 0) { >>> + monitor_printf(mon, "No CPU available\n"); >>> + return; >>> + } >>> + >>> + qmp_memsave(addr, size, filename, true, cpu_index, &err); >>> hmp_handle_error(mon, &err); >>> } >>> >>> diff --git a/monitor.c b/monitor.c >>> index 0841d43..17121ff 100644 >>> --- a/monitor.c >>> +++ b/monitor.c >>> @@ -1025,6 +1025,9 @@ int monitor_set_cpu(int cpu_index) >>> CPUState *mon_get_cpu(void) >>> { >>> if (!cur_mon->mon_cpu) { >>> + if (!first_cpu) { >>> + return NULL; >>> + } >>> monitor_set_cpu(first_cpu->cpu_index); >>> } >>> cpu_synchronize_state(cur_mon->mon_cpu); >>> @@ -1033,17 +1036,27 @@ CPUState *mon_get_cpu(void) >>> >>> CPUArchState *mon_get_cpu_env(void) >>> { >>> - return mon_get_cpu()->env_ptr; >>> + CPUState *cs = mon_get_cpu(); >>> + >>> + return cs ? cs->env_ptr : NULL; >>> } >>> >>> int monitor_get_cpu_index(void) >>> { >>> - return mon_get_cpu()->cpu_index; >>> + CPUState *cs = mon_get_cpu(); >>> + >>> + return cs ? cs->cpu_index : UNASSIGNED_CPU_INDEX; >>> } >>> >>> static void hmp_info_registers(Monitor *mon, const QDict *qdict) >>> { >>> - cpu_dump_state(mon_get_cpu(), (FILE *)mon, monitor_fprintf, >>> CPU_DUMP_FPU); >>> + CPUState *cs = mon_get_cpu(); >>> + >>> + if (!cs) { >>> + monitor_printf(mon, "No CPU available\n"); >>> + return; >>> + } >>> + cpu_dump_state(cs, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU); >>> } >>> >>> static void hmp_info_jit(Monitor *mon, const QDict *qdict) >>> @@ -1076,7 +1089,13 @@ static void hmp_info_history(Monitor *mon, const >>> QDict *qdict) >>> >>> static void hmp_info_cpustats(Monitor *mon, const QDict *qdict) >>> { >>> - cpu_dump_statistics(mon_get_cpu(), (FILE *)mon, &monitor_fprintf, 0); >>> + CPUState *cs = mon_get_cpu(); >>> + >>> + if (!cs) { >>> + monitor_printf(mon, "No CPU available\n"); >>> + return; >>> + } >>> + cpu_dump_statistics(cs, (FILE *)mon, &monitor_fprintf, 0); >>> } >>> >>> static void hmp_info_trace_events(Monitor *mon, const QDict *qdict) >>> @@ -1235,6 +1254,12 @@ 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(); >>> + >>> + if (!cs && (format == 'i' || !is_physical)) { >>> + monitor_printf(mon, "Can not dump without CPU\n"); >>> + return; >>> + } >> >> This is basically "if (we're going to dereference cs)". Not so nice, in >> particular since the dereferences are hidden inside mon_get_cpu_env() >> calls. I guess it'll do. >> >>> >>> if (format == 'i') { >>> int flags = 0; >>> @@ -1264,7 +1289,7 @@ static void memory_dump(Monitor *mon, int count, int >>> format, int wsize, >>> flags = msr_le << 16; >>> flags |= env->bfd_mach; >>> #endif >>> - monitor_disas(mon, mon_get_cpu(), addr, count, is_physical, flags); >>> + monitor_disas(mon, cs, addr, count, is_physical, flags); >>> return; >>> } >>> >>> @@ -1303,7 +1328,7 @@ static void memory_dump(Monitor *mon, int count, int >>> format, int wsize, >>> if (is_physical) { >>> cpu_physical_memory_read(addr, buf, l); >>> } else { >>> - if (cpu_memory_rw_debug(mon_get_cpu(), addr, buf, l, 0) < 0) { >>> + if (cpu_memory_rw_debug(cs, addr, buf, l, 0) < 0) { >>> monitor_printf(mon, " Cannot access memory\n"); >>> break; >>> } >> >> What about get_monitor_def()? >> >> $ qemu-system-x86_64 --nodefaults -S -display none -M none -monitor stdio >> QEMU 2.8.50 monitor - type 'help' for more information >> (qemu) p $pc >> Segmentation fault (core dumped) > > Oh, I saw that get_monitor_def() uses mon_get_cpu_env(), but I only > checked with qemu-system-m68k where "p $pc" simply prints "unknown > register" when you run it with the "none" machine ... that's why I > assumed that no fixes would be needed here. > >> Likewise, info tlb, info mem, ... > > These seem to be target specific, too, so I think they could go into a > separate patch instead. If it's OK for you, I'd like to keep my current > patch as it is, and add these items to > http://qemu-project.org/BiteSizedTasks instead - since these are all > non-urgent problems that should be easy to fix for newcomers, too.
I'm not into rejecting partial fixes to blackmail for more complete fixes. But let's add a FIXME comment next to mon_get_cpu(), to record the issue in the source code, and not just the web site.