[binutils-gdb] Fix PR19927: Avoid unwinder recursion if sniffer uses calls parse_and_eval

[Pedro Alves <palves at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=f245535cf583ae4ca13b10d47b3c7d3334593ece

commit f245535cf583ae4ca13b10d47b3c7d3334593ece
Author: Pedro Alves <palves@redhat.com>
Date:   Mon Sep 5 18:41:38 2016 +0100

    Fix PR19927: Avoid unwinder recursion if sniffer uses calls parse_and_eval
    
    This fixes the problem exercised by Kevin's test at:
    
     https://sourceware.org/ml/gdb-patches/2016-08/msg00216.html
    
    This was originally exposed by the OpenJDK Python-based unwinder.
    
    If an unwinder attempts to call parse_and_eval from within its
    sniffing method, GDB's unwinding machinery enters infinite recursion.
    However, parse_and_eval is a pretty reasonable thing to call, because
    Python/Scheme-based unwinders will often need to read globals out of
    inferior memory.  The recursion happens because:
    
    - get_current_frame() is called soon after the target stops.
    
    - current_frame is NULL, and so we unwind it from the sentinel frame
      (which is special and has level == -1).
    
    - We reach get_prev_frame_if_no_cycle, which does cycle detection
      based on frame id, and thus tries to compute the frame id of the new
      frame.
    
    - Frame id computation requires an unwinder, so we go through all
      unwinder sniffers trying to see if one accepts the new frame (the
      current frame).
    
    - the unwinder's sniffer calls parse_and_eval().
    
    - parse_and_eval depends on the selected frame/block, and if not set
      yet, the selected frame is set to the current frame.
    
    - get_current_frame () is called again.  current_frame is still NULL,
      so ...
    
    - recurse forever.
    
    
    In Kevin's test at:
    
     https://sourceware.org/ml/gdb-patches/2016-08/msg00216.html
    
    gdb doesn't recurse forever simply because the Python unwinder
    contains code to detect and stop the recursion itself.  However, GDB
    goes downhill from here, e.g., by showing the sentinel frame as
    current frame (note the -1):
    
        Breakpoint 1, ccc (arg=<unavailable>) at py-recurse-unwind.c:23
        23      }
        (gdb) bt
        #-1 ccc (arg=<unavailable>) at py-recurse-unwind.c:23
        Backtrace stopped: previous frame identical to this frame (corrupt stack?)
    
    That "-1" frame level comes from this:
    
          if (catch_exceptions (current_uiout, unwind_to_current_frame,
    			    sentinel_frame, RETURN_MASK_ERROR) != 0)
    	{
    	  /* Oops! Fake a current frame?  Is this useful?  It has a PC
                 of zero, for instance.  */
    	  current_frame = sentinel_frame;
    	}
    
    which is bogus.  It's never correct to set the current frame to the
    sentinel frame.  The only reason this has survived so long is that
    getting here normally indicates something wrong has already happened
    before and we fix that.  And this case is no exception -- it doesn't
    really matter how precisely we managed to get to that bogus code (it
    has to do with the the stash), because anything after recursion
    happens is going to be invalid.
    
    So the fix is to avoid the recursion in the first place.
    
    Observations:
    
     #1 - The recursion happens because we try to do cycle detection from
          within get_prev_frame_if_no_cycle.  That requires computing the
          frame id of the frame being unwound, and that itself requires
          calling into the unwinders.
    
     #2 - But, the first time we're unwinding from the sentinel frame,
          when we reach get_prev_frame_if_no_cycle, there's no frame chain
          at all yet:
    
          - current_frame is NULL.
          - the frame stash is empty.
    
    Thus, there's really no need to do cycle detection the first time we
    reach get_prev_frame_if_no_cycle, when building the current frame.
    
    So we can break the recursion by making get_current_frame call a
    simplified version of get_prev_frame_if_no_cycle that results in
    setting the current_frame global _before_ computing the current
    frame's id.
    
    But, we can go a little bit further.  As there's really no reason
    anymore to compute the current frame's frame id immediately, we can
    defer computing it to when some caller of get_current_frame might need
    it.  This was actually how the frame id was computed for all frames
    before the stash-based cycle detection was added.  So in a way, this
    patch reintroduces the lazy frame id computation, but unlike before,
    only for the case of the current frame, which turns out to be special.
    
    This lazyness, however, requires adjusting
    gdb.python/py-unwind-maint.exp, because that assumes unwinders are
    immediately called as side effect of some commands.  I didn't see a
    need to preserve the behavior expected by that test (all it would take
    is call get_frame_id inside get_current_frame), so I adjusted the
    test.
    
    gdb/ChangeLog:
    2016-09-05  Pedro Alves  <palves@redhat.com>
    
    	PR backtrace/19927
    	* frame.c (get_frame_id): Compute the frame id if not computed
    	yet.
    	(unwind_to_current_frame): Delete.
    	(get_current_frame): Use get_prev_frame_always_1 to get the
    	current frame and assert that that always succeeds.
    	(get_prev_frame_if_no_cycle): Skip cycle detection if returning
    	the current frame.
    
    gdb/testsuite/ChangeLog:
    2016-09-05  Pedro Alves  <palves@redhat.com>
    
    	PR backtrace/19927
    	* gdb.python/py-unwind-maint.exp: Adjust tests to not expect that
    	unwinders are immediately called as side effect of "source" or
    	"disable unwinder" commands.
    	* gdb.python/py-recurse-unwind.exp: Remove setup_kfail calls.

