qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2] monitor: Fix crashes when using HMP commands


From: Thomas Huth
Subject: Re: [Qemu-devel] [PATCH v2] monitor: Fix crashes when using HMP commands without CPU
Date: Thu, 12 Jan 2017 13:28:21 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0

On 12.01.2017 13:19, Dr. David Alan Gilbert wrote:
> * Thomas Huth (address@hidden) wrote:
>> 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.
>>
>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
>>  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;
>> +    }
> 
> OK, that includes UNASSIGNED_CPU_INDEX.
> 
>> +
>> +    qmp_memsave(addr, size, filename, true, cpu_index, &err);
>>      hmp_handle_error(mon, &err);
>>  }
>>  
>> diff --git a/monitor.c b/monitor.c
>> index 0841d43..74843eb 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 : -1;
>>  }
> 
> OK, do you think that should use UNASSIGNED_CPU_INDEX
> explicitly rather than -1 ?

I wasn't aware of the fact that we've even got a macro for this ... I'll
send a v3 with that change.

> Reviewed-by: Dr. David Alan Gilbert <address@hidden>

Thanks for the review!

> I'm sure we'll find loads more similar cases where -M none breaks stuff.

I've added two more cases (migration and gdbstub) to the "Potentially
easy bugs" section on http://qemu-project.org/BiteSizedTasks now. I
think these are simple and easy tasks to get started with QEMU hacking...

 Thomas






reply via email to

[Prev in Thread] Current Thread [Next in Thread]