[PATCH] gdbstub.c uses incorrect check for active gdb in use_gdb_syscalls

[PATCH] gdbstub.c uses incorrect check for active gdb in use_gdb_syscalls

[Keith Packard via]

When checking whether there is a live gdb connection, code shouldn't
use 'gdbserver_state.init' as that value is set when the
gdbserver_state structure is initialized in init_gdbserver_state, not
when the gdb socket has a valid connection.

The 'handle_detach' function appears to use 'gdbserver_state.c_cpu' as
an indication of whether there is a connection, so I've used the same
in use_gdb_syscalls.

This avoids a segfault when qemu is run with the '-s' flag (create a
gdb protocol socket), but without the '-S' flag (delay until 'c'
command is received).

I would like this patch to inform a discussion on whether the numerous
other places using gdbserver_state.init are also incorrect (most of
them appear to be using it in the same way use_gdb_syscalls does), and
also whether use_gdb_syscalls should cache the result of this check or
whether it should check each time it is called to see if a gdb
connection is currently acive. For the second question, I don't have a
clear idea; mixing gdb and native calls seems problematic for stateful
operations like file open/close.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 gdbstub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbstub.c b/gdbstub.c
index d99bc0bf2e..4e709d16fd 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -460,7 +460,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_syscall_mode = gdbserver_state.c_cpu != NULL ?
             GDB_SYS_ENABLED : GDB_SYS_DISABLED;
     }
     return gdb_syscall_mode == GDB_SYS_ENABLED;
-- 
2.29.2

Re: [PATCH] gdbstub.c uses incorrect check for active gdb in use_gdb_syscalls

[Alex Bennée]

Keith Packard <keithp@keithp.com> writes:

> When checking whether there is a live gdb connection, code shouldn't
> use 'gdbserver_state.init' as that value is set when the
> gdbserver_state structure is initialized in init_gdbserver_state, not
> when the gdb socket has a valid connection.
>
> The 'handle_detach' function appears to use 'gdbserver_state.c_cpu' as
> an indication of whether there is a connection, so I've used the same
> in use_gdb_syscalls.

I guess it could be anything that is set by gdb_accept_init(). I'm a
little wary of c_cpu given it has a specific meaning of current cpu and
does move around depending on actions of the debugger.

It would be better to wrap the test in a function (static bool
is_connected()?) so the semantic meaning is clear in the code and we can
fix things in one place if needed.

> This avoids a segfault when qemu is run with the '-s' flag (create a
> gdb protocol socket), but without the '-S' flag (delay until 'c'
> command is received).

How exactly did you create the segfault? Just starting with -s and
attaching to a running tasks works fine for me although I Can see
semihosting stuff would never get to gdb after connection.

> I would like this patch to inform a discussion on whether the numerous
> other places using gdbserver_state.init are also incorrect (most of
> them appear to be using it in the same way use_gdb_syscalls does), and
> also whether use_gdb_syscalls should cache the result of this check or
> whether it should check each time it is called to see if a gdb
> connection is currently acive.

Hmm I don't see anything obviously wrong - although I note a bunch of
tests also check for ->fd which is probably a clearer indication of an
active connection. I'm sure this could be improved with a semantically
clearer code though.

> For the second question, I don't have a
> clear idea; mixing gdb and native calls seems problematic for stateful
> operations like file open/close.

Yes it's a bit of a hack. I can imagine starting with a remote GDB
connection and then loosing it after opening a file descriptor would
result in Bad Things (tm). I'm not sure what the cleanest approach is to
handling the resulting mess.

>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  gdbstub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index d99bc0bf2e..4e709d16fd 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -460,7 +460,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_syscall_mode = gdbserver_state.c_cpu != NULL ?
>              GDB_SYS_ENABLED : GDB_SYS_DISABLED;
>      }
>      return gdb_syscall_mode == GDB_SYS_ENABLED;


-- 
Alex Bennée

Re: [PATCH] gdbstub.c uses incorrect check for active gdb in use_gdb_syscalls

[Keith Packard via]

[-- Attachment #1: Type: text/plain, Size: 2663 bytes --]

Alex Bennée <alex.bennee@linaro.org> writes:

> It would be better to wrap the test in a function (static bool
> is_connected()?) so the semantic meaning is clear in the code and we can
> fix things in one place if needed.

That makes good sense to me.

> How exactly did you create the segfault? Just starting with -s and
> attaching to a running tasks works fine for me although I Can see
> semihosting stuff would never get to gdb after connection.

Making a semihosting call before GDB is connected results in
dereferencing a NULL gdbserver_state.c_cpu pointer below
gdb_do_syscallv. The sequence goes like this:

 1. gdbserver_start is called during qemu startup, which calls
    init_gdbserver_state which sets gdbserver_state.init = true

 2. application makes semihosting call (like putc)

 3. semihosting code calls use_gdb_syscalls(), which returns true
    because gdbserver_state.init is true

 4. gdb_do_syscallv checks gdbserver_state.init, which is true

 5. gdb_do_syscallv uses gdbserver_state.c_cpu, which is still NULL and
    causes a segfault in qemu_cpu_kick

> Hmm I don't see anything obviously wrong - although I note a bunch of
> tests also check for ->fd which is probably a clearer indication of an
> active connection. I'm sure this could be improved with a semantically
> clearer code though.

fd is < 0 only *after* a connection has failed, it is not set to -1 before
a connection has started. I agree that using 'fd' is a good idea instead
of c_cpu, but it would need to be combined with checking 'init' and
initializing fd to -1 when init is set to true.

In any case, hiding all of this behind a couple of functions seems like
a good idea. For now, I'll continue to use c_cpu as that is independent
of CONFIG_USER_ONLY *and* has the advantage of being initialized to NULL
by default. It's marked with XXX in the patch as it seems like a bit of
a kludge.

> Yes it's a bit of a hack. I can imagine starting with a remote GDB
> connection and then loosing it after opening a file descriptor would
> result in Bad Things (tm). I'm not sure what the cleanest approach is to
> handling the resulting mess.

Hrm. use_gdb_syscalls caches the results of the first test, so we won't
ever mix things, we'll just get some semihosting calls dropped when the
gdb server is not connected. If use_gdb_syscalls checks for a valid
connection, then gdb will never get semihosting calls if -S is not on
the command line. If use_gdb_syscalls checks for gdbserver_state.init,
then gdb will get semihosting calls whenever it is connected, otherwise
those calls will be dropped.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]