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

[Bruno Larsen <blarsen@redhat.com>]

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)