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

[stephane duverger]

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.

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.

stephane