[PATCH v2] gdb/stack.c: avoid stale pointers when printing frame arguments

[Bruno Larsen <blarsen@redhat.com>]

When a pretty printer calls an inferior function by hand, the whole
framestack cache has to be rebuilt for that function call. However, if
this pretty printer is called when printing the arguments of a frame,
there are many instances where frame_info pointers were kept and end up
stale, or used after free. This could snowball into an infinite
recursion and GDB crash. The problem was documented as PR python/28856.

To fix this problem, we must ensure that the frame cache is up to date
after printing any arguments, so we cache the frame_id of the pointer
that will become stale, and reset that pointer using frame_find_by_id.

This commit also adds a testcase that exercises this codepath with 7
different triggers, run, continue, step, backtrace, finish, up and down.
Some of them can seem to be testing the same thing twice, but since this
test relies on stale pointers, there is always a chance that GDB got lucky
when testing, so better to test extra.

Regression tested on x86_64, using both gcc and clang.

---
Changelog:
v2:
 * Addressed Tom Tromey's and Andrew's comments
 * Added the testcase to this patch
 * Slightly changed "finish" test, to deal better with clang

---
 gdb/infcall.c                                 |  13 +-
 gdb/infcmd.c                                  |   5 +
 gdb/mi/mi-cmd-stack.c                         |   6 +
 gdb/stack.c                                   |  31 +++-
 .../gdb.python/pretty-print-call-by-hand.c    |  53 +++++++
 .../gdb.python/pretty-print-call-by-hand.exp  | 136 ++++++++++++++++++
 .../gdb.python/pretty-print-call-by-hand.py   |  41 ++++++
 7 files changed, 280 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
 create mode 100644 gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
 create mode 100644 gdb/testsuite/gdb.python/pretty-print-call-by-hand.py

diff --git a/gdb/infcall.c b/gdb/infcall.c
index 5365f97049c..ad31a910da5 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -738,8 +738,17 @@ call_function_by_hand (struct value *function,
 		       type *default_return_type,
 		       gdb::array_view<value *> args)
 {
-  return call_function_by_hand_dummy (function, default_return_type,
-				      args, NULL, NULL);
+  scoped_restore_selected_frame restore_frame;
+  struct value *ret = call_function_by_hand_dummy (function,
+						   default_return_type,
+						   args, NULL, NULL);
+  /* Ensure that the frame cache does not contain any frames that were
+     specific to the handmade function call.  If something is leftover
+     while GDB has kept a reference to a frame (for instance, when a
+     stacktrace is being printed and a pretty printed called an inferior
+     function), GDB could crash.  */
+  reinit_frame_cache ();
+  return ret;
 }
 
 /* All this stuff with a dummy frame may seem unnecessarily complicated
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index e909d4d4c81..380d5f0201b 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1853,6 +1853,7 @@ finish_command (const char *arg, int from_tty)
      source.  */
   if (from_tty)
     {
+      struct frame_id id = get_frame_id (get_selected_frame (NULL));
       if (execution_direction == EXEC_REVERSE)
 	gdb_printf (_("Run back to call of "));
       else
@@ -1866,6 +1867,10 @@ finish_command (const char *arg, int from_tty)
 	}
 
       print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
+      frame = frame_find_by_id (id);
+      if (frame == nullptr)
+	  error (_("frames disappeared mid-printing."));
+      frame = get_prev_frame (frame);
     }
 
   if (execution_direction == EXEC_REVERSE)
diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index 0fe204dbc66..51c6573e9be 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -176,10 +176,16 @@ mi_cmd_stack_list_frames (const char *command, char **argv, int argc)
 	   i++, fi = get_prev_frame (fi))
 	{
 	  QUIT;
+	  /* We get the frame ID here because getting it in the for line
+	     could result in getting the previous's frame ID instead.  */
+	  struct frame_id fid = get_frame_id (fi);
 	  /* Print the location and the address always, even for level 0.
 	     If args is 0, don't print the arguments.  */
 	  print_frame_info (user_frame_print_options,
 			    fi, 1, LOC_AND_ADDRESS, 0 /* args */, 0);
+	  fi = frame_find_by_id (fid);
+	  if (fi == nullptr)
+	    error (_("frames disappeared mid-printing."));
 	}
     }
 }
