From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com [IPv6:2a00:1450:4864:20::42e]) by sourceware.org (Postfix) with ESMTPS id 97EE33858D28 for ; Sun, 13 Mar 2022 06:14:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 97EE33858D28 Received: by mail-wr1-x42e.google.com with SMTP id u10so19054959wra.9 for ; Sat, 12 Mar 2022 22:14:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=XJv1/Kpfikih/st2DDmmRbUyWCNh5aq9jMfnYicfG/Q=; b=R6lvbSH0Xhup0xOUkUxpl1hlvCpCh2HpiN0yi50BP5Nekhy7UDvJninjwv8OEb3+P6 yS8mN/r5czY61LiYuj/l+HRfFdGcsIXf9nSFdPuMS8pY/pgtepqxukkRgOinxiiFMQku PaZh8IjqnicOcEuTMemWJA/F1jVkdkWgAlVppK3zOfOAgKSDp8bDrtbl0N3ulDgFxNu4 9018LDItYoYzF6ZaGiRuUmH6WYyQIURoihTvjEzxM8QXf0/VvjUmp6iwt9/oQxkOxvws fG0YI/syxX116iQgOaf4tNs3Hr7vL/9sXiDBy5ybfgZebeiMbOruGR1JGlNispk3s2NL w5hQ== X-Gm-Message-State: AOAM532cUvgVjQehgoTQSGClGfhkqAvhtN1/AN7FouzjNPUyVF4ZeZ+f xzMudu4NFJOe+9Mlpq/gERcZOxMDrQ9S X-Google-Smtp-Source: ABdhPJzG4UGYANKyuaJkFCklqBH8F7FQ/cSbUHuprhJd9R0bTxpgGiGAh+YbGSUAXydFW2CpINWjkg== X-Received: by 2002:a5d:514d:0:b0:1f1:dfd6:7273 with SMTP id u13-20020a5d514d000000b001f1dfd67273mr12063494wrt.464.1647152040342; Sat, 12 Mar 2022 22:14:00 -0800 (PST) Received: from takamaka.home ([2a01:cb22:1d5:1100:649a:6ea9:3e07:fe1f]) by smtp.gmail.com with ESMTPSA id k10-20020adfe3ca000000b001f0329ba94csm16420518wrm.18.2022.03.12.22.13.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 12 Mar 2022 22:13:59 -0800 (PST) Received: by takamaka.home (Postfix, from userid 1000) id A92FBA4AD3; Sun, 13 Mar 2022 10:13:56 +0400 (+04) Date: Sun, 13 Mar 2022 10:13:56 +0400 From: Joel Brobecker To: Bruno Larsen via Gdb-patches Cc: Joel Brobecker Subject: Re: [PATCH v2] [gdb/testsuite] test a function call by hand fron pretty printer Message-ID: References: <20220223185729.25609-1-blarsen@redhat.com> <20220307130924.23940-1-blarsen@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220307130924.23940-1-blarsen@redhat.com> X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Sun, 13 Mar 2022 06:14:04 -0000 On Mon, Mar 07, 2022 at 10:09:24AM -0300, Bruno Larsen via Gdb-patches wrote: > The test case added here is testing the bug gdb/28856, where calling a > function by hand from a pretty printer makes GDB crash. There are 6 > mechanisms to trigger this crash in the current test, using the commands > backtrace, up, down, finish, step and continue. Since the failure happens > because of use-after-free (more details below) the tests will always > have a chance of passing through sheer luck, but anecdotally they seem > to fail all of the time. > > The reason GDB is crashing is a use-after-free problem. The above > mentioned functions save a pointer to the current frame's information, > then calls the pretty printer, and uses the saved pointer for different > reasons, depending on the function. The issue happens because > call_function_by_hand needs to reset the obstack to get the current > frame, invalidating the saved pointer. > --- > > Changes from v1: > * make .c follow GDB's coding guidelines more closely > * Documented .exp file better > * renamed and refactored TCL proc to make it less confusing Thanks for this new version. I noticed a typo in the subject "fron" -> "from". A few more (minor) comments below. > --- > .../gdb.python/pretty-print-call-by-hand.c | 48 ++++++++ > .../gdb.python/pretty-print-call-by-hand.exp | 115 ++++++++++++++++++ > .../gdb.python/pretty-print-call-by-hand.py | 25 ++++ > 3 files changed, 188 insertions(+) > 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/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..c215c78541b > --- /dev/null > +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c > @@ -0,0 +1,48 @@ > +/* 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) Can you put "rec" at the start of the next line? | void | rec (int i) The GCS explains that this allows "diff -p" to print the function name when showing hunks. Same comment for the other functions below. > +{ > + 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 (){ Same here. And also, can you put the opening curly brace on the next line, please? > + 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..7508be20a03 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp > @@ -0,0 +1,115 @@ > +# 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 > +} > + > +# Testing the backtrace command. > +with_test_prefix "frame print" { > + if { [start_test "TAG: final frame"] == 0 } { > + setup_kfail gdb/28856 "*-*-*" > + gdb_test "backtrace -frame-arguments all" ".*" Even though this is not critical for this testcase, I'd like to suggest we do some minimal verification that the output seems about right. What do you think? (here, and all the tests below). > + } > +} > + > +# Testing the down command. > +with_test_prefix "frame movement down" { > + if { [start_test "TAG: first frame"] == 0 } { > + gdb_test "up" ".*" > + setup_kfail gdb/28856 "*-*-*" > + gdb_test "down" ".*" > + } > +} > + > +# Testing the up command. > +with_test_prefix "frame movement up" { > + if { [start_test "TAG: final frame"] == 0 } { > + setup_kfail gdb/28856 "*-*-*" > + gdb_test "up" ".*" > + } > +} > + > +# Testing the finish command. > +with_test_prefix "frame exit through finish" { > + if { [start_test "TAG: first frame"] == 0 } { > + setup_kfail gdb/28856 "*-*-*" > + gdb_test "finish" ".*" > + } > +} > + > +# Testing the step command. > +with_test_prefix "frame enter through step" { > + if { [start_test "TAG: outside the frame"] == 0 } { > + setup_kfail gdb/28856 "*-*-*" > + gdb_test "step" ".*" > + } > +} > + > +# Testing the continue command. > +with_test_prefix "frame enter through continue" { > + if { [start_test "TAG: outside the frame"] == 0 } { > + setup_kfail gdb/28856 "*-*-*" > + 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..627011a9ac9 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py > @@ -0,0 +1,25 @@ > +class MytypePrinter: Can you add a copyright notice to this file, please? > + """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) -- Joel