Diff:
---
 gdb/ChangeLog                                  | 11 ++++
 gdb/frame.c                                    | 79 +++++++++++++++++---------
 gdb/testsuite/ChangeLog                        |  8 +++
 gdb/testsuite/gdb.python/py-recurse-unwind.exp |  2 -
 gdb/testsuite/gdb.python/py-unwind-maint.exp   |  8 ++-
 5 files changed, 76 insertions(+), 32 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 222f787..1fb363d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2016-09-05  Pedro Alves  <palves@redhat.com>
+
+	PR backtrace/19927
+	* frame.c (get_frame_id): Compute the frame id if not computed
+	yet.
+	(unwind_to_current_frame): Delete.
+	(get_current_frame): Use get_prev_frame_always_1 to get the
+	current frame and assert that that always succeeds.
+	(get_prev_frame_if_no_cycle): Skip cycle detection if returning
+	the current frame.
+
 2016-09-02  Tom Tromey  <tom@tromey.com>
 
 	PR gdb/11616:
diff --git a/gdb/frame.c b/gdb/frame.c
index c25ce4c..d3f3056 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -511,7 +511,26 @@ get_frame_id (struct frame_info *fi)
   if (fi == NULL)
     return null_frame_id;
 
-  gdb_assert (fi->this_id.p);
+  if (!fi->this_id.p)
+    {
+      int stashed;
+
+      /* If we haven't computed the frame id yet, then it must be that
+	 this is the current frame.  Compute it now, and stash the
+	 result.  The IDs of other frames are computed as soon as
+	 they're created, in order to detect cycles.  See
+	 get_prev_frame_if_no_cycle.  */
+      gdb_assert (fi->level == 0);
+
+      /* Compute.  */
+      compute_frame_id (fi);
+
+      /* Since this is the first frame in the chain, this should
+	 always succeed.  */
+      stashed = frame_stash_add (fi);
+      gdb_assert (stashed);
+    }
+
   return fi->this_id.value;
 }
 
@@ -1487,23 +1506,7 @@ frame_obstack_zalloc (unsigned long size)
   return data;
 }
 
-/* Return the innermost (currently executing) stack frame.  This is
-   split into two functions.  The function unwind_to_current_frame()
-   is wrapped in catch exceptions so that, even when the unwind of the
-   sentinel frame fails, the function still returns a stack frame.  */
-
-static int
-unwind_to_current_frame (struct ui_out *ui_out, void *args)
-{
-  struct frame_info *frame = get_prev_frame ((struct frame_info *) args);
-
-  /* A sentinel frame can fail to unwind, e.g., because its PC value
-     lands in somewhere like start.  */
-  if (frame == NULL)
-    return 1;
-  current_frame = frame;
-  return 0;
-}
+static struct frame_info *get_prev_frame_always_1 (struct frame_info *this_frame);
 
 struct frame_info *
 get_current_frame (void)
@@ -1525,16 +1528,28 @@ get_current_frame (void)
 
   if (current_frame == NULL)
     {
+      int stashed;
       struct frame_info *sentinel_frame =
 	create_sentinel_frame (current_program_space, get_current_regcache ());
-      if (catch_exceptions (current_uiout, unwind_to_current_frame,
-			    sentinel_frame, RETURN_MASK_ERROR) != 0)
-	{
-	  /* Oops! Fake a current frame?  Is this useful?  It has a PC
-             of zero, for instance.  */
-	  current_frame = sentinel_frame;
-	}
+
+      /* Set the current frame before computing the frame id, to avoid
+	 recursion inside compute_frame_id, in case the frame's
+	 unwinder decides to do a symbol lookup (which depends on the
+	 selected frame's block).
+
+	 This call must always succeed.  In particular, nothing inside
+	 get_prev_frame_always_1 should try to unwind from the
+	 sentinel frame, because that could fail/throw, and we always
+	 want to leave with the current frame created and linked in --
+	 we should never end up with the sentinel frame as outermost
+	 frame.  */
+      current_frame = get_prev_frame_always_1 (sentinel_frame);
+      gdb_assert (current_frame != NULL);
+
+      /* No need to compute the frame id yet.  That'll be done lazily
+	 from inside get_frame_id instead.  */
     }
