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

[PATCH] 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 solution, 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.
We have to use it directly instead of using the simpler
scoped_restore_selected_frame because the latter may save a null frame_ID
when the saved frame is the current frame, and restoring the null frame
wouldn't rebuild the frame cache.

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.

This patch has been regression tested on x86_64

---
 gdb/infcmd.c                                  |   2 +
 gdb/infrun.c                                  |   7 +
 gdb/mi/mi-cmd-stack.c                         |   6 +-
 gdb/stack.c                                   |  20 ++-
 .../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, 261 insertions(+), 4 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/infcmd.c b/gdb/infcmd.c
index ea06ceb992c..ee5b7ddf59d 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1847,6 +1847,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)
 	printf_filtered (_("Run back to call of "));
       else
@@ -1860,6 +1861,7 @@ finish_command (const char *arg, int from_tty)
 	}
 
       print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
+      frame = get_prev_frame (frame_find_by_id (id));
     }
 
   if (execution_direction == EXEC_REVERSE)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 737710f5bae..bf519737d12 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8308,8 +8308,15 @@ print_stop_location (const target_waitstatus &ws)
      SRC_LINE: Print only source line
      LOCATION: Print only location
      SRC_AND_LOC: Print location and source line.  */
+  int frame_cache_count = get_frame_cache_generation ();
   if (do_frame_printing)
     print_stack_frame (get_selected_frame (NULL), 0, source_flag, 1);
+  /* If there are more frame cache generations after we print a stack frame
+     we may have invalidated the cache with a pretty printer.
+     If this happened, the safest option is to restart the whole cache,
+     otherwise GDB may crash because of an use-after-free bug.  */
+  if (frame_cache_count < get_frame_cache_generation ())
+    reinit_frame_cache ();
 }
 
 /* See infrun.h.  */
diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index e894411765a..ce5b93420ca 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -171,15 +171,19 @@ mi_cmd_stack_list_frames (const char *command, char **argv, int argc)
     {
       /* Now let's print the frames up to frame_high, or until there are
 	 frames in the stack.  */
-      for (;
+      for (struct frame_id fid;
 	   fi && (i <= frame_high || frame_high == -1);
 	   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.  */
+	  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);
 	}
     }
 }
diff --git a/gdb/stack.c b/gdb/stack.c
index 10da88b88e5..198eb9e485f 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -364,11 +364,14 @@ 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);
+	set_current_sal_from_frame (frame_find_by_id (fid));
     }
   catch (const gdb_exception_error &e)
     {
@@ -742,6 +745,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 +907,7 @@ print_frame_args (const frame_print_options &fp_opts,
 	    }
 
 	  first = 0;
+	  frame = frame_find_by_id (printed_frame);
 	}
     }
 
@@ -1109,12 +1115,19 @@ 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);
+    }
 
   source_print = (print_what == SRC_LINE || print_what == SRC_AND_LOC);
 
@@ -2060,6 +2073,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 +2081,9 @@ 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 ((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..11a1b2e62fb
--- /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\\).*" ".*TAG: first frame.*"]
+    }
+}
+
+# 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

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

[Tom Tromey]

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

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

Makes sense.

Bruno> We have to use it directly instead of using the simpler
Bruno> scoped_restore_selected_frame because the latter may save a null frame_ID
Bruno> when the saved frame is the current frame, and restoring the null frame
Bruno> wouldn't rebuild the frame cache.

Also it's not the selected frame that is being restored.

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

One thing I've toyed with a couple of times is replacing "frame_info *"
with a "frame_info_ptr" smart pointer that automatically clears itself
when the frame cache is cleared.  This would then cause reliable crashes
in scenarios like this -- with the caveat, of course, that some test has
to actually exercise the failing code path.

This got bogged down a bit because I then tried to have frame_info_ptr
automatically reinflate, which turns out to be hard for some reason I
don't fully recall.

Maybe a reinflating frame_info_ptr could be done piecemeal, starting
with the spots you identified in this patch, though...