diff --git a/gdb/stack.c b/gdb/stack.c
index 71d85985d18..52fc8af3cb9 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -364,11 +364,19 @@ print_stack_frame (struct frame_info *frame, int print_level,
 
   try
     {
+      /* print_frame_info may invalidate FRAME.
+         It's better to use frame_id if we want to set the current sal.  */
+      struct frame_id fid = get_frame_id (frame);
       print_frame_info (user_frame_print_options,
 			frame, print_level, print_what, 1 /* print_args */,
 			set_current_sal);
       if (set_current_sal)
-	set_current_sal_from_frame (frame);
+        {
+	  frame = frame_find_by_id (fid);
+	  if (frame == nullptr)
+	      error (_("frames disappeared mid-printing."));
+	  set_current_sal_from_frame (frame);
+	}
     }
   catch (const gdb_exception_error &e)
     {
@@ -742,6 +750,8 @@ print_frame_args (const frame_print_options &fp_opts,
     = (print_names
        && fp_opts.print_frame_arguments != print_frame_arguments_none);
 
+  struct frame_id printed_frame = get_frame_id (frame);
+
   /* Temporarily change the selected frame to the given FRAME.
      This allows routines that rely on the selected frame instead
      of being given a frame as parameter to use the correct frame.  */
@@ -902,6 +912,9 @@ print_frame_args (const frame_print_options &fp_opts,
 	    }
 
 	  first = 0;
+	  frame = frame_find_by_id (printed_frame);
+	  if (frame == nullptr)
+	    error (_("frames disappeared mid-printing."));
 	}
     }
 
@@ -1109,12 +1122,21 @@ print_frame_info (const frame_print_options &fp_opts,
      to get the line containing FRAME->pc.  */
   symtab_and_line sal = find_frame_sal (frame);
 
+  /* print_frame can invalidate frame, so cache the frame_id to rebuild
+     the whole stack later, if needed.  */
+  struct frame_id frame_id = get_frame_id (frame);
+
   location_print = (print_what == LOCATION
 		    || print_what == SRC_AND_LOC
 		    || print_what == LOC_AND_ADDRESS
 		    || print_what == SHORT_LOCATION);
   if (location_print || !sal.symtab)
-    print_frame (fp_opts, frame, print_level, print_what, print_args, sal);
+    {
+      print_frame (fp_opts, frame, print_level, print_what, print_args, sal);
+      frame = frame_find_by_id (frame_id);
+      if (frame == nullptr)
+	  error (_("frames disappeared mid-printing."));
+    }
 
   source_print = (print_what == SRC_LINE || print_what == SRC_AND_LOC);
 
@@ -2060,6 +2082,7 @@ backtrace_command_1 (const frame_print_options &fp_opts,
       for (fi = trailing; fi && count--; fi = get_prev_frame (fi))
 	{
 	  QUIT;
+	  struct frame_id frame_id = get_frame_id (fi);
 
 	  /* Don't use print_stack_frame; if an error() occurs it probably
 	     means further attempts to backtrace would fail (on the other
@@ -2067,9 +2090,11 @@ backtrace_command_1 (const frame_print_options &fp_opts,
 	     the frame->prev field gets set to NULL in that case).  */
 
 	  print_frame_info (fp_opts, fi, 1, LOCATION, 1, 0);
+	  fi = frame_find_by_id (frame_id);
+	  if (fi == nullptr)
+	    error (_("frames disappeared mid-printing."));
 	  if ((flags & PRINT_LOCALS) != 0)
 	    {
-	      struct frame_id frame_id = get_frame_id (fi);
 
 	      print_frame_local_vars (fi, false, NULL, NULL, 1, gdb_stdout);
 
diff --git a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
new file mode 100644
index 00000000000..3be5675b096
--- /dev/null
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
@@ -0,0 +1,53 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2022 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/>.  */
+
+struct mytype
+{
+  char *x;
+};
+
+void
+rec (int i)
+{
+  if (i <= 0)
+    return;
+  rec (i-1);
+}
+
+int
+f ()
+{
+  rec (100);
+  return 2;
+}
+
+void
+g (struct mytype mt, int depth)
+{
+  if (depth <= 0)
+    return; /* TAG: final frame */
+  g (mt, depth - 1); /* TAG: first frame */
+}
+
+int
+main ()
+{
+  struct mytype mt;
+  mt.x = "hello world";
+  g (mt, 10); /* TAG: outside the frame */
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
new file mode 100644
index 00000000000..0aeb2218f91
--- /dev/null
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
@@ -0,0 +1,136 @@
+# Copyright (C) 2022 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/>.
+
+# This file is part of the GDB testsuite.  It tests a pretty printer that
+# calls an inferior function by hand, triggering a Use-after-Free bug
+# (PR gdb/28856).
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+# gdb needs to be started here for skip_python_tests to work.
+# prepare_for_testing could be used instead, but it could compile the program
+# unnecessarily, so starting GDB like this is preferable.
+gdb_start
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
+    untested "failed to compile"
+    return -1
+}
+
+# This proc restarts GDB, makes the inferior reach the desired spot - marked
+# by a comment in the .c file - and turns on the pretty printer for testing.
+# Starting with a new GDB is important because the test may crash GDB.  The
+# return values are here to avoid us trying to test the pretty printer if
+# there was a problem getting to main.
+proc start_test { breakpoint_comment } {
+    global srcdir subdir testfile binfile
+
+    # Start with a fresh gdb.
+    # This is important because the test can crash GDB.
+
+    clean_restart ${binfile}
+
+    if { ![runto_main] } then {
+	untested "couldn't run to breakpoint"
+	return -1
+    }
+
+    # Let GDB get to the return line.
+    gdb_breakpoint [gdb_get_line_number ${breakpoint_comment} ${testfile}.c ]
+    gdb_continue_to_breakpoint ${breakpoint_comment} ".*"
+
+    gdb_test_no_output "set print pretty on" "starting to pretty print"
+
+    set remote_python_file [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+    gdb_test_no_output "source ${remote_python_file}" "load python file"
+
+    return 0
+}
+
+# Start by testing the "run" command, it can't leverage start_test
+with_test_prefix "run to frame" {
+    if { ![runto_main] } then {
+	untested "couldn't run to main"
+    }
+
+    gdb_test_no_output "set print pretty on" "starting to pretty print"
+
+    set remote_python_file [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+    gdb_test_no_output "source ${remote_python_file}" "load python file"
+
+    gdb_breakpoint [gdb_get_line_number "TAG: final frame" ${testfile}.c]
+    gdb_continue_to_breakpoint "TAG: final frame" ".*"
+}
+
+# Testing the backtrace command.
+with_test_prefix "frame print" {
+    if { [start_test "TAG: final frame"] == 0 } {
+	gdb_test "backtrace -frame-arguments all" [multi_line \
+	"#0 .*g \\(mt=mytype is .*\\, depth=0\\).*"\
+	"#1 .*g \\(mt=mytype is .*\\, depth=1\\).*"\
+	"#2 .*g \\(mt=mytype is .*\\, depth=2\\).*"\
+	"#3 .*g \\(mt=mytype is .*\\, depth=3\\).*"\
+	"#4 .*g \\(mt=mytype is .*\\, depth=4\\).*"\
+	"#5 .*g \\(mt=mytype is .*\\, depth=5\\).*"\
+	"#6 .*g \\(mt=mytype is .*\\, depth=6\\).*"\
+	"#7 .*g \\(mt=mytype is .*\\, depth=7\\).*"\
+	"#8 .*g \\(mt=mytype is .*\\, depth=8\\).*"\
+	"#9 .*g \\(mt=mytype is .*\\, depth=9\\).*"\
+	"#10 .*g \\(mt=mytype is .*\\, depth=10\\).*"\
+	"#11 .*main \\(\\).*"] \
+	"backtrace test"
+    }
+}
+# Testing the down command.
+with_test_prefix "frame movement down" {
+    if { [start_test "TAG: first frame"] == 0 } {
+	gdb_test "up" [multi_line "#1 .*in main \\(\\) at .*" ".*outside the frame.*"]
+	gdb_test "down" [multi_line "#0\\s+g \\(mt=mytype is .*\\, depth=10\\).*" ".*first frame.*"]
+    }
+}
+
+# Testing the up command.
+with_test_prefix "frame movement up" {
+    if { [start_test "TAG: final frame"] == 0 } {
+	gdb_test "up" [multi_line "#1 .*in g \\(mt=mytype is .*\\, depth=1\\).*" ".*first frame.*"]
+    }
+}
+
+# Testing the finish command.
+with_test_prefix "frame exit through finish" {
+    if { [start_test "TAG: final frame"] == 0 } {
+	gdb_test "finish" [multi_line ".*.*g \\(mt=mytype is .*\\, depth=0\\).*" ".*g \\(mt=mytype is .*\\, depth=1\\).*" ".*"]
+    }
+}
+
+# Testing the step command.
+with_test_prefix "frame enter through step" {
+    if { [start_test "TAG: outside the frame"] == 0 } {
+	gdb_test "step" [multi_line "g \\(mt=mytype is .*\\, depth=10\\).*" "41.*if \\(depth \\<= 0\\)"]
+    }
+}
+
+# Testing the continue command.
+with_test_prefix "frame enter through continue" {
+    if { [start_test "TAG: outside the frame"] == 0 } {
+	gdb_breakpoint [gdb_get_line_number "TAG: first frame" ${testfile}.c ]
+	gdb_continue_to_breakpoint "TAG: first frame" ".*TAG: first frame.*"
+    }
+}
diff --git a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
new file mode 100644
index 00000000000..f8f5df678f8
--- /dev/null
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
@@ -0,0 +1,41 @@
+# Copyright (C) 2022 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/>.
+
+
+class MytypePrinter:
+    """pretty print my type"""
+
+    def __init__(self, val):
+        self.val = val
+
+    def to_string(self):
+        calls = gdb.parse_and_eval('f()')
+        return "mytype is %s" % self.val['x']
+
+def ec_lookup_function(val):
+    typ = val.type
+    if typ.code == gdb.TYPE_CODE_REF:
+        typ = typ.target()
+    if str(typ) == 'struct mytype':
+        return MytypePrinter(val)
+    return None
+
+def disable_lookup_function():
+    ec_lookup_function.enabled = False
+
+def enable_lookup_function():
+    ec_lookup_function.enabled = True
+
+gdb.pretty_printers.append(ec_lookup_function)
-- 
2.31.1