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

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

[Bruno Larsen]

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

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

[Bruno Larsen]

ping

Cheers!
Bruno Larsen

On 5/4/22 16:10, Bruno Larsen wrote:
> 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)

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

[Bruno Larsen]

ping

Cheers!
Bruno Larsen

On 5/18/22 08:24, Bruno Larsen wrote:
> ping
> 
> Cheers!
> Bruno Larsen
> 
> On 5/4/22 16:10, Bruno Larsen wrote:
>> 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)

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

[Bruno Larsen]

ping

Cheers!
Bruno Larsen

On 5/24/22 08:25, Bruno Larsen wrote:
> ping
> 
> Cheers!
> Bruno Larsen
> 
> On 5/18/22 08:24, Bruno Larsen wrote:
>> ping
>>
>> Cheers!
>> Bruno Larsen
>>
>> On 5/4/22 16:10, Bruno Larsen wrote:
>>> 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)

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

[Bruno Larsen]

ping!

Cheers!
Bruno Larsen

On 5/30/22 08:16, Bruno Larsen wrote:
> ping
> 
> Cheers!
> Bruno Larsen
> 
> On 5/24/22 08:25, Bruno Larsen wrote:
>> ping
>>
>> Cheers!
>> Bruno Larsen
>>
>> On 5/18/22 08:24, Bruno Larsen wrote:
>>> ping
>>>
>>> Cheers!
>>> Bruno Larsen
>>>
>>> On 5/4/22 16:10, Bruno Larsen wrote:
>>>> 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)

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

[Bruno Larsen]

ping!

Cheers!
Bruno Larsen

On 6/6/22 09:46, Bruno Larsen wrote:
> ping!
> 
> Cheers!
> Bruno Larsen
> 
> On 5/30/22 08:16, Bruno Larsen wrote:
>> ping
>>
>> Cheers!
>> Bruno Larsen
>>
>> On 5/24/22 08:25, Bruno Larsen wrote:
>>> ping
>>>
>>> Cheers!
>>> Bruno Larsen
>>>
>>> On 5/18/22 08:24, Bruno Larsen wrote:
>>>> ping
>>>>
>>>> Cheers!
>>>> Bruno Larsen
>>>>
>>>> On 5/4/22 16:10, Bruno Larsen wrote:
>>>>> 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)

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

[Tom Tromey]

>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

Bruno> @@ -738,8 +738,17 @@ call_function_by_hand (struct value *function,
Bruno>  		       type *default_return_type,
Bruno>  		       gdb::array_view<value *> args)
Bruno>  {
Bruno> -  return call_function_by_hand_dummy (function, default_return_type,
Bruno> -				      args, NULL, NULL);
Bruno> +  scoped_restore_selected_frame restore_frame;
Bruno> +  struct value *ret = call_function_by_hand_dummy (function,
Bruno> +						   default_return_type,
Bruno> +						   args, NULL, NULL);
Bruno> +  /* Ensure that the frame cache does not contain any frames that were
Bruno> +     specific to the handmade function call.  If something is leftover
Bruno> +     while GDB has kept a reference to a frame (for instance, when a
Bruno> +     stacktrace is being printed and a pretty printed called an inferior
Bruno> +     function), GDB could crash.  */
Bruno> +  reinit_frame_cache ();

I don't understand the need for this call.
It seems like it must be redundant, because the frame cache should be
reset by causing the inferior to run during the inferior call.

Bruno> +      struct frame_id id = get_frame_id (get_selected_frame (NULL));
Bruno>        if (execution_direction == EXEC_REVERSE)
Bruno>  	gdb_printf (_("Run back to call of "));
Bruno>        else
Bruno> @@ -1866,6 +1867,10 @@ finish_command (const char *arg, int from_tty)
Bruno>  	}
 
Bruno>        print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
Bruno> +      frame = frame_find_by_id (id);
Bruno> +      if (frame == nullptr)
Bruno> +	  error (_("frames disappeared mid-printing."));
Bruno> +      frame = get_prev_frame (frame);

This code probably needs a comment since it's non-obvious why 'finish'
would need to do the frame-id thing.

I think error text normally does not end in ".".
I see a lot of these though, I wonder if we ought to remove them all.

Bruno> +	    error (_("frames disappeared mid-printing."));

Here too.

Bruno> +  /* print_frame can invalidate frame, so cache the frame_id to rebuild
Bruno> +     the whole stack later, if needed.  */
Bruno> +  struct frame_id frame_id = get_frame_id (frame);

This repeated code makes me wonder if perhaps the print_frame_info API
ought to be changed instead.

Tom

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

[Bruno Larsen]

