Re: [PATCH 1/2] gdbstub: Don't use GDB syscalls if no GDB is attached

[Luc Michel <luc@lmichel.fr>]

On 20:00 Thu 26 May     , Peter Maydell wrote:
> In two places in gdbstub.c we look at gdbserver_state.init to decide
> whether we're going to do a semihosting syscall via the gdb remote
> protocol:
>  * when setting up, if the user didn't explicitly select either
>    native semihosting or gdb semihosting, we autoselect, with the
>    intended behaviour "use gdb if gdb is connected"
>  * when the semihosting layer attempts to do a syscall via gdb, we
>    silently ignore it if the gdbstub wasn't actually set up
> 
> However, if the user's commandline sets up the gdbstub but tells QEMU
> to start rather than waiting for a GDB to connect (eg using '-s' but
> not '-S'), then we will have gdbserver_state.init true but no actual
> connection; an attempt to use gdb syscalls will then crash because we
> try to use gdbserver_state.c_cpu when it hasn't been set up:
> 
> #0  0x00007ffff6803ba8 in qemu_cpu_kick (cpu=0x0) at ../../softmmu/cpus.c:457
> #1  0x00007ffff6c03913 in gdb_do_syscallv (cb=0x7ffff6c19944 <common_semi_cb>,
>     fmt=0x7ffff7573b7e "", va=0x7ffff56294c0) at ../../gdbstub.c:2946
> #2  0x00007ffff6c19c3a in common_semi_gdb_syscall (cs=0x7ffff83fe060,
>     cb=0x7ffff6c19944 <common_semi_cb>, fmt=0x7ffff7573b75 "isatty,%x")
>     at ../../semihosting/arm-compat-semi.c:494
> #3  0x00007ffff6c1a064 in gdb_isattyfn (cs=0x7ffff83fe060, gf=0x7ffff86a3690)
>     at ../../semihosting/arm-compat-semi.c:636
> #4  0x00007ffff6c1b20f in do_common_semihosting (cs=0x7ffff83fe060)
>     at ../../semihosting/arm-compat-semi.c:967
> #5  0x00007ffff693a037 in handle_semihosting (cs=0x7ffff83fe060)
>     at ../../target/arm/helper.c:10316
> 
> You can probably also get into this state via some odd
> corner cases involving connecting a GDB and then telling it
> to detach from all the vCPUs.
> 
> Abstract out the test into a new gdb_attached() function
> which returns true only if there's actually a GDB connected
> to the debug stub and attached to at least one vCPU.
> 
> Reported-by: Liviu Ionescu <ilg@livius.net>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Luc Michel <luc@lmichel.fr>

> ---
> Silently doing nothing in gdb_do_syscallv(), never calling the
> callback function, is kind of dodgy.  But it's what the code is doing
> already, and besides it's not clear what we should do if the user
> specifically says "semihosting calls via the gdb stub" and then
> doesn't connect gdb...
> ---
>  gdbstub.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index a3ff8702cef..88a34c8f522 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -443,6 +443,15 @@ static int get_char(void)
>  }
>  #endif
>  
> +/*
> + * Return true if there is a GDB currently connected to the stub
> + * and attached to a CPU
> + */
> +static bool gdb_attached(void)
> +{
> +    return gdbserver_state.init && gdbserver_state.c_cpu;
> +}
> +
>  static enum {
>      GDB_SYS_UNKNOWN,
>      GDB_SYS_ENABLED,
> @@ -464,8 +473,7 @@ int use_gdb_syscalls(void)
>      /* -semihosting-config target=auto */
>      /* On the first call check if gdb is connected and remember. */
>      if (gdb_syscall_mode == GDB_SYS_UNKNOWN) {
> -        gdb_syscall_mode = gdbserver_state.init ?
> -            GDB_SYS_ENABLED : GDB_SYS_DISABLED;
> +        gdb_syscall_mode = gdb_attached() ? GDB_SYS_ENABLED : GDB_SYS_DISABLED;
>      }
>      return gdb_syscall_mode == GDB_SYS_ENABLED;
>  }
> @@ -2886,7 +2894,7 @@ void gdb_do_syscallv(gdb_syscall_complete_cb cb, const char *fmt, va_list va)
>      target_ulong addr;
>      uint64_t i64;
>  
> -    if (!gdbserver_state.init) {
> +    if (!gdb_attached()) {
>          return;
>      }
>  
> -- 
> 2.25.1
> 
> 

-- 
+---------------------+--------------------------+
| Luc MICHEL          | TIMA Lab / SLS Team      |
|                     | Phone: +33 4 76 57 43 34 |
+---------------------+--------------------------+