Bruno> diff --git a/gdb/infrun.c b/gdb/infrun.c
Bruno> index 737710f5bae..bf519737d12 100644
Bruno> --- a/gdb/infrun.c
Bruno> +++ b/gdb/infrun.c
Bruno> @@ -8308,8 +8308,15 @@ print_stop_location (const target_waitstatus &ws)
Bruno>       SRC_LINE: Print only source line
Bruno>       LOCATION: Print only location
Bruno>       SRC_AND_LOC: Print location and source line.  */
Bruno> +  int frame_cache_count = get_frame_cache_generation ();
Bruno>    if (do_frame_printing)
Bruno>      print_stack_frame (get_selected_frame (NULL), 0, source_flag, 1);
Bruno> +  /* If there are more frame cache generations after we print a stack frame
Bruno> +     we may have invalidated the cache with a pretty printer.
Bruno> +     If this happened, the safest option is to restart the whole cache,
Bruno> +     otherwise GDB may crash because of an use-after-free bug.  */
Bruno> +  if (frame_cache_count < get_frame_cache_generation ())
Bruno> +    reinit_frame_cache ();
Bruno>  }

I don't think I understand this bit.  If the generation changes, hasn't
the cache already been cleared?

Bruno> +      for (struct frame_id fid;
Bruno>  	   fi && (i <= frame_high || frame_high == -1);
Bruno>  	   i++, fi = get_prev_frame (fi))

Better to declare fid where it's first used.

Tom

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

[Bruno Larsen]

On 4/4/22 16:06, Tom Tromey wrote:
>>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Bruno> To fix this solution, we must ensure that the frame cache is up to date
> Bruno> after printing any arguments, so we cache the frame_id of the pointer
> Bruno> that will become stale, and reset that pointer using frame_find_by_id.
> 
> Makes sense.
> 
> Bruno> We have to use it directly instead of using the simpler
> Bruno> scoped_restore_selected_frame because the latter may save a null frame_ID
> Bruno> when the saved frame is the current frame, and restoring the null frame
> Bruno> wouldn't rebuild the frame cache.
> 
> Also it's not the selected frame that is being restored.
> 
> Bruno> This commit also adds a testcase that exercises this codepath with 7
> Bruno> different triggers, run, continue, step, backtrace, finish, up and down.
> Bruno> Some of them can seem to be testing the same thing twice, but since this
> Bruno> test relies on stale pointers, there is always a chance that GDB got lucky
> Bruno> when testing, so better to test extra.
> 
> One thing I've toyed with a couple of times is replacing "frame_info *"
> with a "frame_info_ptr" smart pointer that automatically clears itself
> when the frame cache is cleared.  This would then cause reliable crashes
> in scenarios like this -- with the caveat, of course, that some test has
> to actually exercise the failing code path.
> 
> This got bogged down a bit because I then tried to have frame_info_ptr
> automatically reinflate, which turns out to be hard for some reason I
> don't fully recall.
> 
> Maybe a reinflating frame_info_ptr could be done piecemeal, starting
> with the spots you identified in this patch, though...

This sounds like a good idea. I am just not sure if you are suggesting it as a fix instead of what I proposed, or to implement later, can you clarify it please?

> 
> Bruno> diff --git a/gdb/infrun.c b/gdb/infrun.c
> Bruno> index 737710f5bae..bf519737d12 100644
> Bruno> --- a/gdb/infrun.c
> Bruno> +++ b/gdb/infrun.c
> Bruno> @@ -8308,8 +8308,15 @@ print_stop_location (const target_waitstatus &ws)
> Bruno>       SRC_LINE: Print only source line
> Bruno>       LOCATION: Print only location
> Bruno>       SRC_AND_LOC: Print location and source line.  */
> Bruno> +  int frame_cache_count = get_frame_cache_generation ();
> Bruno>    if (do_frame_printing)
> Bruno>      print_stack_frame (get_selected_frame (NULL), 0, source_flag, 1);
> Bruno> +  /* If there are more frame cache generations after we print a stack frame
> Bruno> +     we may have invalidated the cache with a pretty printer.
> Bruno> +     If this happened, the safest option is to restart the whole cache,
> Bruno> +     otherwise GDB may crash because of an use-after-free bug.  */
> Bruno> +  if (frame_cache_count < get_frame_cache_generation ())
> Bruno> +    reinit_frame_cache ();
> Bruno>  }
> 
> I don't think I understand this bit.  If the generation changes, hasn't
> the cache already been cleared?

If the cache has been cleared by printing a frame, it was done because a function was called manually (probably). If it did happen, the cache may have been invalidated and it is safer to rebuild everything. print_frame_cache by itself doesn't reinitialize the frame cache. I thought it crashed GDB if I did it there, but I was apparently mistaken, so I can move this code there and have a more robust solution.

I should probably also reword the comment to make things more clear.

> 
> Bruno> +      for (struct frame_id fid;
> Bruno>  	   fi && (i <= frame_high || frame_high == -1);
> Bruno>  	   i++, fi = get_prev_frame (fi))
> 
> Better to declare fid where it's first used

