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

[Keith Packard via <qemu-devel@nongnu.org>]

[-- 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 --]