Re: [PATCH] ftrace: Fix gdbserver crash when doing tstatus after detach or process exit

[Simon Marchi <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.