This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] ftrace: Fix gdbserver crash when doing tstatus after detach or process exit
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: <gdb-patches at sourceware dot org>
- Date: Thu, 28 Apr 2016 13:04:21 -0400
- Subject: Re: [PATCH] ftrace: Fix gdbserver crash when doing tstatus after detach or process exit
- Authentication-results: sourceware.org; auth=none
- References: <1459344024-2260-1-git-send-email-simon dot marchi at ericsson dot com>
On 16-03-30 09:20 AM, Simon Marchi wrote:
> This patch fixes a gdbserver crash that is triggered by the following
> sequence of events:
>
> - A process with the in-process agent loaded is debugged under gdbserver.
> - The process is detached or exits.
> - Commands tstatus or enable/disable with fast tracepoints are used.
>
> Using either of tstatus or enable/disable ends up sending the qtstatus
> packet to gdbserver. During the handling of qtstatus, agent_loaded_p ()
> returns true, even though the process that once had the agent loaded is
> not present anymore. We end up trying to read memory with
> current_thread == NULL, causing a segfault here:
>
> gdb/gdbserver/linux-low.c:5560(linux_read_memory)[0x43583c]
> gdb/gdbserver/target.c:153(read_inferior_memory)[0x415d78]
> gdb/gdbserver/tracepoint.c:424(read_inferior_uinteger)[0x41c7fb]
> gdb/gdbserver/tracepoint.c:6288(upload_fast_traceframes)[0x425558]
> gdb/gdbserver/tracepoint.c:3645(cmd_qtstatus)[0x420dee]
> gdb/gdbserver/tracepoint.c:4239(handle_tracepoint_query)[0x4222ab]
> gdb/gdbserver/server.c:2543(handle_query)[0x411639]
> gdb/gdbserver/server.c:3910(process_serial_event)[0x413f39]
> gdb/gdbserver/server.c:4347(handle_serial_event)[0x415010]
> gdb/gdbserver/event-loop.c:428(handle_file_event)[0x41bed7]
> gdb/gdbserver/event-loop.c:184(process_event)[0x41b69e]
> gdb/gdbserver/event-loop.c:547(start_event_loop)[0x41c41d]
> gdb/gdbserver/server.c:3723(captured_main)[0x413a53]
> gdb/gdbserver/server.c:3802(main)[0x413c2f]
>
> A first solution that comes to mind is to make agent_loaded_p check if
> current_thread is NULL, and return false if that's the case. It would
> make sense, since if there is no current thread, the agent can't
> possibly be loaded. However, that would require adding some
> #ifdef GDBSERVER to the common code, which is not acceptable.
>
> An alternative would be to use
>
> current_thread != NULL && agent_loaded_p ()
>
> wherever agent_loaded_p () is used. However, I find it error prone
> for future uses of agent_loaded_p (), since it would be easy to forget
> to check for current_thread.
>
> Instead, the solution I chose is to clear the
> all_agent_symbols_looked_up flag whenever we have no more current thread
> (process exit or detach). I am not 100% sure it's correct, as there
> might be valid situations I don't know about where the agent is loaded
> but current_thread == NULL, so please correct me if I'm wrong.
>
> I added a test that either detaches from the process or makes it exit,
> and then tries to use the commands that would cause the crash. I run
> the test both in all-stop and non-stop, since it was required to fix
> both code paths in gdbserver.
>
> Initially, I wanted to enhance gdb.trace/no-attach-trace.exp instead of
> adding a new test case, since it tests a similar problem (gdbserver
> would crash when doing tstart with no process attached). However, my
> case is specific to fast tracepoints and the in-process agent, whereas
> no-attach-trace.exp also runs on targets that use normal tracing. So I
> left them as two separate test cases.
>
> No regression with native-gdbserver && native-extended-gdbserver.
>
> Finally, as a side-note, and just to make sure I understand correctly:
> since there is a single global all_agent_symbols_looked_up flag, I guess
> the tracking of whether the agent is loaded is not expected to work
> correctly in a multi-process scenario, is that right? If there are two
> processes under gdbserver, there could be one with and one without the
> agent. So ideally (as it would be more "right" than the current patch),
> I suppose we should track this per-process?
>
> gdb/ChangeLog:
>
> * common/agent.h (agent_clear): New declaration.
> * common/agent.c (agent_clear): New function.
>
> gdb/gdbserver/ChangeLog:
>
> * server.c (resume): Call agent_clear on inferior process exit.
> (process_serial_event): Call agent_clear on inferior process
> detach.
> (handle_target_event): Call agent_clear on inferior process
> exit.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.trace/ftrace-commands-after-detach-or-exit.exp: New file.
> * gdb.trace/ftrace-commands-after-detach-or-exit.c: New file.
> ---
> gdb/common/agent.c | 8 ++
> gdb/common/agent.h | 5 +
> gdb/gdbserver/server.c | 9 +-
> .../ftrace-commands-after-detach-or-exit.c | 25 ++++
> .../ftrace-commands-after-detach-or-exit.exp | 154 +++++++++++++++++++++
> 5 files changed, 200 insertions(+), 1 deletion(-)
> create mode 100644 gdb/testsuite/gdb.trace/ftrace-commands-after-detach-or-exit.c
> create mode 100644 gdb/testsuite/gdb.trace/ftrace-commands-after-detach-or-exit.exp
>
> diff --git a/gdb/common/agent.c b/gdb/common/agent.c
> index 9faf8e7..b1fc9c0 100644
> --- a/gdb/common/agent.c
> +++ b/gdb/common/agent.c
> @@ -79,6 +79,14 @@ agent_loaded_p (void)
> return all_agent_symbols_looked_up;
> }
>
> +/* See agent.h. */
> +
> +void
> +agent_clear (void)
> +{
> + all_agent_symbols_looked_up = 0;
> +}
> +
> /* Look up all symbols needed by agent. Return 0 if all the symbols are
> found, return non-zero otherwise. */
>
> diff --git a/gdb/common/agent.h b/gdb/common/agent.h
> index 7fb1b0f..2e42308 100644
> --- a/gdb/common/agent.h
> +++ b/gdb/common/agent.h
> @@ -36,6 +36,11 @@ int agent_look_up_symbols (void *);
>
> int agent_loaded_p (void);
>
> +/* Reset the internal data about the agent, when the debugged process
> + disappears (e.g. exits or is detached). */
> +
> +void agent_clear (void);
> +
> extern int debug_agent;
>
> extern int use_agent;
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index ef715e7..134693f 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -2781,7 +2781,11 @@ resume (struct thread_resume *actions, size_t num_actions)
>
> if (last_status.kind == TARGET_WAITKIND_EXITED
> || last_status.kind == TARGET_WAITKIND_SIGNALLED)
> - mourn_inferior (find_process_pid (ptid_get_pid (last_ptid)));
> + {
> + agent_clear ();
> + mourn_inferior (find_process_pid (ptid_get_pid (last_ptid)));
> + }
> +
> }
> }
>
> @@ -4000,6 +4004,8 @@ process_serial_event (void)
> join_inferior (pid);
> exit (0);
> }
> +
> + agent_clear ();
> }
> break;
> case '!':
> @@ -4392,6 +4398,7 @@ handle_target_event (int err, gdb_client_data client_data)
> if (last_status.kind == TARGET_WAITKIND_EXITED
> || last_status.kind == TARGET_WAITKIND_SIGNALLED)
> {
> + agent_clear ();
> mark_breakpoints_out (process);
> mourn_inferior (process);
> }
> diff --git a/gdb/testsuite/gdb.trace/ftrace-commands-after-detach-or-exit.c b/gdb/testsuite/gdb.trace/ftrace-commands-after-detach-or-exit.c
> new file mode 100644
> index 0000000..9f93b9b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/ftrace-commands-after-detach-or-exit.c
> @@ -0,0 +1,25 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2016 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include "trace-common.h"
> +
> +int
> +main (void)
> +{
> + FAST_TRACEPOINT_LABEL(set_point);
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.trace/ftrace-commands-after-detach-or-exit.exp b/gdb/testsuite/gdb.trace/ftrace-commands-after-detach-or-exit.exp
> new file mode 100644
> index 0000000..1e0a7e8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/ftrace-commands-after-detach-or-exit.exp
> @@ -0,0 +1,154 @@
> +# Copyright 2015-2016 Free Software Foundation, Inc.
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +load_lib "trace-support.exp"
> +
> +standard_testfile
> +set executable $testfile
> +set expfile $testfile.exp
> +
> +# Some targets have leading underscores on assembly symbols.
> +set options [list debug [gdb_target_symbol_prefix_flags]]
> +
> +# Check that the target supports trace.
> +if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
> + untested "Couldn't compile test program"
> + return -1
> +}
> +
> +clean_restart ${testfile}
> +
> +if ![runto_main] {
> + fail "Can't run to main to check for trace support"
> + return -1
> +}
> +
> +if $use_gdb_stub {
> + # This test is about testing commands after detaching from a process or
> + # after letting a process exit, so it doesn't make sense to run it if the
> + # target is stub-like.
> + unsupported "This test is not supported for GDB stub targets."
> + return -1
> +}
> +
> +if ![gdb_target_supports_trace] {
> + unsupported "target does not support trace"
> + return -1
> +}
> +
> +# Compile the test case with the in-process agent library.
> +set libipa [get_in_proc_agent]
> +gdb_load_shlibs $libipa
> +
> +lappend options shlib=$libipa
> +
> +if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
> + untested "Couldn't compile test program with in-process agent library"
> + return -1
> +}
> +
> +# This test makes sure that gdbserver doesn't crash when doing a tstatus
> +# after detaching from a previously traced process.
> +proc test_tstatus_after_detach { } {
> + with_test_prefix "tstatus after detach" {
> + global executable binfile decimal
> + clean_restart ${executable}
> +
> + if ![runto_main] {
> + fail "Can't run to main."
> + return -1
> + }
> +
> + gdb_test "ftrace set_point" "Fast tracepoint .*"
> + gdb_test_no_output "tstart"
> + gdb_test_no_output "tstop"
> + gdb_test "detach" "Detaching from program: $binfile, process $decimal"
> + gdb_test "tstatus" "Trace stopped by a tstop command.*"
> + }
> +}
> +
> +# This test makes sure that gdbserver doesn't crash when doing a tstatus
> +# after a previously traced process has exited.
> +proc test_tstatus_after_exit { } {
> + with_test_prefix "tstatus after exit" {
> + global executable
> + clean_restart ${executable}
> +
> + if ![runto_main] {
> + fail "Can't run to main."
> + return -1
> + }
> +
> + gdb_test "ftrace set_point" "Fast tracepoint .*"
> + gdb_test_no_output "tstart"
> + gdb_test_no_output "tstop"
> + gdb_continue_to_end
> + gdb_test "tstatus" "Trace stopped by a tstop command.*"
> + }
> +}
> +
> +# This test makes sure that gdbserver doesn't crash when doing a enable
> +# or disable after detaching from a previously traced process.
> +proc test_enabledisable_after_detach { } {
> + with_test_prefix "enable/disable after detach" {
> + global executable binfile decimal
> + clean_restart ${executable}
> +
> + if ![runto_main] {
> + fail "Can't run to main."
> + return -1
> + }
> +
> + gdb_test "ftrace set_point" "Fast tracepoint .*"
> + gdb_test_no_output "tstart"
> + gdb_test_no_output "tstop"
> + gdb_test "detach" "Detaching from program: $binfile, process $decimal"
> + gdb_test_no_output "disable"
> + gdb_test_no_output "enable"
> + }
> +}
> +
> +# This test makes sure that gdbserver doesn't crash when doing a enable
> +# or disable after a previously traced process has exited.
> +proc test_enabledisable_after_exit { } {
> + with_test_prefix "enable/disable after exit" {
> + global executable
> + clean_restart ${executable}
> +
> + if ![runto_main] {
> + fail "Can't run to main."
> + return -1
> + }
> +
> + gdb_test "ftrace set_point" "Fast tracepoint .*"
> + gdb_test_no_output "tstart"
> + gdb_test_no_output "tstop"
> + gdb_continue_to_end
> + gdb_test_no_output "disable"
> + gdb_test_no_output "enable"
> + }
> +}
> +
> +foreach nonstop { "off" "on" } {
> + save_vars { GDBFLAGS } {
> + append GDBFLAGS " -ex \"set non-stop $nonstop\""
> +
> + with_test_prefix "non-stop=$nonstop" {
> + test_tstatus_after_detach
> + test_tstatus_after_exit
> + test_enabledisable_after_detach
> + test_enabledisable_after_exit
> + }
> + }
> +}
>
Ping.