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

[stephane duverger]

Hi Phil,

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.


Sorry for that, i will try to explain it clearly.
This is something rather simple indeed:

1 - during MachineState init, install a BP1 at an "OS_ready" location
2 - handle BP1 hit (thanks to vm_state_change())
3 - delete BP1 and save the VM
4 - inject some generated code
5 - install BP2 at injected code end
6 - resume the VM
7 - handle BP2 hit and restore the VM
8 - go to 4

So as you may guess, I didn't need a full gdbstub with a gdbclient
to do that. I just wanted to install breakpoints and handle them
internally. To be fast, no overhead with the gdb protocol.

I didn't find something ready to use in Qemu API, but if there exists
something to do it, I would be glad to know. By the time, I decided to
make use of the GDB breakpoint type. It was a perfect candidate,
but unfortunately any time cpu_handle_guest_debug() is called,
gdb_set_stop_cpu() is systematically called too.

That was my issue. As gdbserver is not up and running,
gdbserver_state is NULL and Qemu segfaults.

Out of this gdb centric function call, the standard Qemu vm stop
notification process seems perfect to me:
- qemu_system_debug_request
- do_vm_stop
- vm_state_notify
- and then my "vm_state_change_handler" is called

So that's why I thought it would be great to be able
to install breakpoints with either
cpu_breakpoint_insert() or kvm_insert_breakpoint().

What could have been done to be pragmatic:
1 - implement a new breakpoint type for internal purpose ?
2 - implement a gdbserver_enabled state ?
3 - check for the gdbserver_state NULL pointer case and return ?

The point 3 was the most simple, tied to only one gdb function,
didn't touch anything else and actually "secured" that
gdb_set_stop_cpu() 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.
>

I do understand that point. So what do we do right now :
1 - ACK the patch as is ?
2 - do you want a new patch with:
    - a gdbserver_enabled state
    - a safe execution condition in gdb_set_stop_cpu()
    - an abort() in it if something is wrong

Regards,

stephane