On 07/10/2018 08:44 AM, stephane duverger wrote:
> Hi Alex,
>
> There don't seem to be any other patches attached? I would NACK a patch
>> that isn't actually used in-tree.
>
>
> No there isn't ! I should have not been so prolix. Actually the patch
> corrects a
> possible null pointer dereference in the gdbserver code. That's all folks.
>
> Below is how I discovered it and why it matters, imho, to be fixed
> (out of a pending issue).
>
> This null deref does not happen in normal operation because of the way
> gdbserver is initialized. However what I was telling you, is that it may be
> really interesting to be able to take benefit of some gdb features
> internally
> without starting a gdbserver and the overhead of the gdb protocol when
> there is no need for a client/server interaction while debugging.
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.
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.
> For instance, I developed a special purpose x86 board where I needed to
> break at some instructions, do some automation (snapshots, vm memory
> access, cpu analysis) and then resume the VM. Implementing a
> "vm_change_state handler" and dealing with RUN_STATE_DEBUG was
> perfect for my need.
I recommend you to use "configure --enable-sanitizers" to build your
special purpose board, that will show up those issues.
> The debug events (#BP, #DB) are tied to the gdbserver stub, at some point
> when "cpu_handle_guest_debug()" is called, a call to "gdb_set_stop_cpu()"
> triggers the NULL deref because the gdbserver is not initialized.
>
> My little fix addresses this error and allows to make use of the following
> Qemu
> breakpoint functions without touching "cpus.c" or "exec.c":
> - cpu_breakpoint_remove/insert()
> - kvm_remove/insert_breakpoint()
>
> Notice, that in the particular case of KVM, I had to async_run_on_cpu() my
> debug handler instead of directly executing it in the context of
> vm_change_state(). The vCPU started to significantly slow down
> (low irq rate), while this never happens with the TCG.
>
> Is that more clear to you Alex ? (and hopefully lead to patch ACK :)
>
> Regards,
>
> stephane
>
>> Signed-off-by: Stephane Duverger <stephane.duver...@gmail.com>
>>> ---
>>> gdbstub.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index d6ab95006c..788fd625ab 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -1412,6 +1412,9 @@ static int gdb_handle_packet(GDBState *s, const
>> char *line_buf)
>>>
>>> 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-> ... }
>>> }
Regards,
Phil.
