From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 37239 invoked by alias); 28 Apr 2016 17:04:35 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 37204 invoked by uid 89); 28 Apr 2016 17:04:34 -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=2015-2016, 5560, agent X-HELO: usplmg20.ericsson.net Received: from usplmg20.ericsson.net (HELO usplmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Thu, 28 Apr 2016 17:04:24 +0000 Received: from EUSAAHC006.ericsson.se (Unknown_Domain [147.117.188.90]) by usplmg20.ericsson.net (Symantec Mail Security) with SMTP id B7.00.09012.F4B32275; Thu, 28 Apr 2016 18:33:19 +0200 (CEST) Received: from [142.133.110.144] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.92) with Microsoft SMTP Server id 14.3.248.2; Thu, 28 Apr 2016 13:04:21 -0400 Subject: Re: [PATCH] ftrace: Fix gdbserver crash when doing tstatus after detach or process exit To: References: <1459344024-2260-1-git-send-email-simon.marchi@ericsson.com> From: Simon Marchi Message-ID: <57224295.6050504@ericsson.com> Date: Thu, 28 Apr 2016 17:04:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1459344024-2260-1-git-send-email-simon.marchi@ericsson.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-04/txt/msg00642.txt.bz2 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 . */ > + > +#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 > + } > + } > +} > Ping.