On 13.01.2017 08:59, Markus Armbruster wrote: > 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.
Hmm, I just changed my mind. Before I respin the patch with a FIXME comment + update the website, I think it's faster if I just add those missing checks directly. ... so I'll send a v4 with those checks added... Thomas