From patchwork Wed Mar 30 13:20:24 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 11560 Received: (qmail 28166 invoked by alias); 30 Mar 2016 13:20:45 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 28147 invoked by uid 89); 30 Mar 2016 13:20:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY autolearn=no version=3.3.2 spammy=7914, 79, 14, wherever, 3645 X-HELO: usplmg21.ericsson.net Received: from usplmg21.ericsson.net (HELO usplmg21.ericsson.net) (198.24.6.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Wed, 30 Mar 2016 13:20:34 +0000 Received: from EUSAAHC008.ericsson.se (Unknown_Domain [147.117.188.96]) by usplmg21.ericsson.net (Symantec Mail Security) with SMTP id E4.B0.22441.C72DBF65; Wed, 30 Mar 2016 15:19:56 +0200 (CEST) Received: from elxcz23q12-y4.dyn.mo.ca.am.ericsson.se (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.96) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 30 Mar 2016 09:20:31 -0400 From: Simon Marchi To: CC: Simon Marchi Subject: [PATCH] ftrace: Fix gdbserver crash when doing tstatus after detach or process exit Date: Wed, 30 Mar 2016 09:20:24 -0400 Message-ID: <1459344024-2260-1-git-send-email-simon.marchi@ericsson.com> MIME-Version: 1.0 X-IsSubscribed: yes 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 . */ + +#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 . + +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 + } + } +}