On 6/15/22 12:43, Tom Tromey wrote:
>>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Bruno> @@ -738,8 +738,17 @@ call_function_by_hand (struct value *function,
> Bruno>  		       type *default_return_type,
> Bruno>  		       gdb::array_view<value *> args)
> Bruno>  {
> Bruno> -  return call_function_by_hand_dummy (function, default_return_type,
> Bruno> -				      args, NULL, NULL);
> Bruno> +  scoped_restore_selected_frame restore_frame;
> Bruno> +  struct value *ret = call_function_by_hand_dummy (function,
> Bruno> +						   default_return_type,
> Bruno> +						   args, NULL, NULL);
> Bruno> +  /* Ensure that the frame cache does not contain any frames that were
> Bruno> +     specific to the handmade function call.  If something is leftover
> Bruno> +     while GDB has kept a reference to a frame (for instance, when a
> Bruno> +     stacktrace is being printed and a pretty printed called an inferior
> Bruno> +     function), GDB could crash.  */
> Bruno> +  reinit_frame_cache ();
> 
> I don't understand the need for this call.
> It seems like it must be redundant, because the frame cache should be
> reset by causing the inferior to run during the inferior call.

Oh yes, it must be from an earlier iteration (or me just not understanding how frames work).

All the changes to the function will be reverted locally, as the scoped_restore was here
because of the re-initialization of the frame cache.

> 
> Bruno> +      struct frame_id id = get_frame_id (get_selected_frame (NULL));
> Bruno>        if (execution_direction == EXEC_REVERSE)
> Bruno>  	gdb_printf (_("Run back to call of "));
> Bruno>        else
> Bruno> @@ -1866,6 +1867,10 @@ finish_command (const char *arg, int from_tty)
> Bruno>  	}
>   
> Bruno>        print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
> Bruno> +      frame = frame_find_by_id (id);
> Bruno> +      if (frame == nullptr)
> Bruno> +	  error (_("frames disappeared mid-printing."));
> Bruno> +      frame = get_prev_frame (frame);
> 
> This code probably needs a comment since it's non-obvious why 'finish'
> would need to do the frame-id thing.

I'll add a comment too, but as an explanation to you: Everywhere that prints a frame
will need this because if a frame is printed, it may call a function by hand, which
makes this pointer stale.

> 
> I think error text normally does not end in ".".
> I see a lot of these though, I wonder if we ought to remove them all.

A very rough 'grep error.*\.\" ' showed 32 lines with this.

Since I'm already making a new version, I can change this on my patch, no problem

> 
> Bruno> +	    error (_("frames disappeared mid-printing."));
> 
> Here too.
> 
> Bruno> +  /* print_frame can invalidate frame, so cache the frame_id to rebuild
> Bruno> +     the whole stack later, if needed.  */
> Bruno> +  struct frame_id frame_id = get_frame_id (frame);
> 
> This repeated code makes me wonder if perhaps the print_frame_info API
> ought to be changed instead.

print_frame_info could return an updated pointer to the frame being printed, or receive a
frame_info** and update it in-place.  However, We'll still need to check for a null pointer
after calling, since we won't want print_frame_info to error out if a frame no longer
exists.  We might not care when printing.

Does any of these options sound like a better option? Or would we always want to error out
if the frame disappears from under us?


Cheers!
Bruno Larsen
> 
> Tom
> 

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

[Bruno Larsen]

On 6/20/22 10:12, Bruno Larsen wrote:
> 
> 
> On 6/15/22 12:43, Tom Tromey wrote:
>>>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>>
...
>>
>> Bruno> +  /* print_frame can invalidate frame, so cache the frame_id to rebuild
>> Bruno> +     the whole stack later, if needed.  */
>> Bruno> +  struct frame_id frame_id = get_frame_id (frame);
>>
>> This repeated code makes me wonder if perhaps the print_frame_info API
>> ought to be changed instead.
> 
> print_frame_info could return an updated pointer to the frame being printed, or receive a
> frame_info** and update it in-place.  However, We'll still need to check for a null pointer
> after calling, since we won't want print_frame_info to error out if a frame no longer
> exists.  We might not care when printing.
> 
> Does any of these options sound like a better option? Or would we always want to error out
> if the frame disappears from under us?
> 

I've spent a couple of days checking this, but to avoid repeating the code, there is just so
much to be re-written that trying to fix the issues you had with making frame_info* smart pointers
seems to be a better use of my time.

Is this a blocking issue, or can the patch be merged with some repeated code and we fix this later?


Cheers!
Bruno Larsen
> 
> Cheers!
> Bruno Larsen
>>
>> Tom
>>