From patchwork Thu Jun 2 17:31:23 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Antoine Tremblay X-Patchwork-Id: 12724 Received: (qmail 92921 invoked by alias); 2 Jun 2016 17:31:46 -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 92909 invoked by uid 89); 2 Jun 2016 17:31: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=sk:unpack_, 3723, pause, Stopping 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; Thu, 02 Jun 2016 17:31:35 +0000 Received: from EUSAAHC002.ericsson.se (Unknown_Domain [147.117.188.78]) by usplmg21.ericsson.net (Symantec Mail Security) with SMTP id 06.C8.03614.E3D60575; Thu, 2 Jun 2016 19:30:38 +0200 (CEST) Received: from elxa4wqvvz1.dyn.mo.ca.am.ericsson.se (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.78) with Microsoft SMTP Server (TLS) id 14.3.294.0; Thu, 2 Jun 2016 13:31:32 -0400 From: Antoine Tremblay To: CC: Antoine Tremblay Subject: [PATCH v2] Fix gdbserver crash when doing ftrace commands after detach or process exit Date: Thu, 2 Jun 2016 13:31:23 -0400 Message-ID: <1464888683-31530-1-git-send-email-antoine.tremblay@ericsson.com> MIME-Version: 1.0 X-IsSubscribed: yes This patch is a v2 of: https://sourceware.org/ml/gdb-patches/2016-03/msg00560.html For the test to be stable you need to apply this patch over: https://sourceware.org/ml/gdb-patches/2016-06/msg00040.html I took the approach Yao suggested, protecting tracing commands from current_thread == NULL where needed. Regarding Pedro's comment here: https://sourceware.org/ml/gdb-patches/2016-04/msg00652.html I interpreted it as that GDBserver should bail out if current_thread == NULL, and that choosing another thread was not an option, that could be wrong, comments are welcome. --- 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 with or without stopping the trace before. - Commands tstatus, enable/disable, ftrace, tstop, quit 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] ftrace, tstop and quit need to be protected from current_thread == NULL in a similar manner. This patch adds a test called gdb.trace/ftrace-commands-after-detach-or-exit.exp. The test tests both in all-stop and non-stop, it also makes sure that a gdb quit call works and that gdbserver is still alive after a test by reconnecting to it after a quit. No regression on x86-linux { native-gdbserver , native-extended-gdbserver } gdb/gdbserver/ChangeLog: * tracepoint.c (cmd_qtdp): Check for current_thread == NULL. (cmd_qtenable_disable): Likewise. (cmd_qtstart): Likewise. (stop_tracing): Likewise. (cmd_qtstop): Likewise. (cmd_qtstatus): Likewise. gdb/testsuite/ChangeLog: * gdb.trace/ftrace-commands-after-detach-or-exit.c: New file. * gdb.trace/ftrace-commands-after-detach-or-exit.exp: New test. * lib/gdbserver-support.exp (gdb_target_cmd): Add support for connection timed out error. --- gdb/gdbserver/tracepoint.c | 36 +++- .../ftrace-commands-after-detach-or-exit.c | 25 +++ .../ftrace-commands-after-detach-or-exit.exp | 231 +++++++++++++++++++++ gdb/testsuite/lib/gdbserver-support.exp | 4 + 4 files changed, 295 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/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c index c07e525..d80e0c98 100644 --- a/gdb/gdbserver/tracepoint.c +++ b/gdb/gdbserver/tracepoint.c @@ -2488,6 +2488,13 @@ cmd_qtdp (char *own_buf) char *actparm; char *packet = own_buf; + /* Can't do this command without a pid attached. */ + if (current_thread == NULL) + { + write_enn (packet); + return; + } + packet += strlen ("QTDP:"); /* A hyphen at the beginning marks a packet specifying actions for a @@ -2752,6 +2759,13 @@ cmd_qtenable_disable (char *own_buf, int enable) ULONGEST num, addr; struct tracepoint *tp; + /* Can't do this command without a pid attached. */ + if (current_thread == NULL) + { + write_enn (packet); + return; + } + packet += strlen (enable ? "QTEnable:" : "QTDisable:"); packet = unpack_varlen_hex (packet, &num); ++packet; /* skip a colon */ @@ -3202,6 +3216,13 @@ cmd_qtstart (char *packet) struct tracepoint *tpoint, *prev_ftpoint, *prev_stpoint; CORE_ADDR tpptr = 0, prev_tpptr = 0; + /* Can't do this command without a pid attached. */ + if (current_thread == NULL) + { + write_enn (packet); + return; + } + trace_debug ("Starting the trace"); /* Pause all threads temporarily while we patch tracepoints. */ @@ -3420,6 +3441,12 @@ stop_tracing (void) return; } + if (current_thread == NULL) + { + trace_debug ("Current thread null, can't stop threads"); + return; + } + trace_debug ("Stopping the trace"); /* Pause all threads before removing fast jumps from memory, @@ -3532,6 +3559,13 @@ flush_trace_buffer_handler (CORE_ADDR addr) static void cmd_qtstop (char *packet) { + /* Can't do this command without a pid attached. */ + if (current_thread == NULL) + { + write_enn (packet); + return; + } + stop_tracing (); write_ok (packet); } @@ -3650,7 +3684,7 @@ cmd_qtstatus (char *packet) trace_debug ("Returning trace status as %d, stop reason %s", tracing, tracing_stop_reason); - if (agent_loaded_p ()) + if (current_thread != NULL && agent_loaded_p ()) { pause_all (1); 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..b68f9db --- /dev/null +++ b/gdb/testsuite/gdb.trace/ftrace-commands-after-detach-or-exit.exp @@ -0,0 +1,231 @@ +# 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 . + +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 "$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] +set remote_libipa [gdb_load_shlib $libipa] + +lappend options shlib=$libipa + +if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } { + untested "Couldn't compile test program with in-process agent library" + return -1 +} + +clean_restart ${testfile} + +if ![runto_main] { + fail "Can't run to main to check for trace support" + return -1 +} + +if { [gdb_test "info sharedlibrary" ".*${remote_libipa}.*" "IPA loaded"] != 0 } { + untested "Could not find IPA lib loaded" + 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" { + gdbserver_startup + global binfile decimal + 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 ()\..*" + test_gdbserver_still_alive + } +} + +# 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" { + gdbserver_startup + 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 ()\..*" + test_gdbserver_still_alive + } +} + +# 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" { + gdbserver_startup + global binfile decimal + 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" + test_gdbserver_still_alive + } +} + +# 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" { + gdbserver_startup + 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" + test_gdbserver_still_alive + } +} + +# This test makes sure that gdbserver doesn't crash when doing an ftrace +# after detaching from a previously traced process. +proc test_ftrace_after_detach { } { + with_test_prefix "ftrace after detach" { + gdbserver_startup + global binfile decimal + gdb_test "ftrace set_point" "Fast tracepoint .*" + gdb_test_no_output "tstart" + gdb_test_no_output "set confirm off" + gdb_test "detach" "Detaching from program: $binfile, process $decimal" + gdb_test "ftrace set_point" ".*Fast tracepoint \[0-9]+ at.*" + test_gdbserver_still_alive + } +} + +# This test makes sure that gdbserver doesn't crash when doing an ftrace +# or disable after a previously traced process has exited. +proc test_ftrace_after_exit { } { + with_test_prefix "ftrace after exit" { + gdbserver_startup + gdb_test "ftrace set_point" "Fast tracepoint .*" + gdb_test_no_output "tstart" + gdb_continue_to_end + gdb_test "ftrace set_point" ".*Target returns error code \'01\'\..*" + test_gdbserver_still_alive + } +} + +# This test makes sure that gdbserver doesn't crash when doing an tstop +# after detaching from a previously traced process. +proc test_tstop_after_detach { } { + with_test_prefix "tstop after detach" { + gdbserver_startup + global binfile decimal + gdb_test "ftrace set_point" "Fast tracepoint .*" + gdb_test_no_output "tstart" + gdb_test_no_output "set confirm off" + gdb_test "detach" "Detaching from program: $binfile, process $decimal" + gdb_test "tstop" "Target returns error code \'01\'\..*" + test_gdbserver_still_alive + } +} + +# This test makes sure that gdbserver doesn't crash when doing a tstop +# after a previously traced process has exited. +proc test_tstop_after_exit { } { + with_test_prefix "tstop after exit" { + gdbserver_startup + gdb_test "ftrace set_point" "Fast tracepoint .*" + gdb_test_no_output "tstart" + gdb_continue_to_end + gdb_test "tstop" "Target returns error code \'01\'\..*" + test_gdbserver_still_alive + } +} + +# Initialize gdbserver in reconnectable mode to be able to test if it's still +# alive after a full test (including gdb exit procedure) +proc gdbserver_startup { } { + global executable gdbserver_reconnect_p + set gdbserver_reconnect_p 1 + clean_restart ${executable} + + if ![runto_main] { + fail "Can't run to main." + return -1 + } +} + +# Test if gdbserver is still alive by reconnecting to it and asking for a +# trace status. +proc test_gdbserver_still_alive { } { + gdb_test_no_output "set confirm off" + gdb_test "disconnect" "Ending remote debugging\\." + set test "reconnect to GDBserver" + if { [gdb_reconnect] == 0 } { + pass $test + } else { + fail $test + return 0 + } +} + +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 + test_ftrace_after_detach + test_ftrace_after_exit + test_tstop_after_detach + test_tstop_after_exit + } + } +} diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp index 951afe5..1017889 100644 --- a/gdb/testsuite/lib/gdbserver-support.exp +++ b/gdb/testsuite/lib/gdbserver-support.exp @@ -90,6 +90,10 @@ proc gdb_target_cmd { targetname serialport } { -re "Timeout reading from remote system.*$gdb_prompt $" { verbose "Got timeout error from gdb." } + -re ".*Connection timed out.*$gdb_prompt $" { + verbose "Got timeout error from gdb." + } + -notransfer -re "Remote debugging using .*\r\n> $" { # We got an unexpected prompt while creating the target. # Leave it there for the test to diagnose.