+
   return current_frame;
 }
 
@@ -1812,8 +1827,18 @@ get_prev_frame_if_no_cycle (struct frame_info *this_frame)
   struct cleanup *prev_frame_cleanup;
 
   prev_frame = get_prev_frame_raw (this_frame);
-  if (prev_frame == NULL)
-    return NULL;
+
+  /* Don't compute the frame id of the current frame yet.  Unwinding
+     the sentinel frame can fail (e.g., if the thread is gone and we
+     can't thus read its registers).  If we let the cycle detection
+     code below try to compute a frame ID, then an error thrown from
+     within the frame ID computation would result in the sentinel
+     frame as outermost frame, which is bogus.  Instead, we'll compute
+     the current frame's ID lazily in get_frame_id.  Note that there's
+     no point in doing cycle detection when there's only one frame, so
+     nothing is lost here.  */
+  if (prev_frame->level == 0)
+    return prev_frame;
 
   /* The cleanup will remove the previous frame that get_prev_frame_raw
      linked onto THIS_FRAME.  */
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index ffb993c..14cbb5a 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,11 @@
+2016-09-05  Pedro Alves  <palves@redhat.com>
+
+	PR backtrace/19927
+	* gdb.python/py-unwind-maint.exp: Adjust tests to not expect that
+	unwinders are immediately called as side effect of "source" or
+	"disable unwinder" commands.
+	* gdb.python/py-recurse-unwind.exp: Remove setup_kfail calls.
+
 2016-09-02  Yao Qi  <yao.qi@linaro.org>
 
 	* gdb.base/return-nodebug.exp: Skip the test if	skip_float_test
diff --git a/gdb/testsuite/gdb.python/py-recurse-unwind.exp b/gdb/testsuite/gdb.python/py-recurse-unwind.exp
index fe17d3f..9629a97 100644
--- a/gdb/testsuite/gdb.python/py-recurse-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-recurse-unwind.exp
@@ -61,7 +61,6 @@ gdb_test_no_output "python TestUnwinder.reset_count()"
 # get hung up in potentially infinite recursion when invoking the
 # Python-based unwinder.
 
-setup_kfail "gdb/19927" "*-*-*"
 gdb_test_sequence "bt"  "backtrace" {
     "\\r\\n#0 .* ccc \\(arg=789\\) at "
     "\\r\\n#1 .* bbb \\(arg=456\\) at "
@@ -71,5 +70,4 @@ gdb_test_sequence "bt"  "backtrace" {
 
 # Test that the python-based unwinder / sniffer was actually called
 # during generation of the backtrace.
-setup_kfail "gdb/19927" "*-*-*"
 gdb_test "python print(TestUnwinder.count > 0)" "True"
diff --git a/gdb/testsuite/gdb.python/py-unwind-maint.exp b/gdb/testsuite/gdb.python/py-unwind-maint.exp
index 3a98cb1..1253057 100644
--- a/gdb/testsuite/gdb.python/py-unwind-maint.exp
+++ b/gdb/testsuite/gdb.python/py-unwind-maint.exp
@@ -34,10 +34,13 @@ if ![runto_main ] then {
     return -1
 }
 
-gdb_test_sequence "source ${pyfile}" "import python scripts" {
-    "Python script imported"
+gdb_test "source ${pyfile}" "Python script imported" \
+    "import python scripts"
+
+gdb_test_sequence "frame" "All unwinders enabled" {
     "py_unwind_maint_ps_unwinder called"
     "global_unwinder called"
+    "#0  main"
 }
 
 gdb_test_sequence "info unwinder" "Show all unwinders" {
@@ -56,7 +59,6 @@ gdb_test_sequence "continue" "Unwinders called" {
 
 gdb_test_sequence "disable unwinder global .*" "Unwinder disabled" {
     "1 unwinder disabled"
-    "py_unwind_maint_ps_unwinder called"
 }
 
 gdb_test_sequence "info unwinder" "Show with global unwinder disabled" {