Will do.

> 
> Tom
> 


-- 
Cheers!
Bruno Larsen

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

[Tom Tromey]

>>>>> "Bruno" == Bruno Larsen <blarsen@redhat.com> writes:

Bruno> This sounds like a good idea. I am just not sure if you are
Bruno> suggesting it as a fix instead of what I proposed, or to
Bruno> implement later, can you clarify it please?

You don't have to do it.

Bruno> +  if (frame_cache_count < get_frame_cache_generation ())
Bruno> +    reinit_frame_cache ();

>> I don't think I understand this bit.  If the generation changes,
>> hasn't
>> the cache already been cleared?

Bruno> If the cache has been cleared by printing a frame, it was done because
Bruno> a function was called manually (probably). If it did happen, the cache
Bruno> may have been invalidated and it is safer to rebuild
Bruno> everything. print_frame_cache by itself doesn't reinitialize the frame
Bruno> cache.

My understanding is that the generation only changes when
reinit_frame_cache is called.  So if that's the case, why does it need
to be reinitalized a second time?  That's the part I don't understand.

thanks,
Tom

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

[Bruno Larsen]

On 4/5/22 10:58, Tom Tromey wrote:
>>>>>> "Bruno" == Bruno Larsen <blarsen@redhat.com> writes:
> 
> Bruno> This sounds like a good idea. I am just not sure if you are
> Bruno> suggesting it as a fix instead of what I proposed, or to
> Bruno> implement later, can you clarify it please?
> 
> You don't have to do it.
> 
> Bruno> +  if (frame_cache_count < get_frame_cache_generation ())
> Bruno> +    reinit_frame_cache ();
> 
>>> I don't think I understand this bit.  If the generation changes,
>>> hasn't
>>> the cache already been cleared?
> 
> Bruno> If the cache has been cleared by printing a frame, it was done because
> Bruno> a function was called manually (probably). If it did happen, the cache
> Bruno> may have been invalidated and it is safer to rebuild
> Bruno> everything. print_frame_cache by itself doesn't reinitialize the frame
> Bruno> cache.
> 
> My understanding is that the generation only changes when
> reinit_frame_cache is called.  So if that's the case, why does it need
> to be reinitalized a second time?  That's the part I don't understand.

It has been reinitialized to call the function by hand (if you set debug frame 1 and call something by hand, you'll see a few refreshes), but it is not been refreshed after finishing the handmade call. We might still have dummy frames or incorrect information leftover from the function call.

> 
> thanks,
> Tom
> 


-- 
Cheers!
Bruno Larsen

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

[Andrew Burgess]

* Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> [2022-04-05 11:47:20 -0300]:

> On 4/5/22 10:58, Tom Tromey wrote:
> > > > > > > "Bruno" == Bruno Larsen <blarsen@redhat.com> writes:
> > 
> > Bruno> This sounds like a good idea. I am just not sure if you are
> > Bruno> suggesting it as a fix instead of what I proposed, or to
> > Bruno> implement later, can you clarify it please?
> > 
> > You don't have to do it.
> > 
> > Bruno> +  if (frame_cache_count < get_frame_cache_generation ())
> > Bruno> +    reinit_frame_cache ();
> > 
> > > > I don't think I understand this bit.  If the generation changes,
> > > > hasn't
> > > > the cache already been cleared?
> > 
> > Bruno> If the cache has been cleared by printing a frame, it was done because
> > Bruno> a function was called manually (probably). If it did happen, the cache
> > Bruno> may have been invalidated and it is safer to rebuild
> > Bruno> everything. print_frame_cache by itself doesn't reinitialize the frame
> > Bruno> cache.
> > 
> > My understanding is that the generation only changes when
> > reinit_frame_cache is called.  So if that's the case, why does it need
> > to be reinitalized a second time?  That's the part I don't understand.
> 
> It has been reinitialized to call the function by hand (if you set
> debug frame 1 and call something by hand, you'll see a few
> refreshes), but it is not been refreshed after finishing the
> handmade call. We might still have dummy frames or incorrect
> information leftover from the function call.

If the handmade (inferior) call can leave invalid frames in the frame
cache, then the correct thing (I would think) is to flush the frame
cache after making the inferior call.

Thanks,
Andrew

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

[Andrew Burgess]

* Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> [2022-03-28 14:57:29 -0300]:

> 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 solution, 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.
> We have to use it directly instead of using the simpler
> scoped_restore_selected_frame because the latter may save a null frame_ID
> when the saved frame is the current frame, and restoring the null frame
> wouldn't rebuild the frame cache.
> 
> 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.
> 
> This patch has been regression tested on x86_64
> 
> ---
>  gdb/infcmd.c                                  |   2 +
>  gdb/infrun.c                                  |   7 +
>  gdb/mi/mi-cmd-stack.c                         |   6 +-
>  gdb/stack.c                                   |  20 ++-
>  .../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, 261 insertions(+), 4 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/infcmd.c b/gdb/infcmd.c
> index ea06ceb992c..ee5b7ddf59d 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1847,6 +1847,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)
>  	printf_filtered (_("Run back to call of "));
>        else
> @@ -1860,6 +1861,7 @@ finish_command (const char *arg, int from_tty)
>  	}
>  
>        print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
> +      frame = get_prev_frame (frame_find_by_id (id));

