[Top][All Lists]

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

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

From: stephane duverger
Subject: Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation
Date: Wed, 11 Jul 2018 09:52:20 +0200

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())

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
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


reply via email to

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