| Message ID | 20201223212752.1145294-1-keithp@keithp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gdbstub.c uses incorrect check for active gdb in use_gdb_syscalls | expand |
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 <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.
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;
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(-)