From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id A35793857353 for ; Wed, 18 May 2022 11:24:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A35793857353 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-137-vnzDv7iQMTCkdjyeL4GRcw-1; Wed, 18 May 2022 07:24:03 -0400 X-MC-Unique: vnzDv7iQMTCkdjyeL4GRcw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C40EB29DD9AB for ; Wed, 18 May 2022 11:24:02 +0000 (UTC) Received: from [10.97.116.34] (ovpn-116-34.gru2.redhat.com [10.97.116.34]) by smtp.corp.redhat.com (Postfix) with ESMTPS id ECB921121314 for ; Wed, 18 May 2022 11:24:01 +0000 (UTC) Message-ID: <75aa2fe2-9ed0-83ed-e1de-4b814849707a@redhat.com> Date: Wed, 18 May 2022 08:24:00 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: [PING] [PATCH v2] gdb/stack.c: avoid stale pointers when printing frame arguments To: "gdb-patches@sourceware.org" References: <20220504191036.143360-1-blarsen@redhat.com> From: Bruno Larsen In-Reply-To: <20220504191036.143360-1-blarsen@redhat.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 May 2022 11:24:07 -0000 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 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 . */ > + > +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 . > + > +# 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 . > + > + > +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)