Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation

[Philippe Mathieu-Daudé]

Hi Stephane,

On 07/11/2018 04:52 AM, stephane duverger wrote:
> To reach gdb_set_stop_cpu() with gdbserver_state == NULL, you previously
>> entered gdb_vm_state_change() with and use CPUState *cpu =
>> gdbserver_state->c_cpu = NULL deref, which shouldn't happen.
>> Also in gdb_set_stop_cpu() you finally call cpu_single_step(cpu=crap)
>> which then deref crap->singlestep_enabled.
>>
>> So I think this is not the correct fix.
>>
> 
> I think you are wrong. You can enter gdb_vm_state_change() only if it has
> been registered through qemu_add_vm_change_state_handler(). And this
> happens in gdbserver_start() which is called only when you start the
> gdbserver stub.
> 
> This is exactly what we don't want to do here: use qemu breakpoints and
> debug events forwarding without the need to enable a gdb stub.

Well, at least we agree the gdb stub code is not straightforward.

And apparently without seeing the bigger picture about how you are using
this, I am missing something.

> There might be an historical reason that vCPU debug events are always
> forwarded to the gdbserver (from cpu_handle_guest_debug()) but this
> should not be mandatory.
> 
> One could consider a check to the gdbserver state right before:
> 
> if (gdbserver_enabled())
>     gdb_set_stop_cpu(cpu);
> 
> As found in other part of qemu code: kvm_enabled, hax_enabled, ...
> 
> 
>> Since this shouldn't happen, I'd add an assert(gdbserver_state) in
>> gdb_set_stop_cpu(), and clean gdb_vm_state_change()'s code flow to not
>> reach this.
>>
> 
> Correct if you previously add the gdbserver_enabled() check. Else this
> would abort the qemu instance each time a debug event is triggered
> and forwarded to a vm_change_state handler.
> 
>>>>  void gdb_set_stop_cpu(CPUState *cpu)
>>>>>  {
>>>>> +    if (!gdbserver_state) {
>>>>> +        return;
>>>>> +    }
>>>>>      gdbserver_state->c_cpu = cpu;
>>>>>      gdbserver_state->g_cpu = cpu;
>>
>> I find it safer the opposite way, if (s) { s-> ... }
>>
> 
> Sincerely, this argument is subjective. If it's part of Qemu coding
> standard,
> i would agree. Again there is a lot of situations in the Qemu code where
> exit conditions are checked first and then lead to a return, preventing an
> unneeded level of indentation. Seriously, we are talking about a 2 lines
> function.

This is indeed a personal subjective suggestion, not part of QEMU Coding
standard. Rational is, if in the future someone needs to modify
gdb_set_stop_cpu(), it would be easier/safer for him.

No worries ;)

Regards,

Phil.

signature.asc
Description: OpenPGP digital signature