[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] gdbstub.c uses incorrect check for active gdb in use_gdb_sys
From: |
Keith Packard |
Subject: |
Re: [PATCH] gdbstub.c uses incorrect check for active gdb in use_gdb_syscalls |
Date: |
Tue, 12 Jan 2021 12:52:55 -0800 |
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
signature.asc
Description: PGP signature