Re: [PATCH] [gdb/testsuite] test a function call by hand fron pretty printer

[Joel Brobecker <brobecker@adacore.com>]

Hello Bruno,

On Wed, Feb 23, 2022 at 03:57:29PM -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.

Thanks for this new testcase.

A few comments below.

> ---
>  .../gdb.python/pretty-print-call-by-hand.c    |  44 ++++++
>  .../gdb.python/pretty-print-call-by-hand.exp  | 148 ++++++++++++++++++
>  .../gdb.python/pretty-print-call-by-hand.py   |  25 +++
>  3 files changed, 217 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..0dcddc9c1e6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
> @@ -0,0 +1,44 @@
> +/* 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{

Can you place the opening curly brace to the start of the next line,
please?

Same remark throughout the rest of this code: Although it is less
important for testcases, we made the decision that, whenever possible,
we would try to keep our testcases adhering to the same coding standards
as with the rest of the code, wheneve

> +    char *x;

Also part of our coding style: Can you use an indentation level of
2 characters, rather than 4?

> +};
> +
> +void rec (int i){

Same as above, for the avoidance of doubt.

> +    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: inside the frame */
> +    g (mt, depth - 1);
> +}
> +
> +int main (){
> +    struct mytype mt;
> +    mt.x = "hello world";
> +    g (mt, 10); /* TAG: break here */
> +    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..6ba3fdcc7b3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
> @@ -0,0 +1,148 @@
> +# 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 Python-based
> +# pretty-printing for the CLI.
> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +
> +# Start with a fresh gdb.
> +gdb_exit
> +gdb_start

I think you can drop the gdb_exit/gdb_start part (it is taken care
of by prepare_for_testing).

> +
> +# 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
> +}
> +
> +proc start_test_no_pretty {} {

Can you please document this function the same way we document
functions in GDB's actual code? We're normally somewhat more
relaxed about this in our testsuite, but in this case, I spent
some time asking that you remove some "return" statements because
these are typically not used in our testsuite, only to understand
a bit later why you are using them. A comment would have avoided
me spending time commenting on something, and so I am thinking
it might help others as well.

> +    global srcdir subdir testfile binfile
> +
> +    # Start with a fresh gdb.
> +    # This is important because the test can crash GDB.
> +
> +    gdb_exit
> +    gdb_start
> +    gdb_reinitialize_dir $srcdir/$subdir
> +    gdb_load ${binfile}

You can use clean_restart <executable> the 4 function calls above.

> +    if { ![runto_main] } then {
> +	perror "couldn't run to breakpoint"

Can you use "untested" rather than "perror"? For reference,
our testcase cookbook wiki page references the following file
as a template you can take a look at:

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/testsuite/gdb.base/template.exp;h=007a1472fee6866e76c3c6d991251502f278a615;hb=HEAD

> +	return -1
> +    }
> +    #gdb_test_no_output "source ${testfile}.py" "load python file"

Can you remove this commented out code, please?

> +    #let GDB get to the return line

Can you format this with a space after the "#", an uppercase letter
to start the sentence, and then a period at the end?

> +    gdb_breakpoint [gdb_get_line_number "TAG: break here" ${testfile}.c ]
> +    gdb_continue_to_breakpoint "TAG: break here" ".*TAG: break here.*"
> +
> +    gdb_test_no_output "set print pretty on" "starting to pretty print"

There is an oddity here; the name of the function seems to indicate
that this should be "off" rather than "on", here. Can you confirm
whether this is correct or not?

> +    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
> +}
> +
> +proc start_test_pretty {} {

This function seems to be identical to the start_test_no_pretty.
Can you perhaps factorize this into a single function, possibly
with one argument being the "on/off" value to be used for
the "print pretty" setting?

If, for some reason I missed, it is better not to factorize in
this case, the same comments I made above should apply here.

> +    global srcdir subdir testfile binfile
> +
> +    # Start with a fresh gdb.
> +    # This is important because the test can crash GDB.
> +
> +    gdb_exit
> +    gdb_start
> +    gdb_reinitialize_dir $srcdir/$subdir
> +    gdb_load ${binfile}
> +
> +    if { ![runto_main] } then {
> +	perror "couldn't run to breakpoint"
> +	return -1
> +    }
> +    #gdb_test_no_output "source ${testfile}.py" "load python file"
> +
> +    #let GDB get to the return line
> +    gdb_breakpoint [gdb_get_line_number "TAG: break here" ${testfile}.c ]
> +    gdb_continue_to_breakpoint "TAG: break here" ".*TAG: break here.*"
> +
> +    gdb_test "step" ".*" #enter the problematic frame
> +
> +    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_pretty] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "backtrace -frame-arguments all" ".*"
> +    }
> +}
> +
> +# Testing the down command
> +with_test_prefix "frame movement up" {
> +    if { [start_test_pretty] == 0 } {
> +	gdb_test "up" ".*"
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "down" ".*"
> +    }
> +}
> +
> +# Testing the up command
> +with_test_prefix "frame movement down" {
> +    if { [start_test_pretty] == 0 } {
> +	gdb_test_no_output "set print pretty off" "removing pretty printing"
> +	gdb_breakpoint [gdb_get_line_number "TAG: inside the frame" ${testfile}.c ]
> +	gdb_continue_to_breakpoint "TAG: inside the frame" ".*TAG: inside the frame.*"
> +	gdb_test_no_output "set print pretty on" "removing pretty printing"
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "up" ".*"
> +    }
> +}
> +
> +# Testing the finish command
> +with_test_prefix "frame exit through finish" {
> +    if { [start_test_pretty] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "finish" ".*"
> +    }
> +}
> +
> +# Testing the step command
> +with_test_prefix "frame enter through step" {
> +    if { [start_test_no_pretty] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "step" ".*"
> +    }
> +}
> +
> +# Testing the continue command
> +with_test_prefix "frame enter through continue" {
> +    if { [start_test_no_pretty] == 0 } {
> +	gdb_test "backtrace -frame-arguments all" ".*"
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_breakpoint [gdb_get_line_number "TAG: inside the frame" ${testfile}.c ]
> +	gdb_continue_to_breakpoint "TAG: inside the frame" ".*TAG: inside the 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..978876723fc
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
> @@ -0,0 +1,25 @@
> +class MytypePrinter:
> +    "pretty print my type"

Can you use """ for this docstring? This is what's typical and it will
help those who don't read Python everyday and are more used to seeing
"""xxx""" strings rather than "x" ones when it comes to documentation.

> +
> +    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
> 

-- 
Joel