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

[Simon Marchi <simon dot marchi at ericsson dot com>]

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
+        }
+    }
+}
-- 
2.7.4