Re: [Qemu-trivial] [Qemu-devel] [PATCH] monitor: Fix crashes when using HMP commands without CPU

[Thomas Huth]

On 12.01.2017 09:10, Markus Armbruster wrote:
> Thomas Huth <address@hidden> writes:
> 
>> When running certain HMP commands ("info registers", "info cpustats"
>> 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,
> 
> "Sachen gibt's!"
> 
>>                               but these HMP commands did not check for
>> a valid CPU pointer yet. Add such a check now and print a message
>> about the missing CPU instead.
> 
> Have you checked uses of first_cpu elsewhere?  Out of scope for this
> patch, of course.

I only looked at monitor.c so far, and that's the only spot that uses
this variable there.

But it seems like gdbstub.c has the same bug, too. If I start the "none"
machine and attach a remote gdb, QEMU segfaults here, too.
I've put this on my TODO-list... (I think it should be fixed with a
separate patch).

>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
>>  monitor.c | 29 +++++++++++++++++++++++++----
>>  1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 0841d43..0103979 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);
> 
> Why are the following dereferences safe?
> 
>    CPUArchState *mon_get_cpu_env(void)
>    {
>        return mon_get_cpu()->env_ptr;
>    }
> 
>    int monitor_get_cpu_index(void)
>    {
>        return mon_get_cpu()->cpu_index;
>    }

Oh, they are apparently not safe either. The HMP commands "nmi" and
"memsave", which use these functions, are crashing on the "none"
machine, too... I'll send a v2 of my patch to fix these, too ...

Thanks for the review!

 Thomas