You need to be really careful here.  get_prev_frame requires a
non-null frame_info*, while frame_find_by_id is not guaranteed to
always return a non-null frame_info*.

Remember, part (the largest part?) of the reason we flush the frame
cache is that we've done some action which _might_ have changed the
inferior state.  The stack might be completely different.  There's
absolutely no guarantee that the frame matching ID still exists.

So you likely need something like:

  frame_info *frame = frame_find_by_id (id);
  if (frame == nullptr)
    error (_("frame disappeared, or some other message..."));
  frame = get_prev_frame (frame);

>      }
>  
>    if (execution_direction == EXEC_REVERSE)
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 737710f5bae..bf519737d12 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -8308,8 +8308,15 @@ print_stop_location (const target_waitstatus &ws)
>       SRC_LINE: Print only source line
>       LOCATION: Print only location
>       SRC_AND_LOC: Print location and source line.  */
> +  int frame_cache_count = get_frame_cache_generation ();
>    if (do_frame_printing)
>      print_stack_frame (get_selected_frame (NULL), 0, source_flag, 1);
> +  /* If there are more frame cache generations after we print a stack frame
> +     we may have invalidated the cache with a pretty printer.
> +     If this happened, the safest option is to restart the whole cache,
> +     otherwise GDB may crash because of an use-after-free bug.  */
> +  if (frame_cache_count < get_frame_cache_generation ())
> +    reinit_frame_cache ();
>  }
>  
>  /* See infrun.h.  */
> diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
> index e894411765a..ce5b93420ca 100644
> --- a/gdb/mi/mi-cmd-stack.c
> +++ b/gdb/mi/mi-cmd-stack.c
> @@ -171,15 +171,19 @@ mi_cmd_stack_list_frames (const char *command, char **argv, int argc)
>      {
>        /* Now let's print the frames up to frame_high, or until there are
>  	 frames in the stack.  */
> -      for (;
> +      for (struct frame_id fid;
>  	   fi && (i <= frame_high || frame_high == -1);
>  	   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.  */
> +	  fid = get_frame_id (fi);

I found this comment really confusing.  Honestly, I think it would be
clearer without the comment.  I assume the problem was trying to
update the fid value in the 'i++, fi = get_prev_frame (fi)' line
maybe?  But I'm not totally sure.

>  	  /* 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);

The same here w.r.t fi possibly being nullptr.

>  	}
>      }
>  }
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 10da88b88e5..198eb9e485f 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -364,11 +364,14 @@ 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);
> +	set_current_sal_from_frame (frame_find_by_id (fid));

Again with frame_find_by_id.  set_current_sal_from_frame (or the
things it calls) does require that the frame be non-nullptr.  Even if
it did handle nullptr we'd probably want to error early.

There's a bunch more of these below, but I've not bothered to call
them all out.

Thanks,
Andrew

>      }
>    catch (const gdb_exception_error &e)
>      {
> @@ -742,6 +745,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 +907,7 @@ print_frame_args (const frame_print_options &fp_opts,
>  	    }
>  
>  	  first = 0;
> +	  frame = frame_find_by_id (printed_frame);
>  	}
>      }
>  
> @@ -1109,12 +1115,19 @@ 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);
> +    }
>  
>    source_print = (print_what == SRC_LINE || print_what == SRC_AND_LOC);
>  
> @@ -2060,6 +2073,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 +2081,9 @@ 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 ((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..11a1b2e62fb
> --- /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\\).*" ".*TAG: first frame.*"]
> +    }
> +}
> +
> +# 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
>