From: Andrew Burgess <aburgess@redhat.com>
To: Bruno Larsen <blarsen@redhat.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] gdb/stack.c: avoid stale pointers when printing frame arguments
Date: Tue, 5 Apr 2022 17:54:01 +0100 [thread overview]
Message-ID: <20220405165401.GY1212730@redhat.com> (raw)
In-Reply-To: <20220328175729.52215-1-blarsen@redhat.com>
* 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
>
prev parent reply other threads:[~2022-04-05 16:54 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-28 17:57 Bruno Larsen
2022-04-04 19:06 ` Tom Tromey
2022-04-04 21:42 ` Bruno Larsen
2022-04-05 13:58 ` Tom Tromey
2022-04-05 14:47 ` Bruno Larsen
2022-04-05 16:39 ` Andrew Burgess
2022-04-05 16:54 ` Andrew Burgess [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220405165401.GY1212730@redhat.com \
--to=aburgess@redhat.com \
--cc=blarsen@redhat.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).