[PATCH] Fix GDB crash after Quit thrown from unwinder sniffer (Re: [RFA] Remove cleanup from frame_prepare_for_sniffer)

[Pedro Alves <palves at redhat dot com>]

Hi Tom,

On 10/08/2017 11:52 PM, Tom Tromey wrote:
> Currently frame_prepare_for_sniffer returns a cleanup.  This patch
> changes it to return void, and exposes frame_cleanup_after_sniffer to
> the caller.
> 
> Normally I would write an RAII class for this sort of thing; but
> because there was just a single caller of frame_prepare_for_sniffer,
> and because this caller is already using try/catch, I thought it
> seemed ok to require explicit calls in this instance.
> 
> Regression tested by the buildbot.
> 
> gdb/ChangeLog
> 2017-10-08  Tom Tromey  <tom@tromey.com>
> 
> 	* frame-unwind.c (frame_unwind_try_unwinder): Update.
> 	* frame.h (frame_cleanup_after_sniffer): Declare.
> 	(frame_prepare_for_sniffer): Return void.
> 	* frame.c (frame_cleanup_after_sniffer): No longer static.  Change
> 	type of argument.
> 	(frame_prepare_for_sniffer): Return void.

I think this caused a regression.  See fix below.

WDYT?

>From 8db37aa3966407c9190820a75e23a75688af9f95 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 14 Feb 2018 15:11:58 +0000
Subject: [PATCH] Fix GDB crash after Quit thrown from unwinder sniffer

I ran into a GDB crash in gdb.base/bp-cmds-continue-ctrl-c.exp in my
multi-target branch, which turns out exposed a bug that exists in
master too.

That testcase has a breakpoint with a "continue" command associated.
Then the breakpoint is constantly being hit.  At the same time, the
testcase is continualy interrupting the program with Ctrl-C, and
re-resuming it, in a loop.

Running that testcase manually under Valgrind, after a few sequences
of 'Ctrl-C' + 'continue', I got:

 Breakpoint 1, Quit
 (gdb) ==21270== Invalid read of size 8
 ==21270==    at 0x4D8185: pyuw_this_id(frame_info*, void**, frame_id*) (py-unwind.c:461)
 ==21270==    by 0x6D426A: compute_frame_id(frame_info*) (frame.c:505)
 ==21270==    by 0x6D43B7: get_frame_id(frame_info*) (frame.c:537)
 ==21270==    by 0x84F3B8: scoped_restore_current_thread::scoped_restore_current_thread() (thread.c:1678)
 ==21270==    by 0x718E3D: fetch_inferior_event(void*) (infrun.c:4076)
 ==21270==    by 0x7067C9: inferior_event_handler(inferior_event_type, void*) (inf-loop.c:43)
 ==21270==    by 0x45BEF9: handle_target_event(int, void*) (linux-nat.c:4419)
 ==21270==    by 0x6C4255: handle_file_event(file_handler*, int) (event-loop.c:733)
 ==21270==    by 0x6C47F8: gdb_wait_for_event(int) (event-loop.c:859)
 ==21270==    by 0x6C3666: gdb_do_one_event() (event-loop.c:322)
 ==21270==    by 0x6C3712: start_event_loop() (event-loop.c:371)
 ==21270==    by 0x746801: captured_command_loop() (main.c:329)
 ==21270==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
 ==21270==
 ==21270==
 ==21270== Process terminating with default action of signal 11 (SIGSEGV): dumping core
 ==21270==  Access not within mapped region at address 0x0
 ==21270==    at 0x4D8185: pyuw_this_id(frame_info*, void**, frame_id*) (py-unwind.c:461)
 ==21270==    by 0x6D426A: compute_frame_id(frame_info*) (frame.c:505)
 ==21270==    by 0x6D43B7: get_frame_id(frame_info*) (frame.c:537)
 ==21270==    by 0x84F3B8: scoped_restore_current_thread::scoped_restore_current_thread() (thread.c:1678)
 ==21270==    by 0x718E3D: fetch_inferior_event(void*) (infrun.c:4076)
 ==21270==    by 0x7067C9: inferior_event_handler(inferior_event_type, void*) (inf-loop.c:43)
 ==21270==    by 0x45BEF9: handle_target_event(int, void*) (linux-nat.c:4419)
 ==21270==    by 0x6C4255: handle_file_event(file_handler*, int) (event-loop.c:733)
 ==21270==    by 0x6C47F8: gdb_wait_for_event(int) (event-loop.c:859)
 ==21270==    by 0x6C3666: gdb_do_one_event() (event-loop.c:322)
 ==21270==    by 0x6C3712: start_event_loop() (event-loop.c:371)
 ==21270==    by 0x746801: captured_command_loop() (main.c:329)
 ==21270==  If you believe this happened as a result of a stack
 ==21270==  overflow in your program's main thread (unlikely but
 ==21270==  possible), you can try to increase the size of the
 ==21270==  main thread stack using the --main-stacksize= flag.
 ==21270==  The main thread stack size used in this run was 8388608.
 ==21270==

Above, when we get to compute_frame_id, fi->unwind is non-NULL,
meaning, we found an unwinder, in this case the Python unwinder, but
somehow, fi->prologue_cache is left NULL.  pyuw_this_id then crashes
because it assumes fi->prologue_cache is non-NULL:

  static void
  pyuw_this_id (struct frame_info *this_frame, void **cache_ptr,
		struct frame_id *this_id)
  {
    *this_id = ((cached_frame_info *) *cache_ptr)->frame_id;
                                      ^^^^^^^^^^

'*cache_ptr' here is 'fi->prologue_cache'.

There's a quit() call in pyuw_sniffer that I believe is the one that
sometimes triggers the crash above.  The crash can be reproduced
easily with this hack to force a quit out of the python unwinder:

 --- a/gdb/python/py-unwind.c
 +++ b/gdb/python/py-unwind.c
 @@ -497,6 +497,8 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
    struct gdbarch *gdbarch = (struct gdbarch *) (self->unwind_data);
    cached_frame_info *cached_frame;

 +  quit ();
 +
    gdbpy_enter enter_py (gdbarch, current_language);

    TRACE_PY_UNWIND (3, "%s (SP=%s, PC=%s)\n", __FUNCTION__,

After that quit is thrown, any subsequent operation that involves
unwinding results in GDB crashing with SIGSEGV like above.

The problem is that this commit:

  commit 30a9c02feff56bd58a276c2a7262f364baa558ac
  CommitDate: Sun Oct 8 23:16:42 2017 -0600
  Subject: Remove cleanup from frame_prepare_for_sniffer

missed that we need to call frame_cleanup_after_sniffer before
rethrowing the exception too.

Without the fix, the "bt" added to
gdb.base/bp-cmds-continue-ctrl-c.exp in this commit makes GDB crash:

  Running src/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp ...
  ERROR: Process no longer exists

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* frame-unwind.c (frame_unwind_try_unwinder): Always call
	frame_cleanup_after_sniffer on exception.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdb.base/bp-cmds-continue-ctrl-c.exp (do_test): Test "bt" after
	getting a "Quit".
---
 gdb/frame-unwind.c                                 |  3 ++-
 gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp | 13 +++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index 66a28ae1c9..e6e63539ad 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -110,13 +110,14 @@ frame_unwind_try_unwinder (struct frame_info *this_frame, void **this_cache,
       /* Catch all exceptions, caused by either interrupt or error.
 	 Reset *THIS_CACHE.  */
       *this_cache = NULL;
+      frame_cleanup_after_sniffer (this_frame);
+
       if (ex.error == NOT_AVAILABLE_ERROR)
 	{
 	  /* This usually means that not even the PC is available,
 	     thus most unwinders aren't able to determine if they're
 	     the best fit.  Keep trying.  Fallback prologue unwinders
 	     should always accept the frame.  */
-	  frame_cleanup_after_sniffer (this_frame);
 	  return 0;
 	}
       throw_exception (ex);
diff --git a/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp b/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp
index 8c2fa77d71..43600fcf81 100644
--- a/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp
+++ b/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp
@@ -89,6 +89,19 @@ proc do_test {} {
 	    }
 	    -re "Quit\r\n$gdb_prompt $" {
 		send_log "$internal_pass (Quit)\n"
+
+		# Check that if we managed to quit somewhere deep in
+		# the unwinders, we can still unwind again.
+		set ok 0
+		gdb_test_multiple "bt" "$internal_pass (bt)" {
+		    -re "#0.*$gdb_prompt $" {
+			send_log "$internal_pass (bt)\n"
+			set ok 1
+		    }
+		}
+		if {!$ok} {
+		    return
+		}
 	    }
 	    -re "Quit\r\n\r\nCommand aborted.\r\n$gdb_prompt $" {
 		send_log "$internal_pass (Command aborted)\n"
-- 